[PATCH net] net: atm: fix memory leak in atm_register_sysfs when device_register fail

[PATCH net] net: atm: fix memory leak in atm_register_sysfs when device_register fail

[Wang Liang]

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.1

Re: [PATCH net] net: atm: fix memory leak in atm_register_sysfs when device_register fail

[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;
 }
 


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

Re: [PATCH net] net: atm: fix memory leak in atm_register_sysfs when device_register fail

[Wang Liang]

在 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 {
>
> ...
>

Re: [PATCH net] net: atm: fix memory leak in atm_register_sysfs when device_register fail

[Simon Horman]

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

Re: [PATCH net] net: atm: fix memory leak in atm_register_sysfs when device_register fail

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 1 Sep 2025 14:35:37 +0800 you 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
> 
> [...]

Here is the summary with links:
  - [net] net: atm: fix memory leak in atm_register_sysfs when device_register fail
    https://git.kernel.org/netdev/net/c/0a228624bcc0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html