[RFC] Add inline routine to free memory used in kobject name

[RFC] Add inline routine to free memory used in kobject name

[Larry Finger]

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.
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);
+}
+
 extern __printf(2, 3)
 int dev_set_name(struct device *dev, const char *name, ...);
 

Re: [RFC] Add inline routine to free memory used in kobject name

[Greg KH]

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 :)

thanks,

greg k-h

Re: [RFC] Add inline routine to free memory used in kobject name

[Larry Finger]

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);
  	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);
  	kfree(card);
  	return NULL;
  }

These changes clear up the memory leak that started the whole subject. The 
difference would be between
	kfree(card->dev.kobj.name);
and
	dev_free_name(&card->dev);

I have not looked at every line in the kernel containing both "kfree" and "name" 
to see how many other places that the new routine could be used. I'm not even 
sure how other drivers free the space allocated for a name.

Larry

Re: [RFC] Add inline routine to free memory used in kobject name

[Greg KH]

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