[PATCH] gfs2: Fix error path kobject memory leak

[PATCH] gfs2: 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.

Function always calls kobject_init_and_add() which always calls
kobject_init().

It is safe to leave object destruction up to the kobject release
function and never free it manually.

Remove call to kfree() and always call kobject_put() in the error path.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---

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.

thanks,
Tobin.

 fs/gfs2/sys.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 1787d295834e..98586b139386 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -661,8 +661,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
 	if (error)
 		goto fail_reg;
 
-	sysfs_frees_sdp = 1; /* Freeing sdp is now done by sysfs calling
-				function gfs2_sbd_release. */
 	error = sysfs_create_group(&sdp->sd_kobj, &tune_group);
 	if (error)
 		goto fail_reg;
@@ -687,10 +685,7 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
 fail_reg:
 	free_percpu(sdp->sd_lkstats);
 	fs_err(sdp, "error %d adding sysfs files\n", error);
-	if (sysfs_frees_sdp)
-		kobject_put(&sdp->sd_kobj);
-	else
-		kfree(sdp);
+	kobject_put(&sdp->sd_kobj);
 	sb->s_fs_info = NULL;
 	return error;
 }
-- 
2.21.0

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Greg Kroah-Hartman]

On Mon, May 13, 2019 at 01:32:13PM +1000, Tobin C. Harding wrote:
> If a call to kobject_init_and_add() fails we must call kobject_put()
> otherwise we leak memory.
> 
> Function always calls kobject_init_and_add() which always calls
> kobject_init().
> 
> It is safe to leave object destruction up to the kobject release
> function and never free it manually.
> 
> Remove call to kfree() and always call kobject_put() in the error path.
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
> 
> 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.
> 
> thanks,
> Tobin.
> 
>  fs/gfs2/sys.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index 1787d295834e..98586b139386 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -661,8 +661,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
>  	if (error)
>  		goto fail_reg;
>  
> -	sysfs_frees_sdp = 1; /* Freeing sdp is now done by sysfs calling
> -				function gfs2_sbd_release. */

You should also delete this variable at the top of the function, as it
is now only set once there and never used.

With that:

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

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Tobin C. Harding]

On Mon, May 13, 2019 at 09:14:05AM +0200, Greg Kroah-Hartman wrote:
> On Mon, May 13, 2019 at 01:32:13PM +1000, Tobin C. Harding wrote:
> > If a call to kobject_init_and_add() fails we must call kobject_put()
> > otherwise we leak memory.
> > 
> > Function always calls kobject_init_and_add() which always calls
> > kobject_init().
> > 
> > It is safe to leave object destruction up to the kobject release
> > function and never free it manually.
> > 
> > Remove call to kfree() and always call kobject_put() in the error path.
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> > 
> > 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.
> > 
> > thanks,
> > Tobin.
> > 
> >  fs/gfs2/sys.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> > index 1787d295834e..98586b139386 100644
> > --- a/fs/gfs2/sys.c
> > +++ b/fs/gfs2/sys.c
> > @@ -661,8 +661,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
> >  	if (error)
> >  		goto fail_reg;
> >  
> > -	sysfs_frees_sdp = 1; /* Freeing sdp is now done by sysfs calling
> > -				function gfs2_sbd_release. */
> 
> You should also delete this variable at the top of the function, as it
> is now only set once there and never used.

Thanks, I should have gotten a compiler warning for that.  I was feeling
so confident with my builds this morning ... pays not to get too cocky
I suppose.

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

Thanks, will re-spin.

	Tobin.

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Andreas Gruenbacher]

On Mon, 13 May 2019 at 12:40, Tobin C. Harding <me@tobin.cc> wrote:
>
> On Mon, May 13, 2019 at 09:14:05AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, May 13, 2019 at 01:32:13PM +1000, Tobin C. Harding wrote:
> > > If a call to kobject_init_and_add() fails we must call kobject_put()
> > > otherwise we leak memory.
> > >
> > > Function always calls kobject_init_and_add() which always calls
> > > kobject_init().
> > >
> > > It is safe to leave object destruction up to the kobject release
> > > function and never free it manually.
> > >
> > > Remove call to kfree() and always call kobject_put() in the error path.
> > >
> > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > > ---
> > >
> > > 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.
> > >
> > > thanks,
> > > Tobin.
> > >
> > >  fs/gfs2/sys.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> > > index 1787d295834e..98586b139386 100644
> > > --- a/fs/gfs2/sys.c
> > > +++ b/fs/gfs2/sys.c
> > > @@ -661,8 +661,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
> > >     if (error)
> > >             goto fail_reg;
> > >
> > > -   sysfs_frees_sdp = 1; /* Freeing sdp is now done by sysfs calling
> > > -                           function gfs2_sbd_release. */
> >
> > You should also delete this variable at the top of the function, as it
> > is now only set once there and never used.
>
> Thanks, I should have gotten a compiler warning for that.  I was feeling
> so confident with my builds this morning ... pays not to get too cocky
> I suppose.
>
> > With that:
> >
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Thanks, will re-spin.

Don't bother, I'll fix that up.

Thanks,
Andreas

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Tobin C. Harding]

On Mon, May 13, 2019 at 06:41:23PM +0200, Andreas Gruenbacher wrote:
> On Mon, 13 May 2019 at 12:40, Tobin C. Harding <me@tobin.cc> wrote:
> >
> > On Mon, May 13, 2019 at 09:14:05AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, May 13, 2019 at 01:32:13PM +1000, Tobin C. Harding wrote:
> > > > If a call to kobject_init_and_add() fails we must call kobject_put()
> > > > otherwise we leak memory.
> > > >
> > > > Function always calls kobject_init_and_add() which always calls
> > > > kobject_init().
> > > >
> > > > It is safe to leave object destruction up to the kobject release
> > > > function and never free it manually.
> > > >
> > > > Remove call to kfree() and always call kobject_put() in the error path.
> > > >
> > > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > > > ---
> > > >
> > > > 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.
> > > >
> > > > thanks,
> > > > Tobin.
> > > >
> > > >  fs/gfs2/sys.c | 7 +------
> > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > >
> > > > diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> > > > index 1787d295834e..98586b139386 100644
> > > > --- a/fs/gfs2/sys.c
> > > > +++ b/fs/gfs2/sys.c
> > > > @@ -661,8 +661,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
> > > >     if (error)
> > > >             goto fail_reg;
> > > >
> > > > -   sysfs_frees_sdp = 1; /* Freeing sdp is now done by sysfs calling
> > > > -                           function gfs2_sbd_release. */
> > >
> > > You should also delete this variable at the top of the function, as it
> > > is now only set once there and never used.
> >
> > Thanks, I should have gotten a compiler warning for that.  I was feeling
> > so confident with my builds this morning ... pays not to get too cocky
> > I suppose.
> >
> > > With that:
> > >
> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Thanks, will re-spin.
> 
> Don't bother, I'll fix that up.

Thanks man!

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Andreas Gruenbacher]

On Tue, 14 May 2019 at 00:47, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, May 13, 2019 at 3:37 PM Andreas Gruenbacher <agruenba@redhat.com> wrote:
> >
> > Sorry, I should have been more explicit. Would you mind taking this
> > patch, please? If it's more convenient or more appropriate, I'll send
> > a pull request instead.
>
> Done.
>
> However, I'd like to point out that when I see patches from people who
> I normally get a pull request from, I usually ignore them.
>
> Particularly when they are in some thread with discussion, I'll often
> just assume that  the patch is part of the thread, not really meant
> for me in particular.
>
> In this case I happened to notice that suddenly my participation
> status changed, which is why I asked, but in general I might have just
> archived the thread with the assumption that I'll be getting the patch
> later as a git pull.
>
> Just so you'll be aware of this in the future, in case I don't react...

Thanks.

Copying Jon because this looks like useful information for other
maintainers as well.

Andreas

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Linus Torvalds]

On Mon, May 13, 2019 at 3:37 PM Andreas Gruenbacher <agruenba@redhat.com> wrote:
>
> Sorry, I should have been more explicit. Would you mind taking this
> patch, please? If it's more convenient or more appropriate, I'll send
> a pull request instead.

Done.

However,I'd like to point out that when I see patches from people who
I normally get a pull request from, I usually ignore them.

Particularly when they are in some thread with discussion, I'll often
just assume that  th epatch is part of the thread, not really meant
for me in particular.

In this case I happened to notice that suddenly my participation
status changed, which is why I asked, but in general I might hav ejust
archived the thread with the assumption that I'll be getting the patch
later as a git pull.

Just so you'll be aware of this in the future, in case I don't react...

               Linus

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Andreas Gruenbacher]

On Tue, 14 May 2019 at 00:27, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Andreas,
>  did you intend for me to take this as a patch from the email, or will
> I get a pull request with this later?

Sorry, I should have been more explicit. Would you mind taking this
patch, please? If it's more convenient or more appropriate, I'll send
a pull request instead.

Thanks,
Andreas

Re: [PATCH] gfs2: Fix error path kobject memory leak

[Linus Torvalds]

Andreas,
 did you intend for me to take this as a patch from the email, or will
I get a pull request with this later?

             Linus

[PATCH] gfs2: Fix error path kobject memory leak

[Andreas Gruenbacher]

From: "Tobin C. Harding" <tobin@kernel.org>

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

Function gfs2_sys_fs_add always calls kobject_init_and_add() which
always calls kobject_init().

It is safe to leave object destruction up to the kobject release
function and never free it manually.

Remove call to kfree() and always call kobject_put() in the error path.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/sys.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 1787d295834e..08e4996adc23 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -650,7 +650,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
 	char ro[20];
 	char spectator[20];
 	char *envp[] = { ro, spectator, NULL };
-	int sysfs_frees_sdp = 0;
 
 	sprintf(ro, "RDONLY=%d", sb_rdonly(sb));
 	sprintf(spectator, "SPECTATOR=%d", sdp->sd_args.ar_spectator ? 1 : 0);
@@ -661,8 +660,6 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
 	if (error)
 		goto fail_reg;
 
-	sysfs_frees_sdp = 1; /* Freeing sdp is now done by sysfs calling
-				function gfs2_sbd_release. */
 	error = sysfs_create_group(&sdp->sd_kobj, &tune_group);
 	if (error)
 		goto fail_reg;
@@ -687,10 +684,7 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
 fail_reg:
 	free_percpu(sdp->sd_lkstats);
 	fs_err(sdp, "error %d adding sysfs files\n", error);
-	if (sysfs_frees_sdp)
-		kobject_put(&sdp->sd_kobj);
-	else
-		kfree(sdp);
+	kobject_put(&sdp->sd_kobj);
 	sb->s_fs_info = NULL;
 	return error;
 }
-- 
2.20.1