LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] gfs2: Fix error path kobject memory leak
@ 2019-05-13  3:32 Tobin C. Harding
  2019-05-13  7:14 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Tobin C. Harding @ 2019-05-13  3:32 UTC (permalink / raw)
  To: Bob Peterson, Andreas Gruenbacher
  Cc: Tobin C. Harding, Greg Kroah-Hartman, Rafael J. Wysocki,
	cluster-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13  3:32 [PATCH] gfs2: Fix error path kobject memory leak Tobin C. Harding
@ 2019-05-13  7:14 ` Greg Kroah-Hartman
  2019-05-13 10:39   ` Tobin C. Harding
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2019-05-13  7:14 UTC (permalink / raw)
  To: Tobin C. Harding
  Cc: Bob Peterson, Andreas Gruenbacher, Rafael J. Wysocki,
	cluster-devel, linux-kernel

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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13  7:14 ` Greg Kroah-Hartman
@ 2019-05-13 10:39   ` Tobin C. Harding
  2019-05-13 16:41     ` Andreas Gruenbacher
  0 siblings, 1 reply; 10+ messages in thread
From: Tobin C. Harding @ 2019-05-13 10:39 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Tobin C. Harding, Bob Peterson, Andreas Gruenbacher,
	Rafael J. Wysocki, cluster-devel, linux-kernel

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13 10:39   ` Tobin C. Harding
@ 2019-05-13 16:41     ` Andreas Gruenbacher
  2019-05-13 21:40       ` Tobin C. Harding
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Gruenbacher @ 2019-05-13 16:41 UTC (permalink / raw)
  To: Tobin C. Harding
  Cc: Greg Kroah-Hartman, Tobin C. Harding, Bob Peterson,
	Rafael J. Wysocki, cluster-devel, LKML

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13 16:41     ` Andreas Gruenbacher
@ 2019-05-13 21:40       ` Tobin C. Harding
  0 siblings, 0 replies; 10+ messages in thread
From: Tobin C. Harding @ 2019-05-13 21:40 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Greg Kroah-Hartman, Tobin C. Harding, Bob Peterson,
	Rafael J. Wysocki, cluster-devel, LKML

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!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13 22:47     ` Linus Torvalds
@ 2019-05-13 22:57       ` Andreas Gruenbacher
  0 siblings, 0 replies; 10+ messages in thread
From: Andreas Gruenbacher @ 2019-05-13 22:57 UTC (permalink / raw)
  To: Linus Torvalds, Jonathan Corbet
  Cc: cluster-devel, Linux List Kernel Mailing, Tobin C. Harding,
	Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13 22:37   ` Andreas Gruenbacher
@ 2019-05-13 22:47     ` Linus Torvalds
  2019-05-13 22:57       ` Andreas Gruenbacher
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2019-05-13 22:47 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: cluster-devel, Linux List Kernel Mailing, Tobin C. Harding,
	Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13 22:27 ` Linus Torvalds
@ 2019-05-13 22:37   ` Andreas Gruenbacher
  2019-05-13 22:47     ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Gruenbacher @ 2019-05-13 22:37 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: cluster-devel, Linux List Kernel Mailing, Tobin C. Harding,
	Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] gfs2: Fix error path kobject memory leak
  2019-05-13 19:59 Andreas Gruenbacher
@ 2019-05-13 22:27 ` Linus Torvalds
  2019-05-13 22:37   ` Andreas Gruenbacher
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2019-05-13 22:27 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: cluster-devel, Linux List Kernel Mailing, Tobin C. Harding,
	Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] gfs2: Fix error path kobject memory leak
@ 2019-05-13 19:59 Andreas Gruenbacher
  2019-05-13 22:27 ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Gruenbacher @ 2019-05-13 19:59 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: cluster-devel, linux-kernel, Tobin C. Harding,
	Greg Kroah-Hartman, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-05-13 22:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13  3:32 [PATCH] gfs2: Fix error path kobject memory leak Tobin C. Harding
2019-05-13  7:14 ` Greg Kroah-Hartman
2019-05-13 10:39   ` Tobin C. Harding
2019-05-13 16:41     ` Andreas Gruenbacher
2019-05-13 21:40       ` Tobin C. Harding
2019-05-13 19:59 Andreas Gruenbacher
2019-05-13 22:27 ` Linus Torvalds
2019-05-13 22:37   ` Andreas Gruenbacher
2019-05-13 22:47     ` Linus Torvalds
2019-05-13 22:57       ` Andreas Gruenbacher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).