* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-06-17 4:00 [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init Ben Skeggs
@ 2025-06-17 11:29 ` Philipp Stanner
2025-06-17 13:05 ` Danilo Krummrich
2025-07-07 8:27 ` Philipp Stanner
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Philipp Stanner @ 2025-06-17 11:29 UTC (permalink / raw)
To: Ben Skeggs, nouveau; +Cc: Danilo Krummrich
On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked. Fix failure paths to free allocated memory.
>
> Also add checks to ensure the allocations succeeded in the first
> place.
>
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
> GSP-RM")
Needs to +Cc the stable kernel
But, see below
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
> ---
> .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 20 +++++++++++++----
> --
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index baf42339f93e..b098a7555fde 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
This seems to be based on a code move that is not yet in mainline.
Therefore, backporting the bugfix to stable seems difficult. Since that
code move is already in drm-misc-next, it would seem that it can only
be solved with two distinct patches for stable and for -next.
But this needs to be judged by a maintainer.
> @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
> union acpi_object argv4 = {
> .buffer.type = ACPI_TYPE_BUFFER,
> .buffer.length = 4,
> - .buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
> }, *obj;
>
> caps->status = 0xffff;
> @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
> if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV,
> BIT_ULL(0x1a)))
> return;
>
> + argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> + if (!argv4.buffer.pointer)
> + return;
> +
This could be done immediately after the creation of argv4. That way
it's more difficult to have the leak again if something is inserted
later on.
> obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID,
> NVOP_DSM_REV, 0x1a, &argv4);
> if (!obj)
> - return;
> + goto done;
>
> if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> WARN_ON(obj->buffer.length != 4))
> - return;
> + goto done;
>
> caps->status = 0;
> caps->optimusCaps = *(u32 *)obj->buffer.pointer;
>
> +done:
> ACPI_FREE(obj);
>
> kfree(argv4.buffer.pointer);
> @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle,
> JT_METHOD_DATA *jt)
> union acpi_object argv4 = {
> .buffer.type = ACPI_TYPE_BUFFER,
> .buffer.length = sizeof(caps),
> - .buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
> }, *obj;
>
> jt->status = 0xffff;
>
> + argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> + if (!argv4.buffer.pointer)
> + return;
> +
> obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV,
> 0x1, &argv4);
> if (!obj)
> - return;
> + goto done;
>
> if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> WARN_ON(obj->buffer.length != 4))
> - return;
> + goto done;
>
> jt->status = 0;
> jt->jtCaps = *(u32 *)obj->buffer.pointer;
> jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
> jt->bSBIOSCaps = 0;
>
> +done:
'done' seems like a bad name considering that the operations are
aborted with a WARN_ON above. Better 'abort' or sth like that.
P.
> ACPI_FREE(obj);
>
> kfree(argv4.buffer.pointer);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-06-17 11:29 ` Philipp Stanner
@ 2025-06-17 13:05 ` Danilo Krummrich
2025-06-17 16:28 ` Danilo Krummrich
0 siblings, 1 reply; 9+ messages in thread
From: Danilo Krummrich @ 2025-06-17 13:05 UTC (permalink / raw)
To: Philipp Stanner; +Cc: Ben Skeggs, nouveau
On Tue, Jun 17, 2025 at 01:29:20PM +0200, Philipp Stanner wrote:
> On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > index baf42339f93e..b098a7555fde 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
>
> This seems to be based on a code move that is not yet in mainline.
It is, it did land with v6.16-rc1.
> Therefore, backporting the bugfix to stable seems difficult. Since that
> code move is already in drm-misc-next, it would seem that it can only
> be solved with two distinct patches for stable and for -next.
drm-misc-fixes is the relevant target branch and given the above, it contains
the code move as well.
However, you're right that this fix won't apply to anything before v6.16-rc1.
Given that, it makes sense to leave a note below the '---' line that this fix
won't apply before v6.16-rc1 and that a backported patch will be sent to stable
once this one hit Linus' tree.
> But this needs to be judged by a maintainer.
>
> > @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle,
> > CAPS_METHOD_DATA *caps)
> > union acpi_object argv4 = {
> > .buffer.type = ACPI_TYPE_BUFFER,
> > .buffer.length = 4,
> > - .buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL),
> > }, *obj;
> >
> > caps->status = 0xffff;
> > @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle,
> > CAPS_METHOD_DATA *caps)
> > if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV,
> > BIT_ULL(0x1a)))
> > return;
> >
> > + argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL);
> > + if (!argv4.buffer.pointer)
> > + return;
> > +
>
> This could be done immediately after the creation of argv4. That way
> it's more difficult to have the leak again if something is inserted
> later on.
I think the idea was to avoid a potential unwind path after acpi_check_dsm().
> > obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID,
> > NVOP_DSM_REV, 0x1a, &argv4);
> > if (!obj)
> > - return;
> > + goto done;
> >
> > if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> > WARN_ON(obj->buffer.length != 4))
> > - return;
> > + goto done;
> >
> > caps->status = 0;
> > caps->optimusCaps = *(u32 *)obj->buffer.pointer;
> >
> > +done:
> > ACPI_FREE(obj);
> >
> > kfree(argv4.buffer.pointer);
> > @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle,
> > JT_METHOD_DATA *jt)
> > union acpi_object argv4 = {
> > .buffer.type = ACPI_TYPE_BUFFER,
> > .buffer.length = sizeof(caps),
> > - .buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL),
> > }, *obj;
> >
> > jt->status = 0xffff;
> >
> > + argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL);
> > + if (!argv4.buffer.pointer)
> > + return;
> > +
> > obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV,
> > 0x1, &argv4);
> > if (!obj)
> > - return;
> > + goto done;
> >
> > if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> > WARN_ON(obj->buffer.length != 4))
> > - return;
> > + goto done;
> >
> > jt->status = 0;
> > jt->jtCaps = *(u32 *)obj->buffer.pointer;
> > jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
> > jt->bSBIOSCaps = 0;
> >
> > +done:
>
> 'done' seems like a bad name considering that the operations are
> aborted with a WARN_ON above. Better 'abort' or sth like that.
I think some neutral name is fine, since we also enter this code path when
everything went well, maybe 'out_free' or just 'free'?
> P.
>
> > ACPI_FREE(obj);
> >
> > kfree(argv4.buffer.pointer);
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-06-17 13:05 ` Danilo Krummrich
@ 2025-06-17 16:28 ` Danilo Krummrich
0 siblings, 0 replies; 9+ messages in thread
From: Danilo Krummrich @ 2025-06-17 16:28 UTC (permalink / raw)
To: Philipp Stanner; +Cc: Ben Skeggs, nouveau
On Tue, Jun 17, 2025 at 03:05:10PM +0200, Danilo Krummrich wrote:
> On Tue, Jun 17, 2025 at 01:29:20PM +0200, Philipp Stanner wrote:
> > On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > > index baf42339f93e..b098a7555fde 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> >
> > This seems to be based on a code move that is not yet in mainline.
>
> It is, it did land with v6.16-rc1.
>
> > Therefore, backporting the bugfix to stable seems difficult. Since that
> > code move is already in drm-misc-next, it would seem that it can only
> > be solved with two distinct patches for stable and for -next.
>
> drm-misc-fixes is the relevant target branch and given the above, it contains
> the code move as well.
>
> However, you're right that this fix won't apply to anything before v6.16-rc1.
> Given that, it makes sense to leave a note below the '---' line that this fix
> won't apply before v6.16-rc1 and that a backported patch will be sent to stable
> once this one hit Linus' tree.
Nevermind, I think this file wasn't split up and is correctly considered as
moved by git, so I think the patch should apply as is.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-06-17 4:00 [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init Ben Skeggs
2025-06-17 11:29 ` Philipp Stanner
@ 2025-07-07 8:27 ` Philipp Stanner
2025-07-07 14:31 ` Danilo Krummrich
2025-07-07 14:59 ` Danilo Krummrich
2025-08-05 9:16 ` Philipp Stanner
3 siblings, 1 reply; 9+ messages in thread
From: Philipp Stanner @ 2025-07-07 8:27 UTC (permalink / raw)
To: Ben Skeggs, nouveau; +Cc: Danilo Krummrich
On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked. Fix failure paths to free allocated memory.
>
> Also add checks to ensure the allocations succeeded in the first
> place.
If you've got a kmemleak trace, it would also be good to put it here in
the commit message so that we can distinguish this bug from potential
other leaks.
P.
>
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
> GSP-RM")
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
> ---
> .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 20 +++++++++++++----
> --
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index baf42339f93e..b098a7555fde 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
> union acpi_object argv4 = {
> .buffer.type = ACPI_TYPE_BUFFER,
> .buffer.length = 4,
> - .buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
> }, *obj;
>
> caps->status = 0xffff;
> @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
> if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV,
> BIT_ULL(0x1a)))
> return;
>
> + argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> + if (!argv4.buffer.pointer)
> + return;
> +
> obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID,
> NVOP_DSM_REV, 0x1a, &argv4);
> if (!obj)
> - return;
> + goto done;
>
> if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> WARN_ON(obj->buffer.length != 4))
> - return;
> + goto done;
>
> caps->status = 0;
> caps->optimusCaps = *(u32 *)obj->buffer.pointer;
>
> +done:
> ACPI_FREE(obj);
>
> kfree(argv4.buffer.pointer);
> @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle,
> JT_METHOD_DATA *jt)
> union acpi_object argv4 = {
> .buffer.type = ACPI_TYPE_BUFFER,
> .buffer.length = sizeof(caps),
> - .buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
> }, *obj;
>
> jt->status = 0xffff;
>
> + argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> + if (!argv4.buffer.pointer)
> + return;
> +
> obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV,
> 0x1, &argv4);
> if (!obj)
> - return;
> + goto done;
>
> if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> WARN_ON(obj->buffer.length != 4))
> - return;
> + goto done;
>
> jt->status = 0;
> jt->jtCaps = *(u32 *)obj->buffer.pointer;
> jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
> jt->bSBIOSCaps = 0;
>
> +done:
> ACPI_FREE(obj);
>
> kfree(argv4.buffer.pointer);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-07-07 8:27 ` Philipp Stanner
@ 2025-07-07 14:31 ` Danilo Krummrich
2025-07-09 9:01 ` Philipp Stanner
0 siblings, 1 reply; 9+ messages in thread
From: Danilo Krummrich @ 2025-07-07 14:31 UTC (permalink / raw)
To: Philipp Stanner; +Cc: Ben Skeggs, nouveau
On 7/7/25 10:27 AM, Philipp Stanner wrote:
> On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
>> If any of the ACPI calls fail, memory allocated for the input buffer
>> would be leaked. Fix failure paths to free allocated memory.
>>
>> Also add checks to ensure the allocations succeeded in the first
>> place.
>
> If you've got a kmemleak trace, it would also be good to put it here in
> the commit message so that we can distinguish this bug from potential
> other leaks.
unreferenced object 0xffff8ed5029bfb28 (size 8):
comm "(udev-worker)", pid 464, jiffies 4294672444
hex dump (first 8 bytes):
7c b4 d4 79 01 59 36 6c |..y.Y6l
backtrace (crc 4461fdb7):
__kmalloc_cache_noprof+0x31b/0x410
r535_gsp_acpi_jt+0x7c/0x110 [nouveau]
r535_gsp_set_system_info+0x153/0x390 [nouveau]
r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
tu102_gsp_oneinit+0x124/0x440 [nouveau]
nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
nvkm_subdev_init_+0x33/0xa0 [nouveau]
nvkm_subdev_init+0x46/0x60 [nouveau]
nvkm_device_init+0x167/0x1f0 [nouveau]
nvkm_udevice_init+0x4b/0x70 [nouveau]
nvkm_object_init+0x3a/0x110 [nouveau]
nvkm_ioctl_new+0x13a/0x200 [nouveau]
nvkm_ioctl+0x9f/0x140 [nouveau]
nvif_object_ctor+0x11a/0x1a0 [nouveau]
nvif_device_ctor+0x2a/0x80 [nouveau]
nouveau_drm_device_new+0x157/0x2e0 [nouveau]
unreferenced object 0xffff8ed502a37580 (size 32):
comm "(udev-worker)", pid 464, jiffies 4294672444
hex dump (first 32 bytes):
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace (crc f1da05aa):
__kmalloc_noprof+0x3ac/0x500
acpi_ut_initialize_buffer+0x67/0xc0
acpi_evaluate_object+0x272/0x2c0
acpi_evaluate_dsm+0xb4/0x120
r535_gsp_acpi_jt+0xa3/0x110 [nouveau]
r535_gsp_set_system_info+0x153/0x390 [nouveau]
r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
tu102_gsp_oneinit+0x124/0x440 [nouveau]
nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
nvkm_subdev_init_+0x33/0xa0 [nouveau]
nvkm_subdev_init+0x46/0x60 [nouveau]
nvkm_device_init+0x167/0x1f0 [nouveau]
nvkm_udevice_init+0x4b/0x70 [nouveau]
nvkm_object_init+0x3a/0x110 [nouveau]
nvkm_ioctl_new+0x13a/0x200 [nouveau]
nvkm_ioctl+0x9f/0x140 [nouveau]
unreferenced object 0xffff8ed5029bf1c0 (size 8):
comm "(udev-worker)", pid 464, jiffies 4294672446
hex dump (first 8 bytes):
cc bb d4 79 01 59 3c 84 ...y.Y<.
backtrace (crc 30e1d939):
__kmalloc_cache_noprof+0x31b/0x410
r535_gsp_acpi_caps+0x7e/0x120 [nouveau]
r535_gsp_set_system_info+0x162/0x390 [nouveau]
r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
tu102_gsp_oneinit+0x124/0x440 [nouveau]
nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
nvkm_subdev_init_+0x33/0xa0 [nouveau]
nvkm_subdev_init+0x46/0x60 [nouveau]
nvkm_device_init+0x167/0x1f0 [nouveau]
nvkm_udevice_init+0x4b/0x70 [nouveau]
nvkm_object_init+0x3a/0x110 [nouveau]
nvkm_ioctl_new+0x13a/0x200 [nouveau]
nvkm_ioctl+0x9f/0x140 [nouveau]
nvif_object_ctor+0x11a/0x1a0 [nouveau]
nvif_device_ctor+0x2a/0x80 [nouveau]
nouveau_drm_device_new+0x157/0x2e0 [nouveau]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-07-07 14:31 ` Danilo Krummrich
@ 2025-07-09 9:01 ` Philipp Stanner
0 siblings, 0 replies; 9+ messages in thread
From: Philipp Stanner @ 2025-07-09 9:01 UTC (permalink / raw)
To: Danilo Krummrich; +Cc: Ben Skeggs, nouveau
On Mon, 2025-07-07 at 16:31 +0200, Danilo Krummrich wrote:
> On 7/7/25 10:27 AM, Philipp Stanner wrote:
> > On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> > > If any of the ACPI calls fail, memory allocated for the input buffer
> > > would be leaked. Fix failure paths to free allocated memory.
> > >
> > > Also add checks to ensure the allocations succeeded in the first
> > > place.
> >
> > If you've got a kmemleak trace, it would also be good to put it here in
> > the commit message so that we can distinguish this bug from potential
> > other leaks.
>
> unreferenced object 0xffff8ed5029bfb28 (size 8):
> comm "(udev-worker)", pid 464, jiffies 4294672444
> hex dump (first 8 bytes):
> 7c b4 d4 79 01 59 36 6c |..y.Y6l
> backtrace (crc 4461fdb7):
> __kmalloc_cache_noprof+0x31b/0x410
> r535_gsp_acpi_jt+0x7c/0x110 [nouveau]
> r535_gsp_set_system_info+0x153/0x390 [nouveau]
> r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
> tu102_gsp_oneinit+0x124/0x440 [nouveau]
> nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
> nvkm_subdev_init_+0x33/0xa0 [nouveau]
> nvkm_subdev_init+0x46/0x60 [nouveau]
> nvkm_device_init+0x167/0x1f0 [nouveau]
> nvkm_udevice_init+0x4b/0x70 [nouveau]
> nvkm_object_init+0x3a/0x110 [nouveau]
> nvkm_ioctl_new+0x13a/0x200 [nouveau]
> nvkm_ioctl+0x9f/0x140 [nouveau]
> nvif_object_ctor+0x11a/0x1a0 [nouveau]
> nvif_device_ctor+0x2a/0x80 [nouveau]
> nouveau_drm_device_new+0x157/0x2e0 [nouveau]
> unreferenced object 0xffff8ed502a37580 (size 32):
> comm "(udev-worker)", pid 464, jiffies 4294672444
> hex dump (first 32 bytes):
> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace (crc f1da05aa):
> __kmalloc_noprof+0x3ac/0x500
> acpi_ut_initialize_buffer+0x67/0xc0
> acpi_evaluate_object+0x272/0x2c0
> acpi_evaluate_dsm+0xb4/0x120
> r535_gsp_acpi_jt+0xa3/0x110 [nouveau]
> r535_gsp_set_system_info+0x153/0x390 [nouveau]
> r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
> tu102_gsp_oneinit+0x124/0x440 [nouveau]
> nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
> nvkm_subdev_init_+0x33/0xa0 [nouveau]
> nvkm_subdev_init+0x46/0x60 [nouveau]
> nvkm_device_init+0x167/0x1f0 [nouveau]
> nvkm_udevice_init+0x4b/0x70 [nouveau]
> nvkm_object_init+0x3a/0x110 [nouveau]
> nvkm_ioctl_new+0x13a/0x200 [nouveau]
> nvkm_ioctl+0x9f/0x140 [nouveau]
> unreferenced object 0xffff8ed5029bf1c0 (size 8):
> comm "(udev-worker)", pid 464, jiffies 4294672446
> hex dump (first 8 bytes):
> cc bb d4 79 01 59 3c 84 ...y.Y<.
> backtrace (crc 30e1d939):
> __kmalloc_cache_noprof+0x31b/0x410
> r535_gsp_acpi_caps+0x7e/0x120 [nouveau]
> r535_gsp_set_system_info+0x162/0x390 [nouveau]
> r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
> tu102_gsp_oneinit+0x124/0x440 [nouveau]
> nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
> nvkm_subdev_init_+0x33/0xa0 [nouveau]
> nvkm_subdev_init+0x46/0x60 [nouveau]
> nvkm_device_init+0x167/0x1f0 [nouveau]
> nvkm_udevice_init+0x4b/0x70 [nouveau]
> nvkm_object_init+0x3a/0x110 [nouveau]
> nvkm_ioctl_new+0x13a/0x200 [nouveau]
> nvkm_ioctl+0x9f/0x140 [nouveau]
> nvif_object_ctor+0x11a/0x1a0 [nouveau]
> nvif_device_ctor+0x2a/0x80 [nouveau]
> nouveau_drm_device_new+0x157/0x2e0 [nouveau]
>
Hm, interesting. That's not the memory leak we have observed. I've got
this trace for leaks of similar size:
unreferenced object 0xff110001518f9348 (size 8):
comm "kworker/0:4", pid 141, jiffies 4294719397
hex dump (first 8 bytes):
98 93 36 c3 c1 b9 80 3e ..6....>
backtrace (crc a04f7c42):
kmalloc_trace+0x28a/0x380
r535_gsp_rpc_set_system_info+0x7e8/0x17f0 [nouveau]
r535_gsp_oneinit+0x1ab0/0x2820 [nouveau]
nvkm_subdev_oneinit_.part.0+0x91/0x1a0 [nouveau]
nvkm_subdev_init_+0xfe/0x2b0 [nouveau]
nvkm_subdev_init+0xa7/0xc0 [nouveau]
nvkm_device_init+0x347/0x540 [nouveau]
nvkm_udevice_init+0x97/0xf0 [nouveau]
nvkm_object_init+0xc3/0x3e0 [nouveau]
nvkm_ioctl_new+0x36c/0x6b0 [nouveau]
nvkm_ioctl+0x1c0/0x4a0 [nouveau]
nvif_object_ctor+0x3c9/0x740 [nouveau]
nvif_device_ctor+0x2e/0x100 [nouveau]
nouveau_drm_device_new+0x367/0x910 [nouveau]
nouveau_drm_probe+0x10c/0x450 [nouveau]
local_pci_probe+0xe8/0x1a0
unreferenced object 0xff11000142e1bf50 (size 8):
comm "kworker/0:4", pid 141, jiffies 4294719397
hex dump (first 8 bytes):
88 b2 58 d0 d2 d7 ac 26 ..X....&
backtrace (crc 8a38119a):
kmalloc_trace+0x28a/0x380
r535_gsp_rpc_set_system_info+0xa8f/0x17f0 [nouveau]
r535_gsp_oneinit+0x1ab0/0x2820 [nouveau]
nvkm_subdev_oneinit_.part.0+0x91/0x1a0 [nouveau]
nvkm_subdev_init_+0xfe/0x2b0 [nouveau]
nvkm_subdev_init+0xa7/0xc0 [nouveau]
nvkm_device_init+0x347/0x540 [nouveau]
nvkm_udevice_init+0x97/0xf0 [nouveau]
nvkm_object_init+0xc3/0x3e0 [nouveau]
nvkm_ioctl_new+0x36c/0x6b0 [nouveau]
nvkm_ioctl+0x1c0/0x4a0 [nouveau]
nvif_object_ctor+0x3c9/0x740 [nouveau]
nvif_device_ctor+0x2e/0x100 [nouveau]
nouveau_drm_device_new+0x367/0x910 [nouveau]
nouveau_drm_probe+0x10c/0x450 [nouveau]
Ben, could those be related?
P.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-06-17 4:00 [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init Ben Skeggs
2025-06-17 11:29 ` Philipp Stanner
2025-07-07 8:27 ` Philipp Stanner
@ 2025-07-07 14:59 ` Danilo Krummrich
2025-08-05 9:16 ` Philipp Stanner
3 siblings, 0 replies; 9+ messages in thread
From: Danilo Krummrich @ 2025-07-07 14:59 UTC (permalink / raw)
To: Ben Skeggs; +Cc: nouveau
On 6/17/25 6:00 AM, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked. Fix failure paths to free allocated memory.
>
> Also add checks to ensure the allocations succeeded in the first place.
>
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
Applied to drm-misc-fixes, thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init
2025-06-17 4:00 [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init Ben Skeggs
` (2 preceding siblings ...)
2025-07-07 14:59 ` Danilo Krummrich
@ 2025-08-05 9:16 ` Philipp Stanner
3 siblings, 0 replies; 9+ messages in thread
From: Philipp Stanner @ 2025-08-05 9:16 UTC (permalink / raw)
To: Ben Skeggs, nouveau; +Cc: Danilo Krummrich
On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked. Fix failure paths to free allocated memory.
>
> Also add checks to ensure the allocations succeeded in the first place.
>
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
Tested-by: Philipp Stanner <phasta@kernel.org>
This
Closes: https://www.spinics.net/lists/nouveau/msg16319.html
P.
> ---
> .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 20 +++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index baf42339f93e..b098a7555fde 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle, CAPS_METHOD_DATA *caps)
> union acpi_object argv4 = {
> .buffer.type = ACPI_TYPE_BUFFER,
> .buffer.length = 4,
> - .buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL),
> }, *obj;
>
> caps->status = 0xffff;
> @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle, CAPS_METHOD_DATA *caps)
> if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV, BIT_ULL(0x1a)))
> return;
>
> + argv4.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL);
> + if (!argv4.buffer.pointer)
> + return;
> +
> obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV, 0x1a, &argv4);
> if (!obj)
> - return;
> + goto done;
>
> if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> WARN_ON(obj->buffer.length != 4))
> - return;
> + goto done;
>
> caps->status = 0;
> caps->optimusCaps = *(u32 *)obj->buffer.pointer;
>
> +done:
> ACPI_FREE(obj);
>
> kfree(argv4.buffer.pointer);
> @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle, JT_METHOD_DATA *jt)
> union acpi_object argv4 = {
> .buffer.type = ACPI_TYPE_BUFFER,
> .buffer.length = sizeof(caps),
> - .buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL),
> }, *obj;
>
> jt->status = 0xffff;
>
> + argv4.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL);
> + if (!argv4.buffer.pointer)
> + return;
> +
> obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV, 0x1, &argv4);
> if (!obj)
> - return;
> + goto done;
>
> if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> WARN_ON(obj->buffer.length != 4))
> - return;
> + goto done;
>
> jt->status = 0;
> jt->jtCaps = *(u32 *)obj->buffer.pointer;
> jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
> jt->bSBIOSCaps = 0;
>
> +done:
> ACPI_FREE(obj);
>
> kfree(argv4.buffer.pointer);
^ permalink raw reply [flat|nested] 9+ messages in thread