[PATCH] lib/kobject: put kobject if kobject_add_internal fails

[PATCH] lib/kobject: put kobject if kobject_add_internal fails

[Xiaotian Feng]

The proper way to use kobject_init_and_add should be:

        retval = kobject_init_and_add(&foo->kobj, &foo_ktype, NULL, "%s", name);
        if (retval) {
                kobject_put(&foo->kobj); (*)
                return NULL;
        }

kobject_init_and_add calls kobject_add_vargs finally, kobject_add_vargs is divided
into two parts: kobject_set_name_vargs and kobject_add_internal. Both the two parts
may return an error. If the error is came from kobject_add_internal, this means
kobject_set_name_vargs already alloc memory for kobj->name.

So if caller forgets to use kobject_put when this kind of error occurs, the memory
for kobj->name leaks. Unfortunately, most of the callers is forgotten to use
kobject_put in this rare situation, grep kobject_init_and_add in kernel source code,
there are 20+ files forgotten this. So I'd prefer to fix this in lib/kobject, not
the whole 20+ files.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
---
 lib/kobject.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index b512b74..e6e6e03 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -200,6 +200,7 @@ static int kobject_add_internal(struct kobject *kobj)
 			printk(KERN_ERR "%s failed for %s (%d)\n",
 			       __func__, kobject_name(kobj), error);
 		dump_stack();
+		kobject_put(kobj);
 	} else
 		kobj->state_in_sysfs = 1;
 
@@ -657,7 +658,6 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
 	if (retval) {
 		printk(KERN_WARNING "%s: kobject_add error: %d\n",
 		       __func__, retval);
-		kobject_put(kobj);
 		kobj = NULL;
 	}
 	return kobj;
-- 
1.6.2.5

Re: [PATCH] lib/kobject: put kobject if kobject_add_internal fails

[Ming Lei]

2009/7/24 Xiaotian Feng <dfeng@redhat.com>:
> The proper way to use kobject_init_and_add should be:
>
>        retval = kobject_init_and_add(&foo->kobj, &foo_ktype, NULL, "%s", name);
>        if (retval) {
>                kobject_put(&foo->kobj); (*)
>                return NULL;
>        }
>

Yes, you are correct.

> kobject_init_and_add calls kobject_add_vargs finally, kobject_add_vargs is divided
> into two parts: kobject_set_name_vargs and kobject_add_internal. Both the two parts
> may return an error. If the error is came from kobject_add_internal, this means
> kobject_set_name_vargs already alloc memory for kobj->name.
>
> So if caller forgets to use kobject_put when this kind of error occurs, the memory
> for kobj->name leaks. Unfortunately, most of the callers is forgotten to use
> kobject_put in this rare situation, grep kobject_init_and_add in kernel source code,
> there are 20+ files forgotten this. So I'd prefer to fix this in lib/kobject, not
> the whole 20+ files.

No, you should fix the 20+ files instead of lib/kobject. One rule should be:

             One who allocated kobject should free the kobject,
instead of others.

Image you have allocated a object, and call some .init function to
initialize it,
but .init frees the object due to some exception, it is very ugly and very prone
to access of the freed object.

Your patch may lead to much oops if the drivers use the proper way of
kobject_init_and_add:

        retval = kobject_init_and_add(&foo->kobj, &foo_ktype, NULL, "%s", name);
        if (retval) {
                kobject_put(&foo->kobj);      /*foo->kobj has been freed, oops*/
                return NULL;
        }

Right?

>
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> ---
>  lib/kobject.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index b512b74..e6e6e03 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -200,6 +200,7 @@ static int kobject_add_internal(struct kobject *kobj)
>                        printk(KERN_ERR "%s failed for %s (%d)\n",
>                               __func__, kobject_name(kobj), error);
>                dump_stack();
> +               kobject_put(kobj);
>        } else
>                kobj->state_in_sysfs = 1;
>
> @@ -657,7 +658,6 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
>        if (retval) {
>                printk(KERN_WARNING "%s: kobject_add error: %d\n",
>                       __func__, retval);
> -               kobject_put(kobj);
>                kobj = NULL;
>        }
>        return kobj;

NAK.

-- 
Lei Ming

Re: [PATCH] lib/kobject: put kobject if kobject_add_internal fails

[Ming Lei]

2009/7/25 Xiaotian Feng <xtfeng@gmail.com>:
>
>
> On Sat, Jul 25, 2009 at 11:38 AM, Ming Lei <tom.leiming@gmail.com> wrote:
>>
>> 2009/7/24 Xiaotian Feng <dfeng@redhat.com>:
>> > The proper way to use kobject_init_and_add should be:
>> >
>> >        retval = kobject_init_and_add(&foo->kobj, &foo_ktype, NULL, "%s",
>> > name);
>> >        if (retval) {
>> >                kobject_put(&foo->kobj); (*)
>> >                return NULL;
>> >        }
>> >
>>
>> Yes, you are correct.
>>
>> > kobject_init_and_add calls kobject_add_vargs finally, kobject_add_vargs
>> > is divided
>> > into two parts: kobject_set_name_vargs and kobject_add_internal. Both
>> > the two parts
>> > may return an error. If the error is came from kobject_add_internal,
>> > this means
>> > kobject_set_name_vargs already alloc memory for kobj->name.
>> >
>> > So if caller forgets to use kobject_put when this kind of error occurs,
>> > the memory
>> > for kobj->name leaks. Unfortunately, most of the callers is forgotten to
>> > use
>> > kobject_put in this rare situation, grep kobject_init_and_add in kernel
>> > source code,
>> > there are 20+ files forgotten this. So I'd prefer to fix this in
>> > lib/kobject, not
>> > the whole 20+ files.
>>
>> No, you should fix the 20+ files instead of lib/kobject. One rule should
>> be:
>>
>>             One who allocated kobject should free the kobject,
>> instead of others.
>>
>> Image you have allocated a object, and call some .init function to
>> initialize it,
>> but .init frees the object due to some exception, it is very ugly and very
>> prone
>> to access of the freed object.
>>
>> Your patch may lead to much oops if the drivers use the proper way of
>> kobject_init_and_add:
>>
>>        retval = kobject_init_and_add(&foo->kobj, &foo_ktype, NULL, "%s",
>> name);
>>        if (retval) {
>>                kobject_put(&foo->kobj);      /*foo->kobj has been freed,
>> oops*/
>>                return NULL;
>>        }
>>
>> Right?
>
>
> No, take a look at kobject_put code, you will see if foo->kobj is NULL,
> kobject_put will do nothing.
> So it's safe for double kobject_put on the register failed path.

You are wrong, foo->kobj  is __not__ NULL.

Can the kobject_init_and_add update the passed pointer of foo->kobj?

No, it can not. Even kobject_init_and_add returns failure, the pointer
of kobject passed does not change, so kobject_put() still can see the
old pointer.

Thanks.
-- 
Lei Ming

Re: [PATCH] lib/kobject: put kobject if kobject_add_internal fails

[Ming Lei]

2009/7/25 Xiaotian Feng <xtfeng@gmail.com>:
>> No, you should fix the 20+ files instead of lib/kobject. One rule should
>> be:
>>
>>             One who allocated kobject should free the kobject,
>> instead of others.
>
>
> kobject_put is not to free the kobject, it's to cleanup the resources
> allocated by kobject_init/add/....
> After kobject_put, we just have a cleanup kobject. The freed action is taken
> by kfree called by who uses kobject,

kobject_put may (but not must) free the kobject, which depends on the kobj_type
passed to  kobject_init_and_add. The .release of the default
kobj_type(dynamic_kobj_ktype) will free the kobject and kobject_put will call
.release.

Even you can make sure _all_ .release of the passed kobj_type does not free
the kobject, you still have the double calling of kobject_put problem,
don't you?

Thanks.

-- 
Lei Ming

Re: [PATCH] lib/kobject: put kobject if kobject_add_internal fails

[Ming Lei]

2009/7/25 Xiaotian Feng <xtfeng@gmail.com>:
>
>
> On Sat, Jul 25, 2009 at 2:52 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> Even you can make sure _all_ .release of the passed kobj_type does not
>> free
>> the kobject, you still have the double calling of kobject_put problem,
>> don't you?
>
>
> hmm, how about change kobject_put to kfree(kobj->name) ? Then it's safe for
> both cases, right?

No, it is very buggy and ugly to kfree kobj->name directly by kobject
users instead
of kobject_put().  Image that someone uses the kobject after kfree(kobj->name)
but before kobject_put,  oops may happen.

Why don't you fix the 20+ callers of  kobject_init_and_add?  It is the
standard way of doing
such thing.

-- 
Lei Ming