Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[Andy Shevchenko]

On Fri, Apr 12, 2019 at 04:36:34PM -0400, Wang Hai wrote:

> +error_register:
> +	device_del(dev);
> +error_device_add:
> +	kfree_const(dev->kobj.name);
>  	return error;

When put_device() will be called on this it will go to double free (in case of
dynamically allocated dev->kobj.name.

Al Viro and me suggested earlier that the correct fix is to call put_device()
in a places where it is appropriate.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] Revert "net-sysfs: Fix memory leak in netdev_register_kobject"

[Andy Shevchenko]

On Fri, Apr 12, 2019 at 04:36:33PM -0400, Wang Hai wrote:
> This reverts commit 6b70fc94afd165342876e53fc4b2f7d085009945.
> 
> The reverted bugfix will cause another issue.
> Reported by syzbot+6024817a931b2830bc93@syzkaller.appspotmail.com.
> See https://syzkaller.appspot.com/x/log.txt?x=1737671b200000 for
> details.
> 

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Wang Hai <wanghai26@huawei.com>
> ---
>  net/core/net-sysfs.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index f8f9430..8f8b7b6 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -1747,20 +1747,16 @@ int netdev_register_kobject(struct net_device *ndev)
>  
>  	error = device_add(dev);
>  	if (error)
> -		goto error_put_device;
> +		return error;
>  
>  	error = register_queue_kobjects(ndev);
> -	if (error)
> -		goto error_device_del;
> +	if (error) {
> +		device_del(dev);
> +		return error;
> +	}
>  
>  	pm_runtime_set_memalloc_noio(dev, true);
>  
> -	return 0;
> -
> -error_device_del:
> -	device_del(dev);
> -error_put_device:
> -	put_device(dev);
>  	return error;
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[wanghai (M)]

在 2019/4/12 16:38, Andy Shevchenko 写道:
> On Fri, Apr 12, 2019 at 04:36:34PM -0400, Wang Hai wrote:
>
>> +error_register:
>> +	device_del(dev);
>> +error_device_add:
>> +	kfree_const(dev->kobj.name);
>>   	return error;
> When put_device() will be called on this it will go to double free (in case of
> dynamically allocated dev->kobj.name.
>
> Al Viro and me suggested earlier that the correct fix is to call put_device()
> in a places where it is appropriate.
Thanks. I'll take a closer look at the code to see when it's time to 
call put_device(). It's really not easy to fix.

Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[Al Viro]

On Fri, Apr 12, 2019 at 08:08:32PM +0800, wanghai (M) wrote:
> 
> 在 2019/4/12 16:38, Andy Shevchenko 写道:
> > On Fri, Apr 12, 2019 at 04:36:34PM -0400, Wang Hai wrote:
> > 
> > > +error_register:
> > > +	device_del(dev);
> > > +error_device_add:
> > > +	kfree_const(dev->kobj.name);
> > >   	return error;
> > When put_device() will be called on this it will go to double free (in case of
> > dynamically allocated dev->kobj.name.
> > 
> > Al Viro and me suggested earlier that the correct fix is to call put_device()
> > in a places where it is appropriate.
> Thanks. I'll take a closer look at the code to see when it's time to call
> put_device(). It's really not easy to fix.

Depends upon the driver, obviously...  Note that if it's a built-in
with device never destroyed, that allocation is no leak at all -
reference to the object remains around.

Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[Eric Dumazet]

On 04/12/2019 01:36 PM, Wang Hai wrote:
> When registering struct net_device, it will call
>         register_netdevice ->
>                 netdev_register_kobject ->
>                         device_initialize(dev);
>                         dev_set_name(dev, "%s", ndev->name)
>                         device_add(dev)
>                         register_queue_kobjects(ndev)
> 
> In netdev_register_kobject(), if device_add(dev) or
> register_queue_kobjects(ndev) failed. Register_netdevice()
> will return error, causing netdev_freemem(ndev) to be
> called to free net_device, however put_device(&dev->dev)->..->
> kobject_cleanup() won't be called, resulting in a memory leak
> which is alloced by dev_set_name()
> 
>


Having two patches with exact same title is rather confusing for bug trackers.

Instead of revert + another_patch, why not just send a cumulative fix ?

Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[Andy Shevchenko]

On Fri, Apr 12, 2019 at 06:03:27AM -0700, Eric Dumazet wrote:
> On 04/12/2019 01:36 PM, Wang Hai wrote:
> > When registering struct net_device, it will call
> >         register_netdevice ->
> >                 netdev_register_kobject ->
> >                         device_initialize(dev);
> >                         dev_set_name(dev, "%s", ndev->name)
> >                         device_add(dev)
> >                         register_queue_kobjects(ndev)
> > 
> > In netdev_register_kobject(), if device_add(dev) or
> > register_queue_kobjects(ndev) failed. Register_netdevice()
> > will return error, causing netdev_freemem(ndev) to be
> > called to free net_device, however put_device(&dev->dev)->..->
> > kobject_cleanup() won't be called, resulting in a memory leak
> > which is alloced by dev_set_name()
> > 
> >
> 
> 
> Having two patches with exact same title is rather confusing for bug trackers.
> 
> Instead of revert + another_patch, why not just send a cumulative fix ?

The second patch is a reincarnation of the first version of the fix which has
been discussed as not a correct approach. But revert should be applied sooner
as the original commit brought a regression.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[wanghai (M)]

在 2019/4/12 21:03, Eric Dumazet 写道:
>
> On 04/12/2019 01:36 PM, Wang Hai wrote:
>> When registering struct net_device, it will call
>>          register_netdevice ->
>>                  netdev_register_kobject ->
>>                          device_initialize(dev);
>>                          dev_set_name(dev, "%s", ndev->name)
>>                          device_add(dev)
>>                          register_queue_kobjects(ndev)
>>
>> In netdev_register_kobject(), if device_add(dev) or
>> register_queue_kobjects(ndev) failed. Register_netdevice()
>> will return error, causing netdev_freemem(ndev) to be
>> called to free net_device, however put_device(&dev->dev)->..->
>> kobject_cleanup() won't be called, resulting in a memory leak
>> which is alloced by dev_set_name()
>>
>>
>
> Having two patches with exact same title is rather confusing for bug trackers.
>
> Instead of revert + another_patch, why not just send a cumulative fix ?
>
Thanks, if I fix it, I will send a cumulative fix again

Re: [PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[wanghai (M)]

在 2019/4/12 21:20, Andy Shevchenko 写道:
> On Fri, Apr 12, 2019 at 06:03:27AM -0700, Eric Dumazet wrote:
>> On 04/12/2019 01:36 PM, Wang Hai wrote:
>>> When registering struct net_device, it will call
>>>          register_netdevice ->
>>>                  netdev_register_kobject ->
>>>                          device_initialize(dev);
>>>                          dev_set_name(dev, "%s", ndev->name)
>>>                          device_add(dev)
>>>                          register_queue_kobjects(ndev)
>>>
>>> In netdev_register_kobject(), if device_add(dev) or
>>> register_queue_kobjects(ndev) failed. Register_netdevice()
>>> will return error, causing netdev_freemem(ndev) to be
>>> called to free net_device, however put_device(&dev->dev)->..->
>>> kobject_cleanup() won't be called, resulting in a memory leak
>>> which is alloced by dev_set_name()
>>>
>>>
>>
>> Having two patches with exact same title is rather confusing for bug trackers.
>>
>> Instead of revert + another_patch, why not just send a cumulative fix ?
> The second patch is a reincarnation of the first version of the fix which has
> been discussed as not a correct approach. But revert should be applied sooner
> as the original commit brought a regression.

Thanks for letting me know. Now should I resend a separate revert, or do 
you just review the first patch?

[PATCH 0/2] net-sysfs: revert wrong bugfix and re-fix

[Wang Hai]

First patch reverts the bugfix which cause another issue.
Reported by syzbot+6024817a931b2830bc93@syzkaller.appspotmail.com. 
See https://syzkaller.appspot.com/x/log.txt?x=1737671b200000 for details
Second patch improves the first(reverted) patch.
Applied these two patches. I tested, the memory leak issue
wasn't reproduced, and it didn't produce the bug which
reported by syzkaller.

Wang Hai (2):
  Revert "net-sysfs: Fix memory leak in netdev_register_kobject"
  net-sysfs: Fix memory leak in netdev_register_kobject

 net/core/net-sysfs.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] Revert "net-sysfs: Fix memory leak in netdev_register_kobject"

[Wang Hai]

This reverts commit 6b70fc94afd165342876e53fc4b2f7d085009945.

The reverted bugfix will cause another issue.
Reported by syzbot+6024817a931b2830bc93@syzkaller.appspotmail.com.
See https://syzkaller.appspot.com/x/log.txt?x=1737671b200000 for
details.

Signed-off-by: Wang Hai <wanghai26@huawei.com>
---
 net/core/net-sysfs.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index f8f9430..8f8b7b6 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1747,20 +1747,16 @@ int netdev_register_kobject(struct net_device *ndev)
 
 	error = device_add(dev);
 	if (error)
-		goto error_put_device;
+		return error;
 
 	error = register_queue_kobjects(ndev);
-	if (error)
-		goto error_device_del;
+	if (error) {
+		device_del(dev);
+		return error;
+	}
 
 	pm_runtime_set_memalloc_noio(dev, true);
 
-	return 0;
-
-error_device_del:
-	device_del(dev);
-error_put_device:
-	put_device(dev);
 	return error;
 }
 
-- 
1.8.3.1

[PATCH 2/2] net-sysfs: Fix memory leak in netdev_register_kobject

[Wang Hai]

When registering struct net_device, it will call
        register_netdevice ->
                netdev_register_kobject ->
                        device_initialize(dev);
                        dev_set_name(dev, "%s", ndev->name)
                        device_add(dev)
                        register_queue_kobjects(ndev)

In netdev_register_kobject(), if device_add(dev) or
register_queue_kobjects(ndev) failed. Register_netdevice()
will return error, causing netdev_freemem(ndev) to be
called to free net_device, however put_device(&dev->dev)->..->
kobject_cleanup() won't be called, resulting in a memory leak
which is alloced by dev_set_name()

syzkaller report this:
BUG: memory leak
unreferenced object 0xffff8881f4fad168 (size 8):
comm "syz-executor.0", pid 3575, jiffies 4294778002 (age 20.134s)
hex dump (first 8 bytes):
  77 70 61 6e 30 00 ff ff                          wpan0...
backtrace:
  [<000000006d2d91d7>] kstrdup_const+0x3d/0x50 mm/util.c:73
  [<00000000ba9ff953>] kvasprintf_const+0x112/0x170 lib/kasprintf.c:48
  [<000000005555ec09>] kobject_set_name_vargs+0x55/0x130 lib/kobject.c:281
  [<0000000098d28ec3>] dev_set_name+0xbb/0xf0 drivers/base/core.c:1915
  [<00000000b7553017>] netdev_register_kobject+0xc0/0x410 net/core/net-sysfs.c:1727
  [<00000000c826a797>] register_netdevice+0xa51/0xeb0 net/core/dev.c:8711
  [<00000000857bfcfd>] cfg802154_update_iface_num.isra.2+0x13/0x90 [ieee802154]
  [<000000003126e453>] ieee802154_llsec_fill_key_id+0x1d5/0x570 [ieee802154]
  [<00000000e4b3df51>] 0xffffffffc1500e0e
  [<00000000b4319776>] platform_drv_probe+0xc6/0x180 drivers/base/platform.c:614
  [<0000000037669347>] really_probe+0x491/0x7c0 drivers/base/dd.c:509
  [<000000008fed8862>] driver_probe_device+0xdc/0x240 drivers/base/dd.c:671
  [<00000000baf52041>] device_driver_attach+0xf2/0x130 drivers/base/dd.c:945
  [<00000000c7cc8dec>] __driver_attach+0x10e/0x210 drivers/base/dd.c:1022
  [<0000000057a757c2>] bus_for_each_dev+0x154/0x1e0 drivers/base/bus.c:304
  [<000000005f5ae04b>] bus_add_driver+0x427/0x5e0 drivers/base/bus.c:645

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
Signed-off-by: Wang Hai <wanghai26@huawei.com>
---
 net/core/net-sysfs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 8f8b7b6..095cdce 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1747,16 +1747,20 @@ int netdev_register_kobject(struct net_device *ndev)
 
 	error = device_add(dev);
 	if (error)
-		return error;
+		goto error_device_add;
 
 	error = register_queue_kobjects(ndev);
-	if (error) {
-		device_del(dev);
-		return error;
-	}
+	if (error)
+		goto error_register;
 
 	pm_runtime_set_memalloc_noio(dev, true);
 
+	return 0;
+
+error_register:
+	device_del(dev);
+error_device_add:
+	kfree_const(dev->kobj.name);
 	return error;
 }
 
-- 
1.8.3.1

Re: [PATCH 1/2] Revert "net-sysfs: Fix memory leak in netdev_register_kobject"

[David Miller]

From: Wang Hai <wanghai26@huawei.com>
Date: Fri, 12 Apr 2019 16:36:33 -0400

> This reverts commit 6b70fc94afd165342876e53fc4b2f7d085009945.
> 
> The reverted bugfix will cause another issue.
> Reported by syzbot+6024817a931b2830bc93@syzkaller.appspotmail.com.
> See https://syzkaller.appspot.com/x/log.txt?x=1737671b200000 for
> details.
> 
> Signed-off-by: Wang Hai <wanghai26@huawei.com>

I'll just apply this by itself.

Thanks Wang.