linux-kernel - Re: [PATCH] net-sysfs: Fix memory leak in netdev_register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Tue, 19 Mar 2019 16:00:19 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     "wanghai (M)" <wanghai26@...wei.com>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        davem@...emloft.net, idosch@...lanox.com,
        alexander.h.duyck@...el.com, tyhicks@...onical.com,
        f.fainelli@...il.com, amritha.nambiar@...el.com, joe@...ches.com,
        dmitry.torokhov@...il.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net-sysfs: Fix memory leak in netdev_register_kobject

On Tue, Mar 19, 2019 at 08:17:01PM +0800, wanghai (M) wrote:
> 在 2019/3/19 18:30, Andy Shevchenko 写道:
> > On Tue, Mar 19, 2019 at 11:19:24AM +0800, wanghai (M) wrote:
> > > 在 2019/3/19 0:19, Andy Shevchenko 写道:

> > > If device_add(dev) or register_queue_kobjects(ndev) fails,
> > > In register_netdevice(), dev-> reg_state = NETREG_UNINITIALIZED and returns
> > > an error, causing put_device(&dev-> dev) -> ..-> kobject_cleanup() not to be
> > > called.
> > OK, that's true, but your patch is wrong.
> > See error handling in device_create_groups_vargs() for example.

> Hi Andy
> Thanks for your advice. I understand the problem of my patch.
> Can you help me see if it can be fixed like this?
> 
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 4ff661f..6fe5b8e 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -1745,17 +1745,21 @@ int netdev_register_kobject(struct net_device *ndev)
> 
>         error = device_add(dev);
>         if (error)
> -               return error;
> +               goto device_add_error;

This part seems correct now.

>         error = register_queue_kobjects(ndev);
> -       if (error) {
> -               device_del(dev);
> -               return error;
> -       }
> +       if (error)
> +               goto register_error;

Yes, seems correct order here, i.e. device_del() followed by put_device().

> 
>         pm_runtime_set_memalloc_noio(dev, true);
> 

> +out:

Better

	return 0;

>         return error;
> +register_error:

Better to describe what you will do here, i.e.

error_device_del:

> +       device_del(dev);

> +device_add_error:

Ditto.

error_put_device:

> +       put_device(dev);

> +       goto out;

Better

	return error;

>  }

-- 
With Best Regards,
Andy Shevchenko