[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250902100716.GB15473@horms.kernel.org>
Date: Tue, 2 Sep 2025 11:07:16 +0100
From: Simon Horman <horms@...nel.org>
To: Wang Liang <wangliang74@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, kuniyu@...gle.com, kay.sievers@...y.org,
gregkh@...e.de, yuehaibing@...wei.com, zhangchangzhong@...wei.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: atm: fix memory leak in atm_register_sysfs when
device_register fail
On Tue, Sep 02, 2025 at 09:41:33AM +0800, Wang Liang wrote:
>
> 在 2025/9/2 3:01, Simon Horman 写道:
> > On Mon, Sep 01, 2025 at 02:35:37PM +0800, Wang Liang wrote:
> > > When device_register() return error in atm_register_sysfs(), which can be
> > > triggered by kzalloc fail in device_private_init() or other reasons,
> > > kmemleak reports the following memory leaks:
> > >
> > > unreferenced object 0xffff88810182fb80 (size 8):
> > > comm "insmod", pid 504, jiffies 4294852464
> > > hex dump (first 8 bytes):
> > > 61 64 75 6d 6d 79 30 00 adummy0.
> > > backtrace (crc 14dfadaf):
> > > __kmalloc_node_track_caller_noprof+0x335/0x450
> > > kvasprintf+0xb3/0x130
> > > kobject_set_name_vargs+0x45/0x120
> > > dev_set_name+0xa9/0xe0
> > > atm_register_sysfs+0xf3/0x220
> > > atm_dev_register+0x40b/0x780
> > > 0xffffffffa000b089
> > > do_one_initcall+0x89/0x300
> > > do_init_module+0x27b/0x7d0
> > > load_module+0x54cd/0x5ff0
> > > init_module_from_file+0xe4/0x150
> > > idempotent_init_module+0x32c/0x610
> > > __x64_sys_finit_module+0xbd/0x120
> > > do_syscall_64+0xa8/0x270
> > > entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > >
> > > When device_create_file() return error in atm_register_sysfs(), the same
> > > issue also can be triggered.
> > >
> > > Function put_device() should be called to release kobj->name memory and
> > > other device resource, instead of kfree().
> > >
> > > Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
> > > Signed-off-by: Wang Liang <wangliang74@...wei.com>
> > Thanks Wang Liang,
> >
> > I agree this is a bug.
> >
> > I think that the guiding principle should be that on error functions
> > unwind any resource allocations they have made, rather than leaving
> > it up to callers to clean things up.
> >
> > So, as the problem you describe seems to be due to atm_register_sysfs()
> > leaking resources if it encounters an error, I think the problem would
> > best be resolved there.
> >
> > Perhaps something like this.
> > (Compile tested only!)
> >
> > diff --git a/net/atm/atm_sysfs.c b/net/atm/atm_sysfs.c
> > index 54e7fb1a4ee5..62f3d520a80a 100644
> > --- a/net/atm/atm_sysfs.c
> > +++ b/net/atm/atm_sysfs.c
> > @@ -148,20 +148,23 @@ int atm_register_sysfs(struct atm_dev *adev, struct device *parent)
> > dev_set_name(cdev, "%s%d", adev->type, adev->number);
> > err = device_register(cdev);
> > if (err < 0)
> > - return err;
> > + goto err_put_dev;
> > for (i = 0; atm_attrs[i]; i++) {
> > err = device_create_file(cdev, atm_attrs[i]);
> > if (err)
> > - goto err_out;
> > + goto err_remove_file;
> > }
> > return 0;
> > -err_out:
> > +err_remove_file:
> > for (j = 0; j < i; j++)
> > device_remove_file(cdev, atm_attrs[j]);
> > device_del(cdev);
> > +err_put_dev:
> > + put_device(cdev);
> > +
> > return err;
> > }
>
>
> Thanks for your replies, it is very clear!
>
> But the above code may introduce a use-after-free issue. If
> device_register()
> fails, put_device() call atm_release() to free atm_dev, and
> atm_proc_dev_deregister() will visit it.
>
> And kfree() should be removed in atm_dev_register() to avoid double-free.
Thanks, I see that now.
I do think that it would be nice to untangle the error handling here.
But, as a but fix I now think your original approach is good.
Because it addresses the issue in a minimal way.
Reviewed-by: Simon Horman <horms@...nel.org>
> > Looking over atm_dev_register, it seems to me that it will deadlock
> > if it calls atm_proc_dev_deregister() if atm_register_sysfs() fails.
> > This is because atm_dev_register() is holding atm_dev_mutex,
> > and atm_proc_dev_deregister() tries to take atm_dev_mutex().
>
>
> I cannot find somewhere tries to take atm_dev_mutex(), can you give some
> hints?
Sorry, my mistake. I was looking at atm_dev_deregister()
rather than atm_proc_dev_deregister().
...
--
pw-bot: under-review
Powered by blists - more mailing lists