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

[Ming Lei <tom.leiming@gmail.com>]

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