* Re: [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create()
2024-11-13 21:03 [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create() Jann Horn
@ 2024-11-14 7:15 ` Boris Brezillon
2024-11-14 10:54 ` Liviu Dudau
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2024-11-14 7:15 UTC (permalink / raw)
To: Jann Horn
Cc: Steven Price, Liviu Dudau, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Mary Guillemard,
dri-devel, linux-kernel, stable
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create()
2024-11-13 21:03 [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create() Jann Horn
2024-11-14 7:15 ` Boris Brezillon
@ 2024-11-14 10:54 ` Liviu Dudau
2024-11-14 11:08 ` Steven Price
2025-07-18 15:55 ` Steven Price
3 siblings, 0 replies; 5+ messages in thread
From: Liviu Dudau @ 2024-11-14 10:54 UTC (permalink / raw)
To: Jann Horn
Cc: Boris Brezillon, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Mary Guillemard,
dri-devel, linux-kernel, stable
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>
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create()
2024-11-13 21:03 [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create() Jann Horn
2024-11-14 7:15 ` Boris Brezillon
2024-11-14 10:54 ` Liviu Dudau
@ 2024-11-14 11:08 ` Steven Price
2025-07-18 15:55 ` Steven Price
3 siblings, 0 replies; 5+ messages in thread
From: Steven Price @ 2024-11-14 11:08 UTC (permalink / raw)
To: Jann Horn, Boris Brezillon, Liviu Dudau
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Mary Guillemard, dri-devel, linux-kernel, stable
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create()
2024-11-13 21:03 [PATCH] drm/panthor: Fix memory leak in panthor_ioctl_group_create() Jann Horn
` (2 preceding siblings ...)
2024-11-14 11:08 ` Steven Price
@ 2025-07-18 15:55 ` Steven Price
3 siblings, 0 replies; 5+ messages in thread
From: Steven Price @ 2025-07-18 15:55 UTC (permalink / raw)
To: Jann Horn, Boris Brezillon, Liviu Dudau
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Mary Guillemard, dri-devel, linux-kernel, stable
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>
I've just discovered that this was never merged. Sorry for taking so
long, I've now applied this to drm-misc-next.
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread