Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 2 May 2019 09:17:42 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     "Tobin C. Harding" <me@tobin.cc>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>, cl@linux.com,
        tycho@tycho.ws, willy@infradead.org, linux-kernel@vger.kernel.org
Subject: Re: memleak around kobject_init_and_add()
Message-ID: <20190502071742.GC16247@kroah.com>
References: <20190427081330.GA26788@eros.localdomain>
 <20190427192809.GA8454@kroah.com>
 <20190501215616.GD18827@eros.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190501215616.GD18827@eros.localdomain>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 02, 2019 at 07:56:16AM +1000, Tobin C. Harding wrote:
> On Sat, Apr 27, 2019 at 09:28:09PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 27, 2019 at 06:13:30PM +1000, Tobin C. Harding wrote:
> > > (Note at bottom on reasons for 'To' list 'Cc' list)
> > > 
> > > Hi,
> > > 
> > > kobject_init_and_add() seems to be routinely misused.  A failed call to this
> > > function requires a call to kobject_put() otherwise we leak memory.
> > > 
> > > Examples memleaks can be seen in:
> > > 
> > > 	mm/slub.c
> > > 	fs/btrfs/sysfs.c
> > > 	fs/xfs/xfs_sysfs.h: xfs_sysfs_init()
> > > 
> > >  Question: Do we fix the misuse or fix the API?
> > 
> > Fix the misuse.
> > 
> > > $ git grep kobject_init_and_add | wc -l
> > > 117
> > > 
> > > Either way, we will have to go through all 117 call sites and check them.
> > 
> > Yes.  Same for other functions like device_add(), that is the "pattern"
> > those users must follow.
> > 
> > > I
> > > don't mind fixing them all but I don't want to do it twice because I chose the
> > > wrong option.  Reaching out to those more experienced for a suggestion please.
> > > 
> > > Fix the API
> > > -----------
> > > 
> > > Typically init functions do not require cleanup if they fail, this argument
> > > leads to this patch
> > > 
> > > diff --git a/lib/kobject.c b/lib/kobject.c
> > > index aa89edcd2b63..62328054bbd0 100644
> > > --- a/lib/kobject.c
> > > +++ b/lib/kobject.c
> > > @@ -453,6 +453,9 @@ int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype,
> > >  	retval = kobject_add_varg(kobj, parent, fmt, args);
> > >  	va_end(args);
> > >  
> > > +	if (retval)
> > > +		kobject_put(kobj);
> > > +
> > >  	return retval;
> > >  }
> > >  EXPORT_SYMBOL_GPL(kobject_init_and_add);
> > 
> > I would _love_ to do this, but realize what a kobject really is.
> > 
> > It's just a "base object" that is embedded inside of some other object.
> > The kobject core has no idea what is going on outside of itself.  If the
> > kobject_init_and_add() function fails, it can NOT drop the last
> > reference on itself, as that would cause the memory owned by the _WHOLE_
> > structure the kobject is embedded in, to be freed.
> > 
> > And the kobject core can not "know" that something else needed to be
> > done _before_ that memory could be freed.  What if the larger structure
> > needs to have some other destructor called on it first?  What if
> > some other api initialization needs to be torn down.
> > 
> > As an example, consider this code:
> > 
> > struct foo {
> > 	struct kobject kobj;
> > 	struct baz *baz;
> > };
> > 
> > void foo_release(struct kobject *kobj)
> > {
> > 	struct foo *foo = container_of(kobj, struct foo, kobj);
> > 	kfree(foo);
> > }
> > 
> > struct kobj_type foo_ktype = {
> > 	.release = foo_release,
> > };
> > 
> > struct foo *foo_create(struct foo *parent, char *name)
> > {
> > 	struct *foo;
> > 
> > 	foo = kzalloc(sizeof(*foo), GFP_KERNEL);
> > 	if (!foo)
> > 		return NULL;
> > 
> > 	foo->baz = baz_create(name);
> > 	if (!foo->baz)
> > 		return NULL;
> > 
> > 	ret = kobject_init_and_add(&foo->kobj, foo_ktype, &parent->kobj, "foo-%s", name);
> > 	if (ret) {
> > 		baz_destroy(foo->baz);
> > 		kobject_put(&foo->kobj);
> > 		return NULL;
> > 	}
> > 
> > 	return foo;
> > }
> > 
> > void foo_destroy(struct foo *foo)
> > {
> > 	baz_destroy(foo->baz);
> > 	kobject_del(&foo->kobj);
> 	kojbect_put(&foo->kobj);
> > }
> 
> Does this need this extra call to kobject_put()?  Then foo_create()
> leaves foo with a refcount of 1 and foo_destroy drops that refcount.

Oops, no, I messed this up, it should _only_ be a call to
kobject_put(), kobject_del() is not needed here.

kobject_del() is for people who "really want to control the lifetime" of
a kobject.  All it does is remove the kobject from sysfs, and drop the
parent reference of the kobject, allowing the kobject to be "free" on
it's own.  Later a kobject_put() call must be called on it to really
clean it up.

If you just call kobject_put(), and this is the last reference,
kobject_del() will be correctly called for you by the kobject code, as
it "knows" this is time to clean up the sysfs entities.

A "normal" user should never have to call kobject_del().

thanks,

greg k-h