[PATCH 0/2] Fix kobject error path memleaks

[PATCH 0/2] Fix kobject error path memleaks

[Tobin C. Harding]

Hi,

Is it ok to send patches during the merge window?  Applies on top of
Linus' mainline tag: v5.1, happy to rebase if there are conflicts.

While auditing kobject_init_and_add() calls throughout the kernel it was
found that btrfs potentially has a couple of memleaks in the error path
code for kobject_init_and_add().

Failing calls to kobject_init_and_add() should be followed by a call to
kobject_put() since kobject_init_and_add() always calls kobject_init().

Of note, adding kobject_put() causes the release method to be called if
kobject_init_and_add() fails.  For patch #1 this means we don't have to
manually free the space_info or call percpu_counter_destroy() since
these are both done by the release method.  In the second patch, I
believe the added call to kobject_put() fits in with the fs_devices
lifecycle assumptions of open_ctree() but please could you review since
I am new to this code.

CC'ing the kobject maintainers/reviewers also.

Thanks,
Tobin.


Tobin C. Harding (2):
  fs: btrfs: Fix error path kobject memory leak
  fs: btrfs: Don't leak memory when failing add fsid

 fs/btrfs/extent-tree.c | 3 +--
 fs/btrfs/sysfs.c       | 7 ++++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.21.0

[PATCH 1/2] fs: btrfs: Fix error path kobject memory leak

[Tobin C. Harding]

If a call to kobject_init_and_add() fails we must call kobject_put()
otherwise we leak memory.

Calling kobject_put() when kobject_init_and_add() fails drops the
refcount back to 0 and calls the ktype release method.

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

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 fs/btrfs/extent-tree.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index c5880329ae37..5e40c8f1e97a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3981,8 +3981,7 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
 				    info->space_info_kobj, "%s",
 				    alloc_name(space_info->flags));
 	if (ret) {
-		percpu_counter_destroy(&space_info->total_bytes_pinned);
-		kfree(space_info);
+		kobject_put(&space_info->kobj);
 		return ret;
 	}
 
-- 
2.21.0

Re: [PATCH 1/2] fs: btrfs: Fix error path kobject memory leak

[Nikolay Borisov]

On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> If a call to kobject_init_and_add() fails we must call kobject_put()
> otherwise we leak memory.
> 
> Calling kobject_put() when kobject_init_and_add() fails drops the
> refcount back to 0 and calls the ktype release method.
> 
> Add call to kobject_put() in the error path of call to
> kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  fs/btrfs/extent-tree.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index c5880329ae37..5e40c8f1e97a 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3981,8 +3981,7 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>  				    info->space_info_kobj, "%s",
>  				    alloc_name(space_info->flags));
>  	if (ret) {
> -		percpu_counter_destroy(&space_info->total_bytes_pinned);
> -		kfree(space_info);
> +		kobject_put(&space_info->kobj);

If you are only fixing kobject-related code then why do you delete
correct code as well? percpu_counter_Destroy is needed to dispose of the
percpu state which might have been allocated in percpu_counter_init
based on whether CONFIG_SMP is enabled or not? Also, the call to kfree
is required.

>  		return ret;
>  	}
>  
> 

Re: [PATCH 1/2] fs: btrfs: Fix error path kobject memory leak

[Greg Kroah-Hartman]

On Mon, May 13, 2019 at 08:59:56AM +0300, Nikolay Borisov wrote:
> 
> 
> On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> > If a call to kobject_init_and_add() fails we must call kobject_put()
> > otherwise we leak memory.
> > 
> > Calling kobject_put() when kobject_init_and_add() fails drops the
> > refcount back to 0 and calls the ktype release method.
> > 
> > Add call to kobject_put() in the error path of call to
> > kobject_init_and_add().
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> >  fs/btrfs/extent-tree.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index c5880329ae37..5e40c8f1e97a 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -3981,8 +3981,7 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
> >  				    info->space_info_kobj, "%s",
> >  				    alloc_name(space_info->flags));
> >  	if (ret) {
> > -		percpu_counter_destroy(&space_info->total_bytes_pinned);
> > -		kfree(space_info);
> > +		kobject_put(&space_info->kobj);
> 
> If you are only fixing kobject-related code then why do you delete
> correct code as well? percpu_counter_Destroy is needed to dispose of the
> percpu state which might have been allocated in percpu_counter_init
> based on whether CONFIG_SMP is enabled or not? Also, the call to kfree
> is required.

Both of those will happen in space_info_release() when the kobject is
properly disposed of with this last put to the kobject reference.

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

[PATCH 2/2] fs: btrfs: Don't leak memory when failing add fsid

[Tobin C. Harding]

A failed call to kobject_init_and_add() must be followed by a call to
kobject_put().  Currently in the error path when adding fs_devices we
are missing this call.  This could be fixed by calling
btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
Here we choose the second option because it prevents the slightly
unusual error path handling requirements of kobject from leaking out
into btrfs functions.

Add a call to kobject_put() in the error path of kobject_add_and_init().
This causes the release method to be called if kobject_init_and_add()
fails.  open_tree() is the function that calls btrfs_sysfs_add_fsid()
and the error code in this function is already written with the
assumption that the release method is called during the error path of
open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
fail_fsdev_sysfs label).

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 fs/btrfs/sysfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 5a5930e3d32b..2f078b77fe14 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -825,7 +825,12 @@ int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs,
 	fs_devs->fsid_kobj.kset = btrfs_kset;
 	error = kobject_init_and_add(&fs_devs->fsid_kobj,
 				&btrfs_ktype, parent, "%pU", fs_devs->fsid);
-	return error;
+	if (error) {
+		kobject_put(&fs_devs->fsid_kobj);
+		return error;
+	}
+
+	return 0;
 }
 
 int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
-- 
2.21.0

Re: [PATCH 2/2] fs: btrfs: Don't leak memory when failing add fsid

[Nikolay Borisov]

On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> A failed call to kobject_init_and_add() must be followed by a call to
> kobject_put().  Currently in the error path when adding fs_devices we
> are missing this call.  This could be fixed by calling
> btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
> by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
> Here we choose the second option because it prevents the slightly
> unusual error path handling requirements of kobject from leaking out
> into btrfs functions.
> 
> Add a call to kobject_put() in the error path of kobject_add_and_init().
> This causes the release method to be called if kobject_init_and_add()
> fails.  open_tree() is the function that calls btrfs_sysfs_add_fsid()
> and the error code in this function is already written with the
> assumption that the release method is called during the error path of
> open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
> fail_fsdev_sysfs label).

I'm not familiar with the internals of kobject but
btrfs_sysfs_remove_fsid calls __btrfs_sysfs_remove_fsid which in turn
does kobject_del followed by kobject_put so its sequence is not exactly
identical with your change. Presumably kobject_del is only required if
you want to dispose of successfully registered sysfs node. This implies
that __btrfs_sysfs_remove_fsid is actually broken when it comes to
handling failed sysfs_add_fsid?

> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  fs/btrfs/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 5a5930e3d32b..2f078b77fe14 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -825,7 +825,12 @@ int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs,
>  	fs_devs->fsid_kobj.kset = btrfs_kset;
>  	error = kobject_init_and_add(&fs_devs->fsid_kobj,
>  				&btrfs_ktype, parent, "%pU", fs_devs->fsid);
> -	return error;
> +	if (error) {
> +		kobject_put(&fs_devs->fsid_kobj);
> +		return error;
> +	}
> +
> +	return 0;
>  }
>  
>  int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)
> 

Re: [PATCH 2/2] fs: btrfs: Don't leak memory when failing add fsid

[Tobin C. Harding]

On Mon, May 13, 2019 at 09:04:49AM +0300, Nikolay Borisov wrote:
> 
> 
> On 13.05.19 г. 6:39 ч., Tobin C. Harding wrote:
> > A failed call to kobject_init_and_add() must be followed by a call to
> > kobject_put().  Currently in the error path when adding fs_devices we
> > are missing this call.  This could be fixed by calling
> > btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
> > by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
> > Here we choose the second option because it prevents the slightly
> > unusual error path handling requirements of kobject from leaking out
> > into btrfs functions.
> > 
> > Add a call to kobject_put() in the error path of kobject_add_and_init().
> > This causes the release method to be called if kobject_init_and_add()
> > fails.  open_tree() is the function that calls btrfs_sysfs_add_fsid()
> > and the error code in this function is already written with the
> > assumption that the release method is called during the error path of
> > open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
> > fail_fsdev_sysfs label).
> 
> I'm not familiar with the internals of kobject but
> btrfs_sysfs_remove_fsid calls __btrfs_sysfs_remove_fsid which in turn
> does kobject_del followed by kobject_put so its sequence is not exactly
> identical with your change. Presumably kobject_del is only required if
> you want to dispose of successfully registered sysfs node. This implies
> that __btrfs_sysfs_remove_fsid is actually broken when it comes to
> handling failed sysfs_add_fsid?

kobject_del() is not technically required in __btrfs_sysfs_remove_fsid()
since if kobject_put() drops the reference count to 0 and kobject_del()
has not been called then the kobject infrastructure will call
kobject_del() for us (and we get a pr_debug() message).  The code
sequence is correct although not _exactly_ written as the kobject
authors intended (I am not one of those authors, I'm just learning).

Thanks for looking at this.

	Tobin

Re: [PATCH 2/2] fs: btrfs: Don't leak memory when failing add fsid

[Greg Kroah-Hartman]

On Mon, May 13, 2019 at 01:39:12PM +1000, Tobin C. Harding wrote:
> A failed call to kobject_init_and_add() must be followed by a call to
> kobject_put().  Currently in the error path when adding fs_devices we
> are missing this call.  This could be fixed by calling
> btrfs_sysfs_remove_fsid() if btrfs_sysfs_add_fsid() returns an error or
> by adding a call to kobject_put() directly in btrfs_sysfs_add_fsid().
> Here we choose the second option because it prevents the slightly
> unusual error path handling requirements of kobject from leaking out
> into btrfs functions.
> 
> Add a call to kobject_put() in the error path of kobject_add_and_init().
> This causes the release method to be called if kobject_init_and_add()
> fails.  open_tree() is the function that calls btrfs_sysfs_add_fsid()
> and the error code in this function is already written with the
> assumption that the release method is called during the error path of
> open_tree() (as seen by the call to btrfs_sysfs_remove_fsid() under the
> fail_fsdev_sysfs label).
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  fs/btrfs/sysfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 5a5930e3d32b..2f078b77fe14 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -825,7 +825,12 @@ int btrfs_sysfs_add_fsid(struct btrfs_fs_devices *fs_devs,
>  	fs_devs->fsid_kobj.kset = btrfs_kset;
>  	error = kobject_init_and_add(&fs_devs->fsid_kobj,
>  				&btrfs_ktype, parent, "%pU", fs_devs->fsid);
> -	return error;
> +	if (error) {
> +		kobject_put(&fs_devs->fsid_kobj);
> +		return error;
> +	}
> +
> +	return 0;
>  }
>  
>  int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info)

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

Re: [PATCH 0/2] Fix kobject error path memleaks

[David Sterba]

On Mon, May 13, 2019 at 01:39:10PM +1000, Tobin C. Harding wrote:
> Is it ok to send patches during the merge window?

Yes (depends on subsystem), the feedback for patches that are not fixes
could be delayed after the merge window closes.

> Applies on top of
> Linus' mainline tag: v5.1, happy to rebase if there are conflicts.
> 
> While auditing kobject_init_and_add() calls throughout the kernel it was
> found that btrfs potentially has a couple of memleaks in the error path
> code for kobject_init_and_add().
> 
> Failing calls to kobject_init_and_add() should be followed by a call to
> kobject_put() since kobject_init_and_add() always calls kobject_init().
> 
> Of note, adding kobject_put() causes the release method to be called if
> kobject_init_and_add() fails.  For patch #1 this means we don't have to
> manually free the space_info or call percpu_counter_destroy() since
> these are both done by the release method.  In the second patch, I
> believe the added call to kobject_put() fits in with the fs_devices
> lifecycle assumptions of open_ctree() but please could you review since
> I am new to this code.

We use the cleanup-after-error pattern where it's up to the callee to
clean up, so it's right to do it like as you did. Patches added to the
queue that's for 5.2-rcX. Thanks.