On Wed 2019-05-01 09:38:03, Tobin C. Harding wrote:
> Hi,
>
> Looks like I've created a bit of confusion trying to fix memleaks in
> calls to kobject_init_and_add(). Its spread over various patches and
> mailing lists so I'm starting a new thread and CC'ing anyone that
> commented on one of those patches.
>
> If there is a better way to go about this discussion please do tell me.
>
> The problem
> -----------
>
> Calls to kobject_init_and_add() are leaking memory throughout the kernel
> because of how the error paths are handled.
>
> The solution
> ------------
>
> Write the error path code correctly.
>
> Example
> -------
>
> We have samples/kobject/kobject-example.c but it uses
> kobject_create_and_add(). I thought of adding another example file here
> but could not think of how to do it off the top of my head without being
> super contrived. Can add this to the TODO list if it will help.
>
> Here is an attempted canonical usage of kobject_init_and_add() typical
> of the code that currently is getting it wrong. This is the second time
> I've written this and the first time it was wrong even after review (you
> know who you are, you are definitely buying the next round of drinks :)
>
>
> Assumes we have an object in memory already that has the kobject
> embedded in it. Variable 'kobj' below would typically be &ptr->kobj
>
>
> void fn(void)
> {
> int ret;
>
> ret = kobject_init_and_add(kobj, ktype, NULL, "foo");
> if (ret) {
> /*
> * This means kobject_init() has succeeded
> * but kobject_add() failed.
> */
> goto err_put;
> }
It is strange to make the structure visible in sysfs before
we initialize it.
> ret = some_init_fn();
> if (ret) {
> /*
> * We need to wind back kobject_add() AND kobject_put().
> * kobject_add() incremented the refcount in
> * kobj->parent, that needs to be decremented THEN we
> need
> * the call to kobject_put() to decrement the
> * refcount of kobj.
*/
> goto err_del;
> }
>
> ret = some_other_init_fn();
> if (ret)
> goto other_err;
>
> kobject_uevent(kobj, KOBJ_ADD);
> return 0;
>
> other_err:
> other_clean_up_fn();
> err_del:
> kobject_del(kobj);
> err_put:
> kobject_put(kobj);
IMHO, separate kobject_del() makes only sense when the sysfs
interface must be destroyed before some other actions.
I guess that we need two examples. I currently understand
it the following way:
1. sysfs interface and the structure can be freed anytime:
struct A
{
struct kobject kobj;
...
};
void fn(void)
{
struct A *a;
int ret;
a = kzalloc(sizeof(*a), GFP_KERNEL);
if (!a)
return;
/*
* Initialize structure before we make it accessible via
* sysfs.
*/
ret = some_init_fn();
if (ret) {
goto init_err;
}
ret = kobject_init_and_add(&a->kobj, ktype, NULL, "foo");
if (ret)
goto kobj_err;
return 0;
kobj_err:
/* kobject_init() always succeds and take reference. */
kobject_put(kobj);
return ret;
init_err:
/* kobject was not initialized, simple free is enough */
kfree(a);
return ret;
}
2. Structure must be registered into the subsystem before
it can be made visible via sysfs:
struct A
{
struct kobject kobj;
...
};
void fn(void)
{
struct A *a;
int ret;
a = kzalloc(sizeof(*a), GFP_KERNEL);
if (!a)
return;
ret = some_init_fn();
if (ret) {
goto init_err;
}
/*
* Structure is in a reasonable state and can be freed
* via the kobject release callback.
*/
kobject_init(&a->kobj);
/*
* Register the structure so that it can cooperate
* with the rest of the system.
*/
ret = register_fn(a);
` if (ret)
goto register_err;
/* Make it visible via sysfs */
ret = kobject_add(&a->kobj, ktype, NULL, "foo");
if (ret) {
goto kobj_add_err;
}
/* Manipulate the structure somehow */
ret = action_fn(a);
if (ret)
goto action_err;
mutex_unlock(&my_mutex);
return 0;
action_err:
/*
* Destroy sysfs interface but the structure
* is still needed.
*/
kobject_del(&a->kboject);
kobject_add_err:
/* Make it invisible to the system. */
unregister_fn(a);
register_err:
/* Release the structure unsing the kobject callback */
kobject_put(&a->kobj);
return;
init_err:
/*
* Custom init failed. Kobject release callback would do
* a double free or so. Simple free is enough.
*/
kfree(a);
}
I would really prefer if we clearly understand where each variant makes
sense before we start modifying the code and documentation.
Best Regards,
Petr
