[PATCH 0/3] Fix error path for kobject_init_and_add()

[PATCH 0/3] Fix error path for kobject_init_and_add()

[Tobin C. Harding]

Hi Dave,

There are a few places in net/ that are not correctly handling the error
path after calls to kobject_init_and_add().  This set fixes all of these
for net/

This corrects a memory leak if kobject_init() is not followed by a call
to kobject_put()

This set is part of an effort to check/fix all of these mem leaks across
the kernel tree.

For reference this is the behaviour that we are trying to achieve

void fn(void)
{
	int ret;

	ret = kobject_init_and_add(kobj, ktype, NULL, "foo");
	if (ret) {
		kobject_put(kobj);
		return ret;
	}

	ret = some_init_fn();
	if (ret)
		goto err;

	ret = some_other_init_fn();
	if (ret)
		goto other_err;

	kobject_uevent(kobj, KOBJ_ADD);
	return 0;

other_err:
	other_clean_up_fn();
err:
	kobject_del(kobj);
	return ret;
}


Testing: No testing done, built with config options

CONFIG_NET=y
CONFIG_SYSFS=y
CONFIG_BRIDGE=y


thanks,
Tobin.

Tobin C. Harding (3):
  bridge: Fix error path for kobject_init_and_add()
  bridge: Use correct cleanup function
  net-sysfs: Fix error path for kobject_init_and_add()

 net/bridge/br_if.c   | 6 ++++--
 net/core/net-sysfs.c | 8 ++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH 1/3] bridge: Fix error path for kobject_init_and_add()

[Tobin C. Harding]

Currently error return from kobject_init_and_add() is not followed by a
call to kobject_put().  This means there is a memory leak.

Add call to kobject_put() in error path of kobject_init_and_add().

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 net/bridge/br_if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 41f0a696a65f..e5c8c9941c51 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -607,8 +607,10 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 
 	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
 				   SYSFS_BRIDGE_PORT_ATTR);
-	if (err)
+	if (err) {
+		kobject_put(&p->kobj);
 		goto err1;
+	}
 
 	err = br_sysfs_addif(p);
 	if (err)
-- 
2.21.0

[PATCH 2/3] bridge: Use correct cleanup function

[Tobin C. Harding]

The correct cleanup function if a call to kobject_init_and_add() has
returned _successfully_ is kobject_del().  kobject_put() is used if the
call to kobject_init_and_add() fails.  kobject_del() calls kobject_put().

Use correct cleanup function in error path.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 net/bridge/br_if.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index e5c8c9941c51..d3a1554ccff4 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -701,7 +701,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 err3:
 	sysfs_remove_link(br->ifobj, p->dev->name);
 err2:
-	kobject_put(&p->kobj);
+	kobject_del(&p->kobj);
 	p = NULL; /* kobject_put frees */
 err1:
 	dev_set_allmulti(dev, -1);
-- 
2.21.0

[PATCH 3/3] net-sysfs: Fix error path for kobject_init_and_add()

[Tobin C. Harding]

Currently error return from kobject_init_and_add() is not followed by a
call to kobject_put().  This means there is a memory leak.

Add call to kobject_put() in error path of kobject_init_and_add().

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 net/core/net-sysfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 8f8b7b6c2945..9d4e3f47b789 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -925,8 +925,10 @@ static int rx_queue_add_kobject(struct net_device *dev, int index)
 	kobj->kset = dev->queues_kset;
 	error = kobject_init_and_add(kobj, &rx_queue_ktype, NULL,
 				     "rx-%u", index);
-	if (error)
+	if (error) {
+		kobject_put(kobj);
 		return error;
+	}
 
 	dev_hold(queue->dev);
 
@@ -1462,8 +1464,10 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index)
 	kobj->kset = dev->queues_kset;
 	error = kobject_init_and_add(kobj, &netdev_queue_ktype, NULL,
 				     "tx-%u", index);
-	if (error)
+	if (error) {
+		kobject_put(kobj);
 		return error;
+	}
 
 	dev_hold(queue->dev);
 
-- 
2.21.0

Re: [PATCH 1/3] bridge: Fix error path for kobject_init_and_add()

[Tobin C. Harding]

On Tue, Apr 30, 2019 at 10:28:15AM +1000, Tobin C. Harding wrote:

[snip]

The cover letter appears to have gotten lost, I can resend if it makes
things better for you Dave.

FTR this is the contents from it:

Hi Dave,

There are a few places in net/ that are not correctly handling the error
path after calls to kobject_init_and_add().  This set fixes all of these
for net/

This corrects a memory leak if kobject_init() is not followed by a call
to kobject_put()

This set is part of an effort to check/fix all of these mem leaks across
the kernel tree.

For reference this is the behaviour that we are trying to achieve

void fn(void)
{
	int ret;

	ret = kobject_init_and_add(kobj, ktype, NULL, "foo");
	if (ret) {
		kobject_put(kobj);
		return ret;
	}

	ret = some_init_fn();
	if (ret)
		goto err;

	ret = some_other_init_fn();
	if (ret)
		goto other_err;

	kobject_uevent(kobj, KOBJ_ADD);
	return 0;

other_err:
	other_clean_up_fn();
err:
	kobject_del(kobj);
	return ret;
}


 Testing: No testing done, built with config options

CONFIG_NET=y
CONFIG_SYSFS=y
CONFIG_BRIDGE=y


thanks,
Tobin.

Re: [PATCH 1/3] bridge: Fix error path for kobject_init_and_add()

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 10:28:15AM +1000, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/3] bridge: Use correct cleanup function

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 10:28:16AM +1000, Tobin C. Harding wrote:
> The correct cleanup function if a call to kobject_init_and_add() has
> returned _successfully_ is kobject_del().  kobject_put() is used if the
> call to kobject_init_and_add() fails.  kobject_del() calls kobject_put().
> 
> Use correct cleanup function in error path.
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 3/3] net-sysfs: Fix error path for kobject_init_and_add()

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 10:28:17AM +1000, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/3] bridge: Fix error path for kobject_init_and_add()

[Tyler Hicks]

On 2019-04-30 10:28:15, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  net/bridge/br_if.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 41f0a696a65f..e5c8c9941c51 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -607,8 +607,10 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
>  
>  	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
>  				   SYSFS_BRIDGE_PORT_ATTR);
> -	if (err)
> +	if (err) {
> +		kobject_put(&p->kobj);
>  		goto err1;
> +	}

I think this is duplicating the code under the err2 label and doing so
in a way that would introduce a double free.

If the refcount hits 0 in the kobject_put(), release_nbp() is called
which calls kfree() on the p pointer. However, the p pointer is never
set to NULL like what's done under the err2 label. Once we're back in
br_add_if(), kfree() is called on the p pointer again just before
returning.

I think it would be better if you just jumped to the err2 label instead
of err1. err1 will no longer be used so, unfortunately, you'll have to
refactor all the labels at the same time.

Tyler

>  
>  	err = br_sysfs_addif(p);
>  	if (err)
> -- 
> 2.21.0
> 

Re: [PATCH 1/3] bridge: Fix error path for kobject_init_and_add()

[Tyler Hicks]

On 2019-04-30 10:14:37, Tyler Hicks wrote:
> On 2019-04-30 10:28:15, Tobin C. Harding wrote:
> > Currently error return from kobject_init_and_add() is not followed by a
> > call to kobject_put().  This means there is a memory leak.
> > 
> > Add call to kobject_put() in error path of kobject_init_and_add().
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> >  net/bridge/br_if.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> > index 41f0a696a65f..e5c8c9941c51 100644
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -607,8 +607,10 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
> >  
> >  	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
> >  				   SYSFS_BRIDGE_PORT_ATTR);
> > -	if (err)
> > +	if (err) {
> > +		kobject_put(&p->kobj);
> >  		goto err1;
> > +	}
> 
> I think this is duplicating the code under the err2 label and doing so
> in a way that would introduce a double free.
> 
> If the refcount hits 0 in the kobject_put(), release_nbp() is called
> which calls kfree() on the p pointer. However, the p pointer is never
> set to NULL like what's done under the err2 label. Once we're back in
> br_add_if(), kfree() is called on the p pointer again just before
> returning.
> 
> I think it would be better if you just jumped to the err2 label instead
> of err1. err1 will no longer be used so, unfortunately, you'll have to
> refactor all the labels at the same time.

I noticed that the last paragraph is bad advice after looking at patch
#2 in this series. I guess you'd be better off calling kobject_put(),
setting p to NULL (the only thing that's missing), and jumping to err1
in this patch.

Tyler

> 
> Tyler
> 
> >  
> >  	err = br_sysfs_addif(p);
> >  	if (err)
> > -- 
> > 2.21.0
> > 

Re: [PATCH 3/3] net-sysfs: Fix error path for kobject_init_and_add()

[Tyler Hicks]

On 2019-04-30 10:28:17, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  net/core/net-sysfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 8f8b7b6c2945..9d4e3f47b789 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -925,8 +925,10 @@ static int rx_queue_add_kobject(struct net_device *dev, int index)
>  	kobj->kset = dev->queues_kset;
>  	error = kobject_init_and_add(kobj, &rx_queue_ktype, NULL,
>  				     "rx-%u", index);
> -	if (error)
> +	if (error) {
> +		kobject_put(kobj);
>  		return error;
> +	}

The commit message of the second patch in this series states, "The
correct cleanup function if a call to kobject_init_and_add() has
returned _successfully_ is kobject_del()." Doesn't that mean that
kobject_del() needs to be called instead of kobject_put() when
sysfs_create_group() fails a little lower in this function?

>  
>  	dev_hold(queue->dev);
>  
> @@ -1462,8 +1464,10 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index)
>  	kobj->kset = dev->queues_kset;
>  	error = kobject_init_and_add(kobj, &netdev_queue_ktype, NULL,
>  				     "tx-%u", index);
> -	if (error)
> +	if (error) {
> +		kobject_put(kobj);
>  		return error;
> +	}
>  
>  	dev_hold(queue->dev);

I think the same s/kobject_put/kobject_del/ substitution may be needed
in this function when sysfs_create_group() fails.

Tyler

>  
> -- 
> 2.21.0
> 

Re: [PATCH 3/3] net-sysfs: Fix error path for kobject_init_and_add()

[Andy Shevchenko]

On Tue, Apr 30, 2019 at 10:28:17AM +1000, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().

It's not obvious to me if this will help to fix what is stated in the
(reverted) commit 6b70fc94afd1 ("net-sysfs: Fix memory leak in
netdev_register_kobject")?

If so, perhaps we need to tell syzkaller guys about this.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] bridge: Fix error path for kobject_init_and_add()

[Tobin C. Harding]

On Tue, Apr 30, 2019 at 10:28:15AM +1000, Tobin C. Harding wrote:

[snip]

Please do not consider this series for merge.  There is a bit of
confusion here.

There are a few of theses patches live on various LKML lists.  Have to
consolidate all the knowledge.  When I _actually_ know how to use
kobject correctly I'll re-spin.

Thanks for your patience.

	Tobin

Re: [PATCH 3/3] net-sysfs: Fix error path for kobject_init_and_add()

[wanghai (M)]

在 2019/5/1 0:11, Andy Shevchenko 写道:
> On Tue, Apr 30, 2019 at 10:28:17AM +1000, Tobin C. Harding wrote:
>> Currently error return from kobject_init_and_add() is not followed by a
>> call to kobject_put().  This means there is a memory leak.
>>
>> Add call to kobject_put() in error path of kobject_init_and_add().
> It's not obvious to me if this will help to fix what is stated in the
> (reverted) commit 6b70fc94afd1 ("net-sysfs: Fix memory leak in
> netdev_register_kobject")?
>
> If so, perhaps we need to tell syzkaller guys about this.
Thanks for reminding. It seems that the bug has not been completely fixed.

in netdev_register_kobject():

1746         error = device_add(dev);
1747         if (error)
1748                 return error;
1749
1750         error = register_queue_kobjects(ndev);
1751         if (error) {
1752                 device_del(dev);
1753                 return error;
1754         }

This only fixes a memory leak after a failure in 
register_queue_kobjects(). If device_add() fails, kobject_put() still 
cann't be called, and the memory leak still exists.