[PATCH] kobject_set_name_vargs memory leak

[PATCH] kobject_set_name_vargs memory leak

[Sergey Senozhatsky]

Hello.
I suppose this patch fixes memory leak in kobject.c
Correct me if I'm wrong.
Thanks.
-----------

Fix memory leak when kobject_set_name_vargs returns -ENOMEM.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
---
diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..922cd8c 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 		return 0;
 
 	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
-	if (!kobj->name)
+	if (!kobj->name) {
+		kfree(old_name);
 		return -ENOMEM;
+	}
 
 	/* ewww... some of these buggers have '/' in the name ... */
 	while ((s = strchr(kobj->name, '/')))

Re: [PATCH] kobject_set_name_vargs memory leak

[Greg KH]

On Fri, Jun 26, 2009 at 05:36:52PM +0300, Sergey Senozhatsky wrote:
> Hello.
> I suppose this patch fixes memory leak in kobject.c
> Correct me if I'm wrong.
> Thanks.
> -----------
> 
> Fix memory leak when kobject_set_name_vargs returns -ENOMEM.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> ---
> diff --git a/lib/kobject.c b/lib/kobject.c
> index b512b74..922cd8c 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>  		return 0;
>  
>  	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> -	if (!kobj->name)
> +	if (!kobj->name) {
> +		kfree(old_name);
>  		return -ENOMEM;
> +	}

We've been through this before (search lkml archives).  If kvasprintf
fails, then we don't want to free old_name, as the caller might want to
do something with it.

Or something along those lines, I can't remember the exact reasoning
this early in the morning.

Kay, do you remember?

thanks,

greg k-h

Re: [PATCH] kobject_set_name_vargs memory leak

[Sergey Senozhatsky]

On (06/26/09 07:49), Greg KH wrote:
> Date: Fri, 26 Jun 2009 07:49:49 -0700
> From: Greg KH <gregkh@suse.de>
> To: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> Cc: Kay Sievers <kay.sievers@vrfy.org>,
> 	"Eric W. Biederman" <ebiederm@xmission.com>,
> 	linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kobject_set_name_vargs memory leak
> User-Agent: Mutt/1.5.19 (2009-01-05)
> 
> On Fri, Jun 26, 2009 at 05:36:52PM +0300, Sergey Senozhatsky wrote:
> > Hello.
> > I suppose this patch fixes memory leak in kobject.c
> > Correct me if I'm wrong.
> > Thanks.
> > -----------
> > 
> > Fix memory leak when kobject_set_name_vargs returns -ENOMEM.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> > ---
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index b512b74..922cd8c 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> >  		return 0;
> >  
> >  	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> > -	if (!kobj->name)
> > +	if (!kobj->name) {
> > +		kfree(old_name);
> >  		return -ENOMEM;
> > +	}
> 
> We've been through this before (search lkml archives).  If kvasprintf
> fails, then we don't want to free old_name, as the caller might want to
> do something with it.
> 
Hello Greg,

int kobject_set_name_vargs.... {
	const char *old_name = kobj->name;

old_name is local variable.

In the following lines we overwrite kobject->name.

	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
	if (!kobj->name)
		return -ENOMEM;

It's not clear to me how we can do anything (including kfree) with old_name after 'return -ENOMEM'.

Well, I'll try to search lklm.
Thanks.


> Or something along those lines, I can't remember the exact reasoning
> this early in the morning.
> 
> Kay, do you remember?
> 
> thanks,
> 
> greg k-h
> 

	Sergey

Re: [PATCH] kobject_set_name_vargs memory leak

[Eric W. Biederman]

Sergey Senozhatsky <sergey.senozhatsky@mail.by> writes:

> On (06/26/09 07:49), Greg KH wrote:
>> Date: Fri, 26 Jun 2009 07:49:49 -0700
>> From: Greg KH <gregkh@suse.de>
>> To: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
>> Cc: Kay Sievers <kay.sievers@vrfy.org>,
>> 	"Eric W. Biederman" <ebiederm@xmission.com>,
>> 	linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] kobject_set_name_vargs memory leak
>> User-Agent: Mutt/1.5.19 (2009-01-05)
>> 
>> On Fri, Jun 26, 2009 at 05:36:52PM +0300, Sergey Senozhatsky wrote:
>> > Hello.
>> > I suppose this patch fixes memory leak in kobject.c
>> > Correct me if I'm wrong.
>> > Thanks.
>> > -----------
>> > 
>> > Fix memory leak when kobject_set_name_vargs returns -ENOMEM.
>> > 
>> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
>> > ---
>> > diff --git a/lib/kobject.c b/lib/kobject.c
>> > index b512b74..922cd8c 100644
>> > --- a/lib/kobject.c
>> > +++ b/lib/kobject.c
>> > @@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>> >  		return 0;
>> >  
>> >  	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
>> > -	if (!kobj->name)
>> > +	if (!kobj->name) {
>> > +		kfree(old_name);
>> >  		return -ENOMEM;
>> > +	}
>> 
>> We've been through this before (search lkml archives).  If kvasprintf
>> fails, then we don't want to free old_name, as the caller might want to
>> do something with it.
>> 
> Hello Greg,
>
> int kobject_set_name_vargs.... {
> 	const char *old_name = kobj->name;
>
> old_name is local variable.
>
> In the following lines we overwrite kobject->name.
>
> 	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> 	if (!kobj->name)
> 		return -ENOMEM;
>
> It's not clear to me how we can do anything (including kfree) with old_name after 'return -ENOMEM'.

My feel is that if we fail we should restore kobject->name to old_name.

That should also prevent the leak without getting us into trouble elsewhere.

Eric

Re: [PATCH] kobject_set_name_vargs memory leak

[Sergey Senozhatsky]

On (06/26/09 16:00), Eric W. Biederman wrote:
> >> > Fix memory leak when kobject_set_name_vargs returns -ENOMEM.
> >> > 
> >> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> >> > ---
> >> > diff --git a/lib/kobject.c b/lib/kobject.c
> >> > index b512b74..922cd8c 100644
> >> > --- a/lib/kobject.c
> >> > +++ b/lib/kobject.c
> >> > @@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> >> >  		return 0;
> >> >  
> >> >  	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> >> > -	if (!kobj->name)
> >> > +	if (!kobj->name) {
> >> > +		kfree(old_name);
> >> >  		return -ENOMEM;
> >> > +	}
> >> 
> >> We've been through this before (search lkml archives).  If kvasprintf
> >> fails, then we don't want to free old_name, as the caller might want to
> >> do something with it.
> >> 
> > Hello Greg,
> >
> > int kobject_set_name_vargs.... {
> > 	const char *old_name = kobj->name;
> >
> > old_name is local variable.
> >
> > In the following lines we overwrite kobject->name.
> >
> > 	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> > 	if (!kobj->name)
> > 		return -ENOMEM;
> >
> > It's not clear to me how we can do anything (including kfree) with old_name after 'return -ENOMEM'.
> 
> My feel is that if we fail we should restore kobject->name to old_name.
> 
> That should also prevent the leak without getting us into trouble elsewhere.
> 
> Eric
> 
Or work with 'new_name' and overwrite kobject->name only 'if(new_name)'.
I thought about restoring. ( Blue or Red Pill? :) )

diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..d6b1502 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -222,8 +222,10 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 		return 0;
 
 	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
-	if (!kobj->name)
+	if (!kobj->name) {
+		kobj->name = old_name;
 		return -ENOMEM;
+	}
 
 	/* ewww... some of these buggers have '/' in the name ... */
 	while ((s = strchr(kobj->name, '/')))


	Sergey

Re: [PATCH] kobject_set_name_vargs memory leak

[Sergey Senozhatsky]

On (06/26/09 07:49), Greg KH wrote:
> We've been through this before (search lkml archives).  If kvasprintf
> fails, then we don't want to free old_name, as the caller might want to
> do something with it.
> 
> Or something along those lines, I can't remember the exact reasoning
> this early in the morning.
> 
> Kay, do you remember?
>
I found.
http://lkml.org/lkml/2009/5/11/11

>kobject with name set before should not come into this function,
>kobject_rename should be used instead.

It's just would be safer to kfree or restore I guess.
 
> thanks,
> 
> greg k-h
> 

	Sergey

Re: [PATCH] kobject_set_name_vargs memory leak

[Eric W. Biederman]

Sergey Senozhatsky <sergey.senozhatsky@mail.by> writes:

> On (06/26/09 07:49), Greg KH wrote:
>> We've been through this before (search lkml archives).  If kvasprintf
>> fails, then we don't want to free old_name, as the caller might want to
>> do something with it.
>> 
>> Or something along those lines, I can't remember the exact reasoning
>> this early in the morning.
>> 
>> Kay, do you remember?
>>
> I found.
> http://lkml.org/lkml/2009/5/11/11
>
>>kobject with name set before should not come into this function,
>>kobject_rename should be used instead.
>
> It's just would be safer to kfree or restore I guess.

Yes.  There does seem to be a good point in there that the code should be:
BUG_ON(kobj->name);

And otherwise simply not handle old_name at all.

Eric

Re: [PATCH] kobject_set_name_vargs memory leak

[Sergey Senozhatsky]

On (06/26/09 19:10), Eric W. Biederman wrote:
> Date: Fri, 26 Jun 2009 19:10:08 -0700
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> To: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> Cc: Greg KH <gregkh@suse.de>, Kay Sievers <kay.sievers@vrfy.org>,
> 	linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kobject_set_name_vargs memory leak
> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux)
> 
> Sergey Senozhatsky <sergey.senozhatsky@mail.by> writes:
> 
> > On (06/26/09 07:49), Greg KH wrote:
> >> We've been through this before (search lkml archives).  If kvasprintf
> >> fails, then we don't want to free old_name, as the caller might want to
> >> do something with it.
> >> 
> >> Or something along those lines, I can't remember the exact reasoning
> >> this early in the morning.
> >> 
> >> Kay, do you remember?
> >>
> > I found.
> > http://lkml.org/lkml/2009/5/11/11
> >
> >>kobject with name set before should not come into this function,
> >>kobject_rename should be used instead.
> >
> > It's just would be safer to kfree or restore I guess.
> 
> Yes.  There does seem to be a good point in there that the code should be:
> BUG_ON(kobj->name);
> 

> And otherwise simply not handle old_name at all.
> 

I run with:

	if (kobj->name) {
		printk(KERN_ERR "name:%s  fmt:%s\n", kobj->name, fmt);
		dump_stack();
	}

I seems ok not to handle old_name.

>Dave Young
>I rethought about this problem, does such issue exist really? I means
>that kobject->name != NULL scenario.
>
>there's following comments of this function:
>
> * This sets the name of the kobject.  If you have already added the
> * kobject to the system, you must call kobject_rename() in order to
> * change the name of the kobject


Greg, Kay, Eric, what do you think?


diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..fd1983a 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -215,7 +215,6 @@ static int kobject_add_internal(struct kobject *kobj)
 int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 				  va_list vargs)
 {
-	const char *old_name = kobj->name;
 	char *s;
 
 	if (kobj->name && !fmt)


> Eric
> 

	Sergey

Re: [PATCH] kobject_set_name_vargs memory leak

[Sergey Senozhatsky]

On (06/26/09 19:10), Eric W. Biederman wrote:
> Date: Fri, 26 Jun 2009 19:10:08 -0700
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> To: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
> Cc: Greg KH <gregkh@suse.de>, Kay Sievers <kay.sievers@vrfy.org>,
> 	linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kobject_set_name_vargs memory leak
> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux)
> 
> Sergey Senozhatsky <sergey.senozhatsky@mail.by> writes:
> 
> > On (06/26/09 07:49), Greg KH wrote:
> >> We've been through this before (search lkml archives).  If kvasprintf
> >> fails, then we don't want to free old_name, as the caller might want to
> >> do something with it.
> >> 
> >> Or something along those lines, I can't remember the exact reasoning
> >> this early in the morning.
> >> 
> >> Kay, do you remember?
> >>
> > I found.
> > http://lkml.org/lkml/2009/5/11/11
> >
> >>kobject with name set before should not come into this function,
> >>kobject_rename should be used instead.
> >
> > It's just would be safer to kfree or restore I guess.
> 
> Yes.  There does seem to be a good point in there that the code should be:
> BUG_ON(kobj->name);
> 
> And otherwise simply not handle old_name at all.
> 

Sorry, correct one.

diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..3ab224b 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -215,7 +215,6 @@ static int kobject_add_internal(struct kobject *kobj)
 int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 				  va_list vargs)
 {
-	const char *old_name = kobj->name;
 	char *s;
 
 	if (kobj->name && !fmt)
@@ -229,7 +228,6 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 	while ((s = strchr(kobj->name, '/')))
 		s[0] = '!';
 
-	kfree(old_name);
 	return 0;
 }
 

> Eric
> 

	Sergey

Re: [PATCH] kobject_set_name_vargs memory leak

[Kay Sievers]

On Sat, 2009-06-27 at 12:39 +0300, Sergey Senozhatsky wrote:

> > >> Or something along those lines, I can't remember the exact reasoning
> > >> this early in the morning.
> > >> 
> > >> Kay, do you remember?

Hmm, yes, I think there was only something to work around during the
transition from the static name array, which is gone now. At least I
can't see anything we need to care about with the current code.

> Sorry, correct one.
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index b512b74..3ab224b 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -215,7 +215,6 @@ static int kobject_add_internal(struct kobject *kobj)
>  int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>  				  va_list vargs)
>  {
> -	const char *old_name = kobj->name;

I guess, that would leak an allocated name, when it is set several times
in a row? Something like this?

Thanks,
Kay

diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..780e89f 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -215,21 +215,22 @@ static int kobject_add_internal(struct kobject *kobj)
 int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 				  va_list vargs)
 {
-	const char *old_name = kobj->name;
+	const char *name = kobj->name;
 	char *s;
 
 	if (kobj->name && !fmt)
 		return 0;
 
-	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
-	if (!kobj->name)
+	name = kvasprintf(GFP_KERNEL, fmt, vargs);
+	if (!name)
 		return -ENOMEM;
 
 	/* ewww... some of these buggers have '/' in the name ... */
-	while ((s = strchr(kobj->name, '/')))
+	while ((s = strchr(name, '/')))
 		s[0] = '!';
 
-	kfree(old_name);
+	kfree(kobj->name);
+	kobj->name = name;
 	return 0;
 }
 

Re: [PATCH] kobject_set_name_vargs memory leak

[Eric W. Biederman]

Kay Sievers <kay.sievers@vrfy.org> writes:

> On Sat, 2009-06-27 at 12:39 +0300, Sergey Senozhatsky wrote:
>
>> > >> Or something along those lines, I can't remember the exact reasoning
>> > >> this early in the morning.
>> > >> 
>> > >> Kay, do you remember?
>
> Hmm, yes, I think there was only something to work around during the
> transition from the static name array, which is gone now. At least I
> can't see anything we need to care about with the current code.
>
>> Sorry, correct one.
>> 
>> diff --git a/lib/kobject.c b/lib/kobject.c
>> index b512b74..3ab224b 100644
>> --- a/lib/kobject.c
>> +++ b/lib/kobject.c
>> @@ -215,7 +215,6 @@ static int kobject_add_internal(struct kobject *kobj)
>>  int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>>  				  va_list vargs)
>>  {
>> -	const char *old_name = kobj->name;
>
> I guess, that would leak an allocated name, when it is set several times
> in a row? Something like this?

But setting a kobject's name several times in a row is a bug.  You
need to call kobject_rename if you are going to change the name.

So how about we fix the driver core not to do that.  Stop treating fmt
as a flag, and make it clear kobject_add should not be passed a name.

I really hate DWIM functions there is no maintaining them.

Something like this patch:

 block/blk-sysfs.c          |    6 ++++-
 block/elevator.c           |    4 ++-
 drivers/base/core.c        |   10 ++++++---
 drivers/firmware/memmap.c  |    3 +-
 drivers/md/md.c            |    5 +++-
 drivers/net/iseries_veth.c |    4 +--
 drivers/uio/uio.c          |    9 +++-----
 include/linux/kobject.h    |    3 --
 lib/kobject.c              |   46 ++++++++++++++-------------------------------
 9 files changed, 43 insertions(+), 47 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..92da396 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -215,11 +215,9 @@ static int kobject_add_internal(struct kobject *kobj)
 int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 				  va_list vargs)
 {
-	const char *old_name = kobj->name;
 	char *s;
 
-	if (kobj->name && !fmt)
-		return 0;
+	BUG_ON(kobj->name);
 
 	kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
 	if (!kobj->name)
@@ -229,7 +227,6 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 	while ((s = strchr(kobj->name, '/')))
 		s[0] = '!';
 
-	kfree(old_name);
 	return 0;
 }
 
@@ -296,20 +293,6 @@ error:
 }
 EXPORT_SYMBOL(kobject_init);
 
-static int kobject_add_varg(struct kobject *kobj, struct kobject *parent,
-			    const char *fmt, va_list vargs)
-{
-	int retval;
-
-	retval = kobject_set_name_vargs(kobj, fmt, vargs);
-	if (retval) {
-		printk(KERN_ERR "kobject: can not set name properly!\n");
-		return retval;
-	}
-	kobj->parent = parent;
-	return kobject_add_internal(kobj);
-}
-
 /**
  * kobject_add - the main kobject add function
  * @kobj: the kobject to add
@@ -335,15 +318,14 @@ static int kobject_add_varg(struct kobject *kobj, struct kobject *parent,
  * kobject_uevent() with the UEVENT_ADD parameter to ensure that
  * userspace is properly notified of this kobject's creation.
  */
-int kobject_add(struct kobject *kobj, struct kobject *parent,
-		const char *fmt, ...)
+int kobject_add(struct kobject *kobj, struct kobject *parent)
 {
-	va_list args;
-	int retval;
-
 	if (!kobj)
 		return -EINVAL;
 
+	if (!kobj->name)
+		return -EINVAL;
+
 	if (!kobj->state_initialized) {
 		printk(KERN_ERR "kobject '%s' (%p): tried to add an "
 		       "uninitialized object, something is seriously wrong.\n",
@@ -351,11 +333,8 @@ int kobject_add(struct kobject *kobj, struct kobject *parent,
 		dump_stack();
 		return -EINVAL;
 	}
-	va_start(args, fmt);
-	retval = kobject_add_varg(kobj, parent, fmt, args);
-	va_end(args);
-
-	return retval;
+	kobj->parent = parent;
+	return kobject_add_internal(kobj);
 }
 EXPORT_SYMBOL(kobject_add);
 
@@ -379,9 +358,12 @@ int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype,
 	kobject_init(kobj, ktype);
 
 	va_start(args, fmt);
-	retval = kobject_add_varg(kobj, parent, fmt, args);
+	retval = kobject_set_name(kobj, fmt, args);
 	va_end(args);
 
+	if (!retval)
+		retval = kobject_add(kobj, parent);
+
 	return retval;
 }
 EXPORT_SYMBOL_GPL(kobject_init_and_add);
@@ -653,7 +635,9 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
 	if (!kobj)
 		return NULL;
 
-	retval = kobject_add(kobj, parent, "%s", name);
+	retval = kobject_set_name(kobj, "%s", name);
+	if (!retval)
+		retval = kobject_add(kobj, parent);
 	if (retval) {
 		printk(KERN_WARNING "%s: kobject_add error: %d\n",
 		       __func__, retval);
@@ -798,7 +782,7 @@ static struct kset *kset_create(const char *name,
 	kset = kzalloc(sizeof(*kset), GFP_KERNEL);
 	if (!kset)
 		return NULL;
-	retval = kobject_set_name(&kset->kobj, name);
+	retval = kobject_set_name(&kset->kobj, "%s", name);
 	if (retval) {
 		kfree(kset);
 		return NULL;
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 58ae8e0..de5f5d1 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -83,8 +83,7 @@ static inline const char *kobject_name(const struct kobject *kobj)
 
 extern void kobject_init(struct kobject *kobj, struct kobj_type *ktype);
 extern int __must_check kobject_add(struct kobject *kobj,
-				    struct kobject *parent,
-				    const char *fmt, ...);
+				    struct kobject *parent);
 extern int __must_check kobject_init_and_add(struct kobject *kobj,
 					     struct kobj_type *ktype,
 					     struct kobject *parent,
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index b1cd040..64f7270 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -433,7 +433,11 @@ int blk_register_queue(struct gendisk *disk)
 	if (ret)
 		return ret;
 
-	ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
+	ret = kobj_set_name(&q->kobj, "%s", "queue");
+	if (ret < 0)
+		return ret;
+
+	ret = kobject_add(&q->kobj, kobject_get(&dev->kobj));
 	if (ret < 0)
 		return ret;
 
diff --git a/block/elevator.c b/block/elevator.c
index ca86192..f26dca0 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -903,7 +903,9 @@ int elv_register_queue(struct request_queue *q)
 	struct elevator_queue *e = q->elevator;
 	int error;
 
-	error = kobject_add(&e->kobj, &q->kobj, "%s", "iosched");
+	error = kobject_set_name(&e->kobj, "%s", "iosched");
+	if (!error)
+		error = kobject_add(&e->kobj, &q->kobj);
 	if (!error) {
 		struct elv_fs_entry *attr = e->elevator_type->elevator_attrs;
 		if (attr) {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7ecb193..f0cfa8a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -625,7 +625,12 @@ static struct kobject *get_device_parent(struct device *dev,
 		if (!k)
 			return NULL;
 		k->kset = &dev->class->p->class_dirs;
-		retval = kobject_add(k, parent_kobj, "%s", dev->class->name);
+		retval = kobject_set_name(k, "%s", dev->class->name);
+		if (retval < 0) {
+			kobject_put(k);
+			return NULL;
+		}
+		retval = kobject_add(k, parent_kobj);
 		if (retval < 0) {
 			kobject_put(k);
 			return NULL;
@@ -900,8 +905,7 @@ int device_add(struct device *dev)
 		set_dev_node(dev, dev_to_node(parent));
 
 	/* first, register with generic layer. */
-	/* we require the name to be set before, and pass NULL */
-	error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
+	error = kobject_add(&dev->kobj, dev->kobj.parent);
 	if (error)
 		goto Error;
 
diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
index d5ea8a6..161a165 100644
--- a/drivers/firmware/memmap.c
+++ b/drivers/firmware/memmap.c
@@ -224,7 +224,8 @@ static int __init memmap_init(void)
 
 	list_for_each_entry(entry, &map_entries, list) {
 		entry->kobj.kset = memmap_kset;
-		if (kobject_add(&entry->kobj, NULL, "%d", i++))
+		if (kobject_set_name(&entry->kobj, "%d", i++) ||
+		    kobject_add(&entry->kobj, NULL))
 			kobject_put(&entry->kobj);
 	}
 
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 09be637..2f9fa72 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1575,7 +1575,10 @@ static int bind_rdev_to_array(mdk_rdev_t * rdev, mddev_t * mddev)
 	rdev->mddev = mddev;
 	printk(KERN_INFO "md: bind<%s>\n", b);
 
-	if ((err = kobject_add(&rdev->kobj, &mddev->kobj, "dev-%s", b)))
+	if ((err = kobject_set_name(&rdev->kobj, "dev-%s", b)))
+		goto fail;
+
+	if ((err = kobject_add(&rdev->kobj, &mddev->kobj)))
 		goto fail;
 
 	ko = &part_to_dev(rdev->bdev->bd_part)->kobj;
diff --git a/drivers/net/iseries_veth.c b/drivers/net/iseries_veth.c
index e44215c..a024c24 100644
--- a/drivers/net/iseries_veth.c
+++ b/drivers/net/iseries_veth.c
@@ -1089,8 +1089,8 @@ static struct net_device *veth_probe_one(int vlan,
 		return NULL;
 	}
 
-	kobject_init(&port->kobject, &veth_port_ktype);
-	if (0 != kobject_add(&port->kobject, &dev->dev.kobj, "veth_port"))
+	if (kobject_init_and_add(&port->kobject, &veth_port_ktype,
+				 &dev->dev.kobj, "%s", "veth_port")
 		veth_error("Failed adding port for %s to sysfs.\n", dev->name);
 
 	veth_info("%s attached to iSeries vlan %d (LPAR map = 0x%.4X)\n",
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 03efb06..7c53bb9 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -303,10 +303,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		map = kzalloc(sizeof(*map), GFP_KERNEL);
 		if (!map)
 			goto err_map;
-		kobject_init(&map->kobj, &map_attr_type);
 		map->mem = mem;
 		mem->map = map;
-		ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
+		ret = kobject_init_and_add(&map->kobj, &map_attr_type,
+					   idev->map_dir, "map%d", mi);
 		if (ret)
 			goto err_map;
 		ret = kobject_uevent(&map->kobj, KOBJ_ADD);
@@ -328,11 +328,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		portio = kzalloc(sizeof(*portio), GFP_KERNEL);
 		if (!portio)
 			goto err_portio;
-		kobject_init(&portio->kobj, &portio_attr_type);
 		portio->port = port;
 		port->portio = portio;
-		ret = kobject_add(&portio->kobj, idev->portio_dir,
-							"port%d", pi);
+		ret = kobject_init_and_add(&portio->kobj, &portio_attr_type,
+					   idev->portio_dir, "port%d", pi);
 		if (ret)
 			goto err_portio;
 		ret = kobject_uevent(&portio->kobj, KOBJ_ADD);

Re: [PATCH] kobject_set_name_vargs memory leak

[Kay Sievers]

On Sun, Jun 28, 2009 at 14:07, Eric W. Biederman<ebiederm@xmission.com> wrote:

> But setting a kobject's name several times in a row is a bug.  You
> need to call kobject_rename if you are going to change the name.

Sure, we can define in that way.

> So how about we fix the driver core not to do that.  Stop treating fmt
> as a flag, and make it clear kobject_add should not be passed a name.

Sounds fine to me. You did not try to compile your patch, right? :)

block/blk-sysfs.c: In function ‘blk_register_queue’:
block/blk-sysfs.c:436: error: implicit declaration of function ‘kobj_set_name’

drivers/base/driver.c: In function ‘driver_add_kobj’:
drivers/base/driver.c:149: error: too many arguments to function ‘kobject_add’

Documentation/kobject.txt would also need an update then.

Thanks,
Kay

Re: [PATCH] kobject_set_name_vargs memory leak

[Eric W. Biederman]

Kay Sievers <kay.sievers@vrfy.org> writes:

> On Sun, Jun 28, 2009 at 14:07, Eric W. Biederman<ebiederm@xmission.com> wrote:
>
>> But setting a kobject's name several times in a row is a bug.  You
>> need to call kobject_rename if you are going to change the name.
>
> Sure, we can define in that way.
>
>> So how about we fix the driver core not to do that.  Stop treating fmt
>> as a flag, and make it clear kobject_add should not be passed a name.
>
> Sounds fine to me. You did not try to compile your patch, right? :)

Only the lib/kobject bits...

> block/blk-sysfs.c: In function ‘blk_register_queue’:
> block/blk-sysfs.c:436: error: implicit declaration of function ‘kobj_set_name’
>
> drivers/base/driver.c: In function ‘driver_add_kobj’:
> drivers/base/driver.c:149: error: too many arguments to function ‘kobject_add’

Ugh I totally missed that one.

> Documentation/kobject.txt would also need an update then.

As for the rules it already seems correct.
But getting the prototype and mentioning kobject_set_name wouldn't
hurt.

Eric

Re: [PATCH] kobject_set_name_vargs memory leak

[Sergey Senozhatsky]

On (06/28/09 05:07), Eric W. Biederman wrote:
> Date: Sun, 28 Jun 2009 05:07:10 -0700
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> To: Kay Sievers <kay.sievers@vrfy.org>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@mail.by>,
> 	Greg KH <gregkh@suse.de>, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kobject_set_name_vargs memory leak
> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux)
> 
> Kay Sievers <kay.sievers@vrfy.org> writes:
> 
> > On Sat, 2009-06-27 at 12:39 +0300, Sergey Senozhatsky wrote:
> >
> >> > >> Or something along those lines, I can't remember the exact reasoning
> >> > >> this early in the morning.
> >> > >> 
> >> > >> Kay, do you remember?
> > [....]
> > Hmm, yes, I think there was only something to work around during the
> > transition from the static name array, which is gone now. At least I
> > can't see anything we need to care about with the current code.
> >
> > I guess, that would leak an allocated name, when it is set several times
> > in a row? Something like this?
> 
> But setting a kobject's name several times in a row is a bug.  You
> need to call kobject_rename if you are going to change the name.
> 
Yes.

> So how about we fix the driver core not to do that.  Stop treating fmt
> as a flag, and make it clear kobject_add should not be passed a name.
> 
Sounds good.

	Sergey

Re: [PATCH] kobject_set_name_vargs memory leak

[Dave Young]

On Sun, Jun 28, 2009 at 8:07 PM, Eric W. Biederman<ebiederm@xmission.com> wrote:
> Kay Sievers <kay.sievers@vrfy.org> writes:
>
>> On Sat, 2009-06-27 at 12:39 +0300, Sergey Senozhatsky wrote:
>>
>>> > >> Or something along those lines, I can't remember the exact reasoning
>>> > >> this early in the morning.
>>> > >>
>>> > >> Kay, do you remember?
>>
>> Hmm, yes, I think there was only something to work around during the
>> transition from the static name array, which is gone now. At least I
>> can't see anything we need to care about with the current code.
>>
>>> Sorry, correct one.
>>>
>>> diff --git a/lib/kobject.c b/lib/kobject.c
>>> index b512b74..3ab224b 100644
>>> --- a/lib/kobject.c
>>> +++ b/lib/kobject.c
>>> @@ -215,7 +215,6 @@ static int kobject_add_internal(struct kobject *kobj)
>>>  int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>>>                                va_list vargs)
>>>  {
>>> -    const char *old_name = kobj->name;
>>
>> I guess, that would leak an allocated name, when it is set several times
>> in a row? Something like this?
>
> But setting a kobject's name several times in a row is a bug.  You
> need to call kobject_rename if you are going to change the name.

Agree

>
> So how about we fix the driver core not to do that.  Stop treating fmt
> as a flag, and make it clear kobject_add should not be passed a name.
>
> I really hate DWIM functions there is no maintaining them.

How about set name in kobject_init? or another kobject_init_with_name?

>
> Something like this patch:
>
>  block/blk-sysfs.c          |    6 ++++-
>  block/elevator.c           |    4 ++-
>  drivers/base/core.c        |   10 ++++++---
>  drivers/firmware/memmap.c  |    3 +-
>  drivers/md/md.c            |    5 +++-
>  drivers/net/iseries_veth.c |    4 +--
>  drivers/uio/uio.c          |    9 +++-----
>  include/linux/kobject.h    |    3 --
>  lib/kobject.c              |   46 ++++++++++++++-------------------------------
>  9 files changed, 43 insertions(+), 47 deletions(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index b512b74..92da396 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -215,11 +215,9 @@ static int kobject_add_internal(struct kobject *kobj)
>  int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>                                  va_list vargs)
>  {
> -       const char *old_name = kobj->name;
>        char *s;
>
> -       if (kobj->name && !fmt)
> -               return 0;
> +       BUG_ON(kobj->name);
>
>        kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
>        if (!kobj->name)
> @@ -229,7 +227,6 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>        while ((s = strchr(kobj->name, '/')))
>                s[0] = '!';
>
> -       kfree(old_name);
>        return 0;
>  }
>
> @@ -296,20 +293,6 @@ error:
>  }
>  EXPORT_SYMBOL(kobject_init);
>
> -static int kobject_add_varg(struct kobject *kobj, struct kobject *parent,
> -                           const char *fmt, va_list vargs)
> -{
> -       int retval;
> -
> -       retval = kobject_set_name_vargs(kobj, fmt, vargs);
> -       if (retval) {
> -               printk(KERN_ERR "kobject: can not set name properly!\n");
> -               return retval;
> -       }
> -       kobj->parent = parent;
> -       return kobject_add_internal(kobj);
> -}
> -
>  /**
>  * kobject_add - the main kobject add function
>  * @kobj: the kobject to add
> @@ -335,15 +318,14 @@ static int kobject_add_varg(struct kobject *kobj, struct kobject *parent,
>  * kobject_uevent() with the UEVENT_ADD parameter to ensure that
>  * userspace is properly notified of this kobject's creation.
>  */
> -int kobject_add(struct kobject *kobj, struct kobject *parent,
> -               const char *fmt, ...)
> +int kobject_add(struct kobject *kobj, struct kobject *parent)
>  {
> -       va_list args;
> -       int retval;
> -
>        if (!kobj)
>                return -EINVAL;
>
> +       if (!kobj->name)
> +               return -EINVAL;
> +
>        if (!kobj->state_initialized) {
>                printk(KERN_ERR "kobject '%s' (%p): tried to add an "
>                       "uninitialized object, something is seriously wrong.\n",
> @@ -351,11 +333,8 @@ int kobject_add(struct kobject *kobj, struct kobject *parent,
>                dump_stack();
>                return -EINVAL;
>        }
> -       va_start(args, fmt);
> -       retval = kobject_add_varg(kobj, parent, fmt, args);
> -       va_end(args);
> -
> -       return retval;
> +       kobj->parent = parent;
> +       return kobject_add_internal(kobj);
>  }
>  EXPORT_SYMBOL(kobject_add);
>
> @@ -379,9 +358,12 @@ int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype,
>        kobject_init(kobj, ktype);
>
>        va_start(args, fmt);
> -       retval = kobject_add_varg(kobj, parent, fmt, args);
> +       retval = kobject_set_name(kobj, fmt, args);
>        va_end(args);
>
> +       if (!retval)
> +               retval = kobject_add(kobj, parent);
> +
>        return retval;
>  }
>  EXPORT_SYMBOL_GPL(kobject_init_and_add);
> @@ -653,7 +635,9 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
>        if (!kobj)
>                return NULL;
>
> -       retval = kobject_add(kobj, parent, "%s", name);
> +       retval = kobject_set_name(kobj, "%s", name);
> +       if (!retval)
> +               retval = kobject_add(kobj, parent);
>        if (retval) {
>                printk(KERN_WARNING "%s: kobject_add error: %d\n",
>                       __func__, retval);
> @@ -798,7 +782,7 @@ static struct kset *kset_create(const char *name,
>        kset = kzalloc(sizeof(*kset), GFP_KERNEL);
>        if (!kset)
>                return NULL;
> -       retval = kobject_set_name(&kset->kobj, name);
> +       retval = kobject_set_name(&kset->kobj, "%s", name);
>        if (retval) {
>                kfree(kset);
>                return NULL;
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index 58ae8e0..de5f5d1 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -83,8 +83,7 @@ static inline const char *kobject_name(const struct kobject *kobj)
>
>  extern void kobject_init(struct kobject *kobj, struct kobj_type *ktype);
>  extern int __must_check kobject_add(struct kobject *kobj,
> -                                   struct kobject *parent,
> -                                   const char *fmt, ...);
> +                                   struct kobject *parent);
>  extern int __must_check kobject_init_and_add(struct kobject *kobj,
>                                             struct kobj_type *ktype,
>                                             struct kobject *parent,
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index b1cd040..64f7270 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -433,7 +433,11 @@ int blk_register_queue(struct gendisk *disk)
>        if (ret)
>                return ret;
>
> -       ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
> +       ret = kobj_set_name(&q->kobj, "%s", "queue");
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = kobject_add(&q->kobj, kobject_get(&dev->kobj));
>        if (ret < 0)
>                return ret;
>
> diff --git a/block/elevator.c b/block/elevator.c
> index ca86192..f26dca0 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -903,7 +903,9 @@ int elv_register_queue(struct request_queue *q)
>        struct elevator_queue *e = q->elevator;
>        int error;
>
> -       error = kobject_add(&e->kobj, &q->kobj, "%s", "iosched");
> +       error = kobject_set_name(&e->kobj, "%s", "iosched");
> +       if (!error)
> +               error = kobject_add(&e->kobj, &q->kobj);
>        if (!error) {
>                struct elv_fs_entry *attr = e->elevator_type->elevator_attrs;
>                if (attr) {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 7ecb193..f0cfa8a 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -625,7 +625,12 @@ static struct kobject *get_device_parent(struct device *dev,
>                if (!k)
>                        return NULL;
>                k->kset = &dev->class->p->class_dirs;
> -               retval = kobject_add(k, parent_kobj, "%s", dev->class->name);
> +               retval = kobject_set_name(k, "%s", dev->class->name);
> +               if (retval < 0) {
> +                       kobject_put(k);
> +                       return NULL;
> +               }
> +               retval = kobject_add(k, parent_kobj);
>                if (retval < 0) {
>                        kobject_put(k);
>                        return NULL;
> @@ -900,8 +905,7 @@ int device_add(struct device *dev)
>                set_dev_node(dev, dev_to_node(parent));
>
>        /* first, register with generic layer. */
> -       /* we require the name to be set before, and pass NULL */
> -       error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
> +       error = kobject_add(&dev->kobj, dev->kobj.parent);
>        if (error)
>                goto Error;
>
> diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
> index d5ea8a6..161a165 100644
> --- a/drivers/firmware/memmap.c
> +++ b/drivers/firmware/memmap.c
> @@ -224,7 +224,8 @@ static int __init memmap_init(void)
>
>        list_for_each_entry(entry, &map_entries, list) {
>                entry->kobj.kset = memmap_kset;
> -               if (kobject_add(&entry->kobj, NULL, "%d", i++))
> +               if (kobject_set_name(&entry->kobj, "%d", i++) ||
> +                   kobject_add(&entry->kobj, NULL))
>                        kobject_put(&entry->kobj);
>        }
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 09be637..2f9fa72 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1575,7 +1575,10 @@ static int bind_rdev_to_array(mdk_rdev_t * rdev, mddev_t * mddev)
>        rdev->mddev = mddev;
>        printk(KERN_INFO "md: bind<%s>\n", b);
>
> -       if ((err = kobject_add(&rdev->kobj, &mddev->kobj, "dev-%s", b)))
> +       if ((err = kobject_set_name(&rdev->kobj, "dev-%s", b)))
> +               goto fail;
> +
> +       if ((err = kobject_add(&rdev->kobj, &mddev->kobj)))
>                goto fail;
>
>        ko = &part_to_dev(rdev->bdev->bd_part)->kobj;
> diff --git a/drivers/net/iseries_veth.c b/drivers/net/iseries_veth.c
> index e44215c..a024c24 100644
> --- a/drivers/net/iseries_veth.c
> +++ b/drivers/net/iseries_veth.c
> @@ -1089,8 +1089,8 @@ static struct net_device *veth_probe_one(int vlan,
>                return NULL;
>        }
>
> -       kobject_init(&port->kobject, &veth_port_ktype);
> -       if (0 != kobject_add(&port->kobject, &dev->dev.kobj, "veth_port"))
> +       if (kobject_init_and_add(&port->kobject, &veth_port_ktype,
> +                                &dev->dev.kobj, "%s", "veth_port")
>                veth_error("Failed adding port for %s to sysfs.\n", dev->name);
>
>        veth_info("%s attached to iSeries vlan %d (LPAR map = 0x%.4X)\n",
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 03efb06..7c53bb9 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -303,10 +303,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>                map = kzalloc(sizeof(*map), GFP_KERNEL);
>                if (!map)
>                        goto err_map;
> -               kobject_init(&map->kobj, &map_attr_type);
>                map->mem = mem;
>                mem->map = map;
> -               ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
> +               ret = kobject_init_and_add(&map->kobj, &map_attr_type,
> +                                          idev->map_dir, "map%d", mi);
>                if (ret)
>                        goto err_map;
>                ret = kobject_uevent(&map->kobj, KOBJ_ADD);
> @@ -328,11 +328,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>                portio = kzalloc(sizeof(*portio), GFP_KERNEL);
>                if (!portio)
>                        goto err_portio;
> -               kobject_init(&portio->kobj, &portio_attr_type);
>                portio->port = port;
>                port->portio = portio;
> -               ret = kobject_add(&portio->kobj, idev->portio_dir,
> -                                                       "port%d", pi);
> +               ret = kobject_init_and_add(&portio->kobj, &portio_attr_type,
> +                                          idev->portio_dir, "port%d", pi);
>                if (ret)
>                        goto err_portio;
>                ret = kobject_uevent(&portio->kobj, KOBJ_ADD);
> --
> 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/
>



-- 
Regards
dave