| Message ID | 20241113-panthor-fix-gcq-bailout-v1-1-654307254d68@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Fix memory leak in panthor_ioctl_group_create() | expand |
On Wed, 13 Nov 2024 22:03:39 +0100 Jann Horn <jannh@google.com> wrote: > When bailing out due to group_priority_permit() failure, the queue_args > need to be freed. Fix it by rearranging the function to use the > goto-on-error pattern, such that the success case flows straight without > indentation while error cases jump forward to cleanup. > > Cc: stable@vger.kernel.org > Fixes: 5f7762042f8a ("drm/panthor: Restrict high priorities on group_create") > Signed-off-by: Jann Horn <jannh@google.com> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > testcase: > ``` > #include <err.h> > #include <fcntl.h> > #include <stddef.h> > #include <sys/ioctl.h> > #include <drm/panthor_drm.h> > > #define SYSCHK(x) ({ \ > typeof(x) __res = (x); \ > if (__res == (typeof(x))-1) \ > err(1, "SYSCHK(" #x ")"); \ > __res; \ > }) > > #define GPU_PATH "/dev/dri/by-path/platform-fb000000.gpu-card" > > int main(void) { > int fd = SYSCHK(open(GPU_PATH, O_RDWR)); > > while (1) { > struct drm_panthor_queue_create qc[16] = {}; > struct drm_panthor_group_create gc = { > .queues = { > .stride = sizeof(struct drm_panthor_queue_create), > .count = 16, > .array = (unsigned long)qc > }, > .priority = PANTHOR_GROUP_PRIORITY_HIGH+1/*invalid*/ > }; > ioctl(fd, DRM_IOCTL_PANTHOR_GROUP_CREATE, &gc); > } > } > ``` > > I have tested that without this patch, after running the testcase for a > few seconds and then manually killing it, 2G of RAM in kmalloc-128 have > been leaked. With the patch applied, the memory leak is gone. > > (By the way, get_maintainer.pl suggests that I also send this patch to > the general DRM maintainers and the DRM-misc maintainers; looking at > MAINTAINERS, it looks like it is normal that the general DRM maintainers > are listed for everything under drivers/gpu/, but DRM-misc has exclusion > rules for a bunch of drivers but not panthor. I don't know if that is > intentional.) > --- > drivers/gpu/drm/panthor/panthor_drv.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index c520f156e2d73f7e735f8bf2d6d8e8efacec9362..815c23cff25f305d884e8e3e263fa22888f7d5ce 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1032,14 +1032,15 @@ static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, > > ret = group_priority_permit(file, args->priority); > if (ret) > - return ret; > + goto out; > > ret = panthor_group_create(pfile, args, queue_args); > - if (ret >= 0) { > - args->group_handle = ret; > - ret = 0; > - } > + if (ret < 0) > + goto out; > + args->group_handle = ret; > + ret = 0; > > +out: > kvfree(queue_args); > return ret; > } > > --- > base-commit: 9f8e716d46c68112484a23d1742d9ec725e082fc > change-id: 20241113-panthor-fix-gcq-bailout-2d9ac36590ed >
On Wed, Nov 13, 2024 at 10:03:39PM +0100, Jann Horn wrote: > When bailing out due to group_priority_permit() failure, the queue_args > need to be freed. Fix it by rearranging the function to use the > goto-on-error pattern, such that the success case flows straight without > indentation while error cases jump forward to cleanup. > > Cc: stable@vger.kernel.org > Fixes: 5f7762042f8a ("drm/panthor: Restrict high priorities on group_create") > Signed-off-by: Jann Horn <jannh@google.com> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > --- > testcase: > ``` > #include <err.h> > #include <fcntl.h> > #include <stddef.h> > #include <sys/ioctl.h> > #include <drm/panthor_drm.h> > > #define SYSCHK(x) ({ \ > typeof(x) __res = (x); \ > if (__res == (typeof(x))-1) \ > err(1, "SYSCHK(" #x ")"); \ > __res; \ > }) > > #define GPU_PATH "/dev/dri/by-path/platform-fb000000.gpu-card" > > int main(void) { > int fd = SYSCHK(open(GPU_PATH, O_RDWR)); > > while (1) { > struct drm_panthor_queue_create qc[16] = {}; > struct drm_panthor_group_create gc = { > .queues = { > .stride = sizeof(struct drm_panthor_queue_create), > .count = 16, > .array = (unsigned long)qc > }, > .priority = PANTHOR_GROUP_PRIORITY_HIGH+1/*invalid*/ > }; > ioctl(fd, DRM_IOCTL_PANTHOR_GROUP_CREATE, &gc); > } > } > ``` > > I have tested that without this patch, after running the testcase for a > few seconds and then manually killing it, 2G of RAM in kmalloc-128 have > been leaked. With the patch applied, the memory leak is gone. > > (By the way, get_maintainer.pl suggests that I also send this patch to > the general DRM maintainers and the DRM-misc maintainers; looking at > MAINTAINERS, it looks like it is normal that the general DRM maintainers > are listed for everything under drivers/gpu/, but DRM-misc has exclusion > rules for a bunch of drivers but not panthor. I don't know if that is > intentional.) It is intentional, the drivers excluded from DRM-misc have their own trees and maintainers. DRM-misc is more of a group maintainership where everyone with maintainer rights in DRM-misc can push patches and send pull requests. Not all of us do it that often, so the most active ones are listed in the MAINTAINERS file. Best regards, Liviu > --- > drivers/gpu/drm/panthor/panthor_drv.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index c520f156e2d73f7e735f8bf2d6d8e8efacec9362..815c23cff25f305d884e8e3e263fa22888f7d5ce 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1032,14 +1032,15 @@ static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, > > ret = group_priority_permit(file, args->priority); > if (ret) > - return ret; > + goto out; > > ret = panthor_group_create(pfile, args, queue_args); > - if (ret >= 0) { > - args->group_handle = ret; > - ret = 0; > - } > + if (ret < 0) > + goto out; > + args->group_handle = ret; > + ret = 0; > > +out: > kvfree(queue_args); > return ret; > } > > --- > base-commit: 9f8e716d46c68112484a23d1742d9ec725e082fc > change-id: 20241113-panthor-fix-gcq-bailout-2d9ac36590ed > > -- > Jann Horn <jannh@google.com> >
On 13/11/2024 21:03, Jann Horn wrote: > When bailing out due to group_priority_permit() failure, the queue_args > need to be freed. Fix it by rearranging the function to use the > goto-on-error pattern, such that the success case flows straight without > indentation while error cases jump forward to cleanup. > > Cc: stable@vger.kernel.org > Fixes: 5f7762042f8a ("drm/panthor: Restrict high priorities on group_create") > Signed-off-by: Jann Horn <jannh@google.com> Reviewed-by: Steven Price <steven.price@arm.com> Thanks, Steve > --- > testcase: > ``` > #include <err.h> > #include <fcntl.h> > #include <stddef.h> > #include <sys/ioctl.h> > #include <drm/panthor_drm.h> > > #define SYSCHK(x) ({ \ > typeof(x) __res = (x); \ > if (__res == (typeof(x))-1) \ > err(1, "SYSCHK(" #x ")"); \ > __res; \ > }) > > #define GPU_PATH "/dev/dri/by-path/platform-fb000000.gpu-card" > > int main(void) { > int fd = SYSCHK(open(GPU_PATH, O_RDWR)); > > while (1) { > struct drm_panthor_queue_create qc[16] = {}; > struct drm_panthor_group_create gc = { > .queues = { > .stride = sizeof(struct drm_panthor_queue_create), > .count = 16, > .array = (unsigned long)qc > }, > .priority = PANTHOR_GROUP_PRIORITY_HIGH+1/*invalid*/ > }; > ioctl(fd, DRM_IOCTL_PANTHOR_GROUP_CREATE, &gc); > } > } > ``` > > I have tested that without this patch, after running the testcase for a > few seconds and then manually killing it, 2G of RAM in kmalloc-128 have > been leaked. With the patch applied, the memory leak is gone. > > (By the way, get_maintainer.pl suggests that I also send this patch to > the general DRM maintainers and the DRM-misc maintainers; looking at > MAINTAINERS, it looks like it is normal that the general DRM maintainers > are listed for everything under drivers/gpu/, but DRM-misc has exclusion > rules for a bunch of drivers but not panthor. I don't know if that is > intentional.) > --- > drivers/gpu/drm/panthor/panthor_drv.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index c520f156e2d73f7e735f8bf2d6d8e8efacec9362..815c23cff25f305d884e8e3e263fa22888f7d5ce 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1032,14 +1032,15 @@ static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, > > ret = group_priority_permit(file, args->priority); > if (ret) > - return ret; > + goto out; > > ret = panthor_group_create(pfile, args, queue_args); > - if (ret >= 0) { > - args->group_handle = ret; > - ret = 0; > - } > + if (ret < 0) > + goto out; > + args->group_handle = ret; > + ret = 0; > > +out: > kvfree(queue_args); > return ret; > } > > --- > base-commit: 9f8e716d46c68112484a23d1742d9ec725e082fc > change-id: 20241113-panthor-fix-gcq-bailout-2d9ac36590ed >
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c index c520f156e2d73f7e735f8bf2d6d8e8efacec9362..815c23cff25f305d884e8e3e263fa22888f7d5ce 100644 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@ -1032,14 +1032,15 @@ static int panthor_ioctl_group_create(struct drm_device *ddev, void *data, ret = group_priority_permit(file, args->priority); if (ret) - return ret; + goto out; ret = panthor_group_create(pfile, args, queue_args); - if (ret >= 0) { - args->group_handle = ret; - ret = 0; - } + if (ret < 0) + goto out; + args->group_handle = ret; + ret = 0; +out: kvfree(queue_args); return ret; }
When bailing out due to group_priority_permit() failure, the queue_args need to be freed. Fix it by rearranging the function to use the goto-on-error pattern, such that the success case flows straight without indentation while error cases jump forward to cleanup. Cc: stable@vger.kernel.org Fixes: 5f7762042f8a ("drm/panthor: Restrict high priorities on group_create") Signed-off-by: Jann Horn <jannh@google.com> --- testcase: ``` #include <err.h> #include <fcntl.h> #include <stddef.h> #include <sys/ioctl.h> #include <drm/panthor_drm.h> #define SYSCHK(x) ({ \ typeof(x) __res = (x); \ if (__res == (typeof(x))-1) \ err(1, "SYSCHK(" #x ")"); \ __res; \ }) #define GPU_PATH "/dev/dri/by-path/platform-fb000000.gpu-card" int main(void) { int fd = SYSCHK(open(GPU_PATH, O_RDWR)); while (1) { struct drm_panthor_queue_create qc[16] = {}; struct drm_panthor_group_create gc = { .queues = { .stride = sizeof(struct drm_panthor_queue_create), .count = 16, .array = (unsigned long)qc }, .priority = PANTHOR_GROUP_PRIORITY_HIGH+1/*invalid*/ }; ioctl(fd, DRM_IOCTL_PANTHOR_GROUP_CREATE, &gc); } } ``` I have tested that without this patch, after running the testcase for a few seconds and then manually killing it, 2G of RAM in kmalloc-128 have been leaked. With the patch applied, the memory leak is gone. (By the way, get_maintainer.pl suggests that I also send this patch to the general DRM maintainers and the DRM-misc maintainers; looking at MAINTAINERS, it looks like it is normal that the general DRM maintainers are listed for everything under drivers/gpu/, but DRM-misc has exclusion rules for a bunch of drivers but not panthor. I don't know if that is intentional.) --- drivers/gpu/drm/panthor/panthor_drv.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) --- base-commit: 9f8e716d46c68112484a23d1742d9ec725e082fc change-id: 20241113-panthor-fix-gcq-bailout-2d9ac36590ed