[PATCH] firmware: arm_scmi: Balance device refcount when destroying devices

[PATCH] firmware: arm_scmi: Balance device refcount when destroying devices

[Cristian Marussi]

Using device_find_child() to lookup the proper SCMI device to destroy
causes an unbalance in device refcount, since device_find_child() calls an
implicit get_device(): this, in turns, inhibits the call of the provided
release methods upon devices destruction.

As a consequence, one of the structures that is not freed properly upon
destruction is the internal struct device_private dev->p populated by the
drivers subsystem core.

KMemleak detects this situation since loading/unloding some SCMI driver
causes related devices to be created/destroyed without calling any
device_release method.

unreferenced object 0xffff00000f583800 (size 512):
  comm "insmod", pid 227, jiffies 4294912190
  hex dump (first 32 bytes):
    00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
    ff ff ff ff ff ff ff ff 60 36 1d 8a 00 80 ff ff  ........`6......
  backtrace (crc 114e2eed):
    kmemleak_alloc+0xbc/0xd8
    __kmalloc_cache_noprof+0x2dc/0x398
    device_add+0x954/0x12d0
    device_register+0x28/0x40
    __scmi_device_create.part.0+0x1bc/0x380
    scmi_device_create+0x2d0/0x390
    scmi_create_protocol_devices+0x74/0xf8
    scmi_device_request_notifier+0x1f8/0x2a8
    notifier_call_chain+0x110/0x3b0
    blocking_notifier_call_chain+0x70/0xb0
    scmi_driver_register+0x350/0x7f0
    0xffff80000a3b3038
    do_one_initcall+0x12c/0x730
    do_init_module+0x1dc/0x640
    load_module+0x4b20/0x5b70
    init_module_from_file+0xec/0x158

$ ./scripts/faddr2line ./vmlinux device_add+0x954/0x12d0
device_add+0x954/0x12d0:
kmalloc_noprof at include/linux/slab.h:901
(inlined by) kzalloc_noprof at include/linux/slab.h:1037
(inlined by) device_private_init at drivers/base/core.c:3510
(inlined by) device_add at drivers/base/core.c:3561

Balance device refcount by issuing a put_device() on devices found via
device_find_child().

Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/linux-arm-kernel/Z8nK3uFkspy61yjP@arm.com/T/#mc1f73a0ea5e41014fa145147b7b839fc988ada8f
CC: Sudeep Holla <sudeep.holla@arm.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
Fixes: d4f9dddd21f3 ("firmware: arm_scmi: Add dynamic scmi devices creation")
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/bus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 725c7c565333..7732e46d8a95 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -271,6 +271,9 @@ static struct scmi_device *scmi_child_dev_find(struct device *parent,
 	if (!dev)
 		return NULL;
 
+	/* Drop the refcnt bumped implicitly by device_find_child */
+	put_device(dev);
+
 	return to_scmi_dev(dev);
 }
 
-- 
2.47.0

Re: [PATCH] firmware: arm_scmi: Balance device refcount when destroying devices

[Alice Ryhl]

On Thu, Mar 06, 2025 at 06:54:47PM +0000, Cristian Marussi wrote:
> Using device_find_child() to lookup the proper SCMI device to destroy
> causes an unbalance in device refcount, since device_find_child() calls an
> implicit get_device(): this, in turns, inhibits the call of the provided
> release methods upon devices destruction.
> 
> As a consequence, one of the structures that is not freed properly upon
> destruction is the internal struct device_private dev->p populated by the
> drivers subsystem core.
> 
> KMemleak detects this situation since loading/unloding some SCMI driver
> causes related devices to be created/destroyed without calling any
> device_release method.
> 
> unreferenced object 0xffff00000f583800 (size 512):
>   comm "insmod", pid 227, jiffies 4294912190
>   hex dump (first 32 bytes):
>     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>     ff ff ff ff ff ff ff ff 60 36 1d 8a 00 80 ff ff  ........`6......
>   backtrace (crc 114e2eed):
>     kmemleak_alloc+0xbc/0xd8
>     __kmalloc_cache_noprof+0x2dc/0x398
>     device_add+0x954/0x12d0
>     device_register+0x28/0x40
>     __scmi_device_create.part.0+0x1bc/0x380
>     scmi_device_create+0x2d0/0x390
>     scmi_create_protocol_devices+0x74/0xf8
>     scmi_device_request_notifier+0x1f8/0x2a8
>     notifier_call_chain+0x110/0x3b0
>     blocking_notifier_call_chain+0x70/0xb0
>     scmi_driver_register+0x350/0x7f0
>     0xffff80000a3b3038
>     do_one_initcall+0x12c/0x730
>     do_init_module+0x1dc/0x640
>     load_module+0x4b20/0x5b70
>     init_module_from_file+0xec/0x158
> 
> $ ./scripts/faddr2line ./vmlinux device_add+0x954/0x12d0
> device_add+0x954/0x12d0:
> kmalloc_noprof at include/linux/slab.h:901
> (inlined by) kzalloc_noprof at include/linux/slab.h:1037
> (inlined by) device_private_init at drivers/base/core.c:3510
> (inlined by) device_add at drivers/base/core.c:3561
> 
> Balance device refcount by issuing a put_device() on devices found via
> device_find_child().
> 
> Reported-by: Alice Ryhl <aliceryhl@google.com>
> Closes: https://lore.kernel.org/linux-arm-kernel/Z8nK3uFkspy61yjP@arm.com/T/#mc1f73a0ea5e41014fa145147b7b839fc988ada8f
> CC: Sudeep Holla <sudeep.holla@arm.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> Fixes: d4f9dddd21f3 ("firmware: arm_scmi: Add dynamic scmi devices creation")
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

I was not able to reproduce the memory leak after applying this patch.

Tested-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] firmware: arm_scmi: Balance device refcount when destroying devices

[Cristian Marussi]

On Fri, Mar 07, 2025 at 09:34:15AM +0000, Alice Ryhl wrote:
> On Thu, Mar 06, 2025 at 06:54:47PM +0000, Cristian Marussi wrote:
> > Using device_find_child() to lookup the proper SCMI device to destroy
> > causes an unbalance in device refcount, since device_find_child() calls an
> > implicit get_device(): this, in turns, inhibits the call of the provided
> > release methods upon devices destruction.
> > 
> > As a consequence, one of the structures that is not freed properly upon
> > destruction is the internal struct device_private dev->p populated by the
> > drivers subsystem core.
> > 
> > KMemleak detects this situation since loading/unloding some SCMI driver
> > causes related devices to be created/destroyed without calling any
> > device_release method.
> > 
> > unreferenced object 0xffff00000f583800 (size 512):
> >   comm "insmod", pid 227, jiffies 4294912190
> >   hex dump (first 32 bytes):
> >     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
> >     ff ff ff ff ff ff ff ff 60 36 1d 8a 00 80 ff ff  ........`6......
> >   backtrace (crc 114e2eed):
> >     kmemleak_alloc+0xbc/0xd8
> >     __kmalloc_cache_noprof+0x2dc/0x398
> >     device_add+0x954/0x12d0
> >     device_register+0x28/0x40
> >     __scmi_device_create.part.0+0x1bc/0x380
> >     scmi_device_create+0x2d0/0x390
> >     scmi_create_protocol_devices+0x74/0xf8
> >     scmi_device_request_notifier+0x1f8/0x2a8
> >     notifier_call_chain+0x110/0x3b0
> >     blocking_notifier_call_chain+0x70/0xb0
> >     scmi_driver_register+0x350/0x7f0
> >     0xffff80000a3b3038
> >     do_one_initcall+0x12c/0x730
> >     do_init_module+0x1dc/0x640
> >     load_module+0x4b20/0x5b70
> >     init_module_from_file+0xec/0x158
> > 
> > $ ./scripts/faddr2line ./vmlinux device_add+0x954/0x12d0
> > device_add+0x954/0x12d0:
> > kmalloc_noprof at include/linux/slab.h:901
> > (inlined by) kzalloc_noprof at include/linux/slab.h:1037
> > (inlined by) device_private_init at drivers/base/core.c:3510
> > (inlined by) device_add at drivers/base/core.c:3561
> > 
> > Balance device refcount by issuing a put_device() on devices found via
> > device_find_child().
> > 
> > Reported-by: Alice Ryhl <aliceryhl@google.com>
> > Closes: https://lore.kernel.org/linux-arm-kernel/Z8nK3uFkspy61yjP@arm.com/T/#mc1f73a0ea5e41014fa145147b7b839fc988ada8f
> > CC: Sudeep Holla <sudeep.holla@arm.com>
> > CC: Catalin Marinas <catalin.marinas@arm.com>
> > Fixes: d4f9dddd21f3 ("firmware: arm_scmi: Add dynamic scmi devices creation")
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> I was not able to reproduce the memory leak after applying this patch.
> 
> Tested-by: Alice Ryhl <aliceryhl@google.com>

Thanks a lot for testing (and for the report) !

Cristian

Re: [PATCH] firmware: arm_scmi: Balance device refcount when destroying devices

[Sudeep Holla]

On Thu, 06 Mar 2025 18:54:47 +0000, Cristian Marussi wrote:
> Using device_find_child() to lookup the proper SCMI device to destroy
> causes an unbalance in device refcount, since device_find_child() calls an
> implicit get_device(): this, in turns, inhibits the call of the provided
> release methods upon devices destruction.
> 
> As a consequence, one of the structures that is not freed properly upon
> destruction is the internal struct device_private dev->p populated by the
> drivers subsystem core.
> 
> [...]

Applied to sudeep.holla/linux (for-next/scmi/fixes), thanks!

[1/1] firmware: arm_scmi: Balance device refcount when destroying devices
      https://git.kernel.org/sudeep.holla/c/9ca67840c0dd
--
Regards,
Sudeep