net/atm/resources.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
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@huawei.com>
---
net/atm/resources.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/atm/resources.c b/net/atm/resources.c
index b19d851e1f44..7c6fdedbcf4e 100644
--- a/net/atm/resources.c
+++ b/net/atm/resources.c
@@ -112,7 +112,9 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
if (atm_proc_dev_register(dev) < 0) {
pr_err("atm_proc_dev_register failed for dev %s\n", type);
- goto out_fail;
+ mutex_unlock(&atm_dev_mutex);
+ kfree(dev);
+ return NULL;
}
if (atm_register_sysfs(dev, parent) < 0) {
@@ -128,7 +130,7 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
return dev;
out_fail:
- kfree(dev);
+ put_device(&dev->class_dev);
dev = NULL;
goto out;
}
--
2.34.1On 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@huawei.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;
}
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().
If so, I wonder if this can be resolved (in a separate patch to
the fix for atm_register_sysfs()) like this.
(Also compile tested only!)
diff --git a/net/atm/resources.c b/net/atm/resources.c
index b19d851e1f44..3002ff5b60f8 100644
--- a/net/atm/resources.c
+++ b/net/atm/resources.c
@@ -112,13 +110,12 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
if (atm_proc_dev_register(dev) < 0) {
pr_err("atm_proc_dev_register failed for dev %s\n", type);
- goto out_fail;
+ goto err_free_dev;
}
if (atm_register_sysfs(dev, parent) < 0) {
pr_err("atm_register_sysfs failed for dev %s\n", type);
- atm_proc_dev_deregister(dev);
- goto out_fail;
+ goto err_proc_dev_unregister;
}
list_add_tail(&dev->dev_list, &atm_devs);
@@ -127,7 +124,9 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
mutex_unlock(&atm_dev_mutex);
return dev;
-out_fail:
+err_proc_dev_unregister:
+ atm_proc_dev_deregister(dev);
+err_free_dev:
kfree(dev);
dev = NULL;
goto out;
Lastly, while not a bug and not material for net, it would be nice to
follow-up on the above and consolidate the error handling in
atm_dev_register().
Something like this (compile tested only!):
diff --git a/net/atm/resources.c b/net/atm/resources.c
index b19d851e1f44..3002ff5b60f8 100644
--- a/net/atm/resources.c
+++ b/net/atm/resources.c
@@ -89,9 +89,7 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
inuse = __atm_dev_lookup(number);
if (inuse) {
atm_dev_put(inuse);
- mutex_unlock(&atm_dev_mutex);
- kfree(dev);
- return NULL;
+ goto err_free_dev;
}
dev->number = number;
} else {
...
--
pw-bot: changes-requested
在 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@huawei.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.
>
>
> 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?
------
Best regards
Wang Liang
> If so, I wonder if this can be resolved (in a separate patch to
> the fix for atm_register_sysfs()) like this.
> (Also compile tested only!)
>
> diff --git a/net/atm/resources.c b/net/atm/resources.c
> index b19d851e1f44..3002ff5b60f8 100644
> --- a/net/atm/resources.c
> +++ b/net/atm/resources.c
> @@ -112,13 +110,12 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
>
> if (atm_proc_dev_register(dev) < 0) {
> pr_err("atm_proc_dev_register failed for dev %s\n", type);
> - goto out_fail;
> + goto err_free_dev;
> }
>
> if (atm_register_sysfs(dev, parent) < 0) {
> pr_err("atm_register_sysfs failed for dev %s\n", type);
> - atm_proc_dev_deregister(dev);
> - goto out_fail;
> + goto err_proc_dev_unregister;
> }
>
> list_add_tail(&dev->dev_list, &atm_devs);
> @@ -127,7 +124,9 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
> mutex_unlock(&atm_dev_mutex);
> return dev;
>
> -out_fail:
> +err_proc_dev_unregister:
> + atm_proc_dev_deregister(dev);
> +err_free_dev:
> kfree(dev);
> dev = NULL;
> goto out;
>
> Lastly, while not a bug and not material for net, it would be nice to
> follow-up on the above and consolidate the error handling in
> atm_dev_register().
>
> Something like this (compile tested only!):
>
> diff --git a/net/atm/resources.c b/net/atm/resources.c
> index b19d851e1f44..3002ff5b60f8 100644
> --- a/net/atm/resources.c
> +++ b/net/atm/resources.c
> @@ -89,9 +89,7 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
> inuse = __atm_dev_lookup(number);
> if (inuse) {
> atm_dev_put(inuse);
> - mutex_unlock(&atm_dev_mutex);
> - kfree(dev);
> - return NULL;
> + goto err_free_dev;
> }
> dev->number = number;
> } else {
>
> ...
>
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@huawei.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@kernel.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
© 2016 - 2025 Red Hat, Inc.