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(-)
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, 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
linux-stable-mirror@lists.linaro.org
-
Boris Brezillon -
Jann Horn -
Liviu Dudau -
Steven Price