Date: Mon, 7 Oct 2013 18:58:49 -0700
From: Greg KH <gregkh@linuxfoundation.org>
To: Larry Finger <Larry.Finger@lwfinger.net>
Cc: linux-kernel@vger.kernel.org, Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [RFC] Add inline routine to free memory used in kobject name
Message-ID: <20131008015849.GA2707@kroah.com>
References: <5252f2cd.33yoX1hUWLRsRUrq%Larry.Finger@lwfinger.net>
 <20131007201044.GA6305@kroah.com>
 <52531C1B.9070502@lwfinger.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <52531C1B.9070502@lwfinger.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3827
Lines: 100

On Mon, Oct 07, 2013 at 03:39:55PM -0500, Larry Finger wrote:
> On 10/07/2013 03:10 PM, Greg KH wrote:
> > On Mon, Oct 07, 2013 at 12:43:41PM -0500, Larry Finger wrote:
> >> At present, if one wants to free the memory allocation used for
> >> a dev->kobj name, it is necessary to go quite deeply into the structure.
> >
> > Why would you ever want to do this?
> >
> >> To avoid this much dependence on the structure details in driver
> >> code, a new inline routine is created.
> >>
> >> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> >> ---
> >>
> >> Index: wireless-testing-save/include/linux/device.h
> >> ===================================================================
> >> --- wireless-testing-save.orig/include/linux/device.h
> >> +++ wireless-testing-save/include/linux/device.h
> >> @@ -27,6 +27,7 @@
> >>   #include <linux/ratelimit.h>
> >>   #include <linux/uidgid.h>
> >>   #include <asm/device.h>
> >> +#include <linux/slab.h>
> >>
> >>   struct device;
> >>   struct device_private;
> >> @@ -789,6 +790,11 @@ static inline const char *dev_name(const
> >>   	return kobject_name(&dev->kobj);
> >>   }
> >>
> >> +static inline void dev_free_name(struct device *dev)
> >> +{
> >> +	kfree(dev->kobj.name);
> >> +}
> >
> > Please show how you would use this function, I can't add functions that
> > no one calls.
> >
> > And given that this type of thing hasn't been needed before, I'm
> > thinking that it still isn't needed :)
> 
> In the thread at 
> http://lkml.indiana.edu/hypermail/linux/kernel/1310.0/02008.html, I reported a 
> leak of kobj->name in the error path of the memstick driver. My solution was to 
> free it in the error path by using
> 
> 	kfree(card->dev.kobj.name);
> 
> Catalin Marinas responded with "It looks weird to go into dev.kobj internals 
> here for freeing the name. There is also memstick_free_card() which doesn't seem 
> to do anything about the name freeing."
> 
> Later in the thread, he agreed that having a new function sounded like a good 
> idea. I should have submitted the second patch using the new function as follows:
> 
> Index: wireless-testing-save/drivers/memstick/core/memstick.c
> ===================================================================
> --- wireless-testing-save.orig/drivers/memstick/core/memstick.c
> +++ wireless-testing-save/drivers/memstick/core/memstick.c
> @@ -195,6 +195,7 @@ static void memstick_free_card(struct de
>   {
>   	struct memstick_dev *card = container_of(dev, struct memstick_dev,
>   						 dev);
> +	dev_free_name(&card->dev);

This is incorrect, the cleanup for the struct device will have already
freed the kobject name variable, you just did it twice :(

>   	kfree(card);
>   }
> 
> @@ -415,6 +416,7 @@ static struct memstick_dev *memstick_all
>   	return card;
>   err_out:
>   	host->card = old_card;
> +	dev_free_name(&card->dev);

This shouldn't be needed, otherwise we need to do the same thing for all
error paths for creating struct device.

I've been down this path before, and every time I get confused.
kobject_cleanup() frees the name of the kobject, and if you ever create
a kobject (or struct device), you _have_ to properly dispose of it, you
can't just think you can call 'kfree()' on the object if something fails
(the driver core documentation should be saying this, right?)

It's not just the name of the kobject that you will leak, struct device
also has lots of internal pointers that needs to be freed.

So this memstick patch isn't needed from what I can tell, so I don't see
why we need the new .h inline function either.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/