kobject_init_and_add() confusion

kobject_init_and_add() confusion

[Tobin C. Harding]

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;
		}

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

	        return ret;
	}


Have I got this correct?

TODO
----

- Fix all the callsites to kobject_init_and_add()
- Further clarify the function docstring for kobject_init_and_add() [perhaps]
- Add a section to Documentation/kobject.txt [optional]
- Add a sample usage file under samples/kobject [optional]


Thanks,
Tobin.

Re: kobject_init_and_add() confusion

[Rafael J. Wysocki]

On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@tobin.cc> 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;
>                 }
>
>                 ret = some_init_fn();
>                 if (ret) {
>                         /*
>                          * We need to wind back kobject_add() AND kobject_put().

kobject_add() and kobject_init() I suppose?

>                          * 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.
>                          */

So actually, if you look at kobject_cleanup(), it calls kobject_del()
if kobj->state_in_sysfs is set.

Now, if you look at kobject_add_internal(), it sets
kobj->state_in_sysfs when about to return 0 (success).

Therefore calling kobject_put() without the preceding kobject_del() is
not a bug technically, even though it will trigger the "auto cleanup
kobject_del" message with debug enabled.

>                         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);
>
>                 return ret;
>         }
>
>
> Have I got this correct?
>
> TODO
> ----
>
> - Fix all the callsites to kobject_init_and_add()
> - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> - Add a section to Documentation/kobject.txt [optional]
> - Add a sample usage file under samples/kobject [optional]

The plan sounds good to me, but there is one thing to note IMO:
kobject_cleanup() invokes the ->release() callback for the ktype, so
these callbacks need to be able to cope with kobjects after a failing
kobject_add() which may not be entirely obvious to developers
introducing them ATM.

Thanks,
Rafael

Re: kobject_init_and_add() confusion

[Greg Kroah-Hartman]

On Wed, May 01, 2019 at 09:38:03AM +1000, 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.

s/are leaking/have the potential to leak/

Note, no one ever hits these error paths, so it isn't a big issue, and
is why no one has seen this except for the use of syzbot at times.

> 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.

You could take the example I wrote in that old email and use it, or your
version below as well.

> 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

kobject_init() can not fail except in fun ways that dumps the stack and
then keeps on going due to the failure being on the caller, not the
kobject code itself.

> 			 * but kobject_add() failed.
> 			 */
> 			goto err_put;
> 		}
> 
> 	        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);
> 
> 	        return ret;
> 	}
> 
> 
> Have I got this correct?

From what I can tell, yes.

> TODO
> ----
> 
> - Fix all the callsites to kobject_init_and_add()
> - Further clarify the function docstring for kobject_init_and_add() [perhaps]

More documentation, sure!

> - Add a section to Documentation/kobject.txt [optional]

That file should probably be reviewed and converted to .rst, I haven't
looked at it in years.

> - Add a sample usage file under samples/kobject [optional]

Would be a good idea, so we can point people at it.

thanks,

greg k-h

Re: kobject_init_and_add() confusion

[Tobin C. Harding]

On Wed, May 01, 2019 at 09:54:16AM +0200, Rafael J. Wysocki wrote:
> On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@tobin.cc> 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;
> >                 }
> >
> >                 ret = some_init_fn();
> >                 if (ret) {
> >                         /*
> >                          * We need to wind back kobject_add() AND kobject_put().
> 
> kobject_add() and kobject_init() I suppose?

You are correct, my mistake.

> >                          * 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.
> >                          */
> 
> So actually, if you look at kobject_cleanup(), it calls kobject_del()
> if kobj->state_in_sysfs is set.
> 
> Now, if you look at kobject_add_internal(), it sets
> kobj->state_in_sysfs when about to return 0 (success).
> 
> Therefore calling kobject_put() without the preceding kobject_del() is
> not a bug technically, even though it will trigger the "auto cleanup
> kobject_del" message with debug enabled.

Thanks for this explanation.  Points noted.

> 
> >                         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);
> >
> >                 return ret;
> >         }
> >
> >
> > Have I got this correct?
> >
> > TODO
> > ----
> >
> > - Fix all the callsites to kobject_init_and_add()
> > - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> > - Add a section to Documentation/kobject.txt [optional]
> > - Add a sample usage file under samples/kobject [optional]
> 
> The plan sounds good to me, but there is one thing to note IMO:
> kobject_cleanup() invokes the ->release() callback for the ktype, so
> these callbacks need to be able to cope with kobjects after a failing
> kobject_add() which may not be entirely obvious to developers
> introducing them ATM.

During docs fixes I'll try to work this in.

Thanks,
Tobin.

Re: kobject_init_and_add() confusion

[Tobin C. Harding]

On Wed, May 01, 2019 at 01:10:22PM +0200, Greg Kroah-Hartman wrote:
> On Wed, May 01, 2019 at 09:38:03AM +1000, 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.
> 
> s/are leaking/have the potential to leak/
> 
> Note, no one ever hits these error paths, so it isn't a big issue, and
> is why no one has seen this except for the use of syzbot at times.

One day I'll find an important issue to fix in the kernel.  At the
moment sweeping these up is good practice/learning.  If you have any
_real_ issues that need someone to turn the crank on feel free to dump
them on me :)

> > 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.
> 
> You could take the example I wrote in that old email and use it, or your
> version below as well.

Responded just now to that email.

> 
> > 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
> 
> kobject_init() can not fail except in fun ways that dumps the stack and
> then keeps on going due to the failure being on the caller, not the
> kobject code itself.

Cheers, writing good documentation is HARD.

> > 			 * but kobject_add() failed.
> > 			 */
> > 			goto err_put;
> > 		}
> > 
> > 	        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);
> > 
> > 	        return ret;
> > 	}
> > 
> > 
> > Have I got this correct?
> 
> From what I can tell, yes.

:)

> > TODO
> > ----
> > 
> > - Fix all the callsites to kobject_init_and_add()
> > - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> 
> More documentation, sure!
> 
> > - Add a section to Documentation/kobject.txt [optional]
> 
> That file should probably be reviewed and converted to .rst, I haven't
> looked at it in years.

On my TODO list once I get kobject usage clear in my head.

> > - Add a sample usage file under samples/kobject [optional]
> 
> Would be a good idea, so we can point people at it.

I'll combine your other email example with the extra init/error stuff
from this one and BOOM!

Thanks Greg.

	Tobin

Re: kobject_init_and_add() confusion

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 07:58:58AM +1000, Tobin C. Harding wrote:
> On Wed, May 01, 2019 at 01:10:22PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, May 01, 2019 at 09:38:03AM +1000, 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.
> > 
> > s/are leaking/have the potential to leak/
> > 
> > Note, no one ever hits these error paths, so it isn't a big issue, and
> > is why no one has seen this except for the use of syzbot at times.
> 
> One day I'll find an important issue to fix in the kernel.  At the
> moment sweeping these up is good practice/learning.  If you have any
> _real_ issues that need someone to turn the crank on feel free to dump
> them on me :)

Once you get this done, I do have some "fun" ideas about the cdev api
and how it can be "fixed up".

Your knowledge of reference counts and kobjects will come in handy
there, so talk to me off-list when you are ready :)

keep up the great work,

greg k-h

Re: kobject_init_and_add() confusion

[Petr Mladek]

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

Re: kobject_init_and_add() confusion

[Greg Kroah-Hartman]

On Thu, May 02, 2019 at 10:34:12AM +0200, Petr Mladek wrote:
> 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.

Yes, that is not a good patern, some_init_fn() should happen first.

> > 	        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.

Yes, kobject_del() should not be used unless you really know what you
are doing.

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


need to unwind some_init_fn() here too.

> 		/* 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;
> 	}


Yes.

> 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.

The second variant is much more rare (or at least it should be), but
your example is a good one.

thanks,

greg k-h

Re: kobject_init_and_add() confusion

[Tobin C. Harding]

On Thu, May 02, 2019 at 10:34:12AM +0200, Petr Mladek wrote:
> 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.

Hi Petr,

Shall we work these two examples into samples/kobject/.  I'm AFK now for
the rest of the week but I can do it on Monday if you don't mind me
doing it?

Cheers,
Tobin.

Re: kobject_init_and_add() confusion

[Petr Mladek]

On Fri 2019-05-03 11:16:26, Tobin C. Harding wrote:
> On Thu, May 02, 2019 at 10:34:12AM +0200, Petr Mladek wrote:
> > On Wed 2019-05-01 09:38:03, Tobin C. Harding wrote:
> > 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.
> 
> Hi Petr,
> 
> Shall we work these two examples into samples/kobject/.  I'm AFK now for
> the rest of the week but I can do it on Monday if you don't mind me
> doing it?

Sounds good to me. The current samples/kobject/kobject-example shows
the most simple case when the kobject is standalone. While the
above samples shows how to have it bundled in a bigger structure.

Thanks,
Petr

Re: kobject_init_and_add() confusion

[Tobin C. Harding]

On Wed, May 01, 2019 at 09:54:16AM +0200, Rafael J. Wysocki wrote:
> On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@tobin.cc> 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;
> >                 }
> >
> >                 ret = some_init_fn();
> >                 if (ret) {
> >                         /*
> >                          * We need to wind back kobject_add() AND kobject_put().
> 
> kobject_add() and kobject_init() I suppose?
> 
> >                          * 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.
> >                          */
> 
> So actually, if you look at kobject_cleanup(), it calls kobject_del()
> if kobj->state_in_sysfs is set.
> 
> Now, if you look at kobject_add_internal(), it sets
> kobj->state_in_sysfs when about to return 0 (success).
> 
> Therefore calling kobject_put() without the preceding kobject_del() is
> not a bug technically, even though it will trigger the "auto cleanup
> kobject_del" message with debug enabled.
> 
> >                         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);
> >
> >                 return ret;
> >         }
> >
> >
> > Have I got this correct?
> >
> > TODO
> > ----
> >
> > - Fix all the callsites to kobject_init_and_add()
> > - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> > - Add a section to Documentation/kobject.txt [optional]
> > - Add a sample usage file under samples/kobject [optional]
> 
> The plan sounds good to me, but there is one thing to note IMO:
> kobject_cleanup() invokes the ->release() callback for the ktype, so
> these callbacks need to be able to cope with kobjects after a failing
> kobject_add() which may not be entirely obvious to developers
> introducing them ATM.

It has taken a while for this to soak in.  This is actually quite an
insidious issue.  If I give an example and perhaps we can come to a
solution.  This example is based on the code (and assumptions) in
mm/slub.c

If a developer has an object that they wish to add to sysfs they go
ahead and embed a kobject in it.  Correctly set up a ktype including
release function that just frees the object (using container of).  Now
assume that the object is already set up and in use when we go to set up
the sysfs entry.  If kobject_init_and_add() fails and we correctly call
kobject_put() the containing object will be free'd.  Yet the calling
code may not be done with the object, more to the point just because
sysfs setup fails the object is now unusable.  Besides the interesting
theoretical discussion this means we cannot just go and willy-nilly add
calls to kobject_put() in the error path of kobject_init_and_add() if
the original code was not written under the assumption that the release
method could be called during the error path (I have found 2 places at
least where behaviour of calling the release method is non-trivial to
ascertain).

I guess, as Greg said, its just a matter that reference counting within
the kernel is a hard problem.  So we fix the easy ones and then look a
bit harder at the hard ones ...

Any better suggestion?

thanks,
Tobin.

Re: kobject_init_and_add() confusion

[Rafael J. Wysocki]

On Fri, May 10, 2019 at 4:36 AM Tobin C. Harding <me@tobin.cc> wrote:
>
> On Wed, May 01, 2019 at 09:54:16AM +0200, Rafael J. Wysocki wrote:
> > On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@tobin.cc> 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;
> > >                 }
> > >
> > >                 ret = some_init_fn();
> > >                 if (ret) {
> > >                         /*
> > >                          * We need to wind back kobject_add() AND kobject_put().
> >
> > kobject_add() and kobject_init() I suppose?
> >
> > >                          * 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.
> > >                          */
> >
> > So actually, if you look at kobject_cleanup(), it calls kobject_del()
> > if kobj->state_in_sysfs is set.
> >
> > Now, if you look at kobject_add_internal(), it sets
> > kobj->state_in_sysfs when about to return 0 (success).
> >
> > Therefore calling kobject_put() without the preceding kobject_del() is
> > not a bug technically, even though it will trigger the "auto cleanup
> > kobject_del" message with debug enabled.
> >
> > >                         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);
> > >
> > >                 return ret;
> > >         }
> > >
> > >
> > > Have I got this correct?
> > >
> > > TODO
> > > ----
> > >
> > > - Fix all the callsites to kobject_init_and_add()
> > > - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> > > - Add a section to Documentation/kobject.txt [optional]
> > > - Add a sample usage file under samples/kobject [optional]
> >
> > The plan sounds good to me, but there is one thing to note IMO:
> > kobject_cleanup() invokes the ->release() callback for the ktype, so
> > these callbacks need to be able to cope with kobjects after a failing
> > kobject_add() which may not be entirely obvious to developers
> > introducing them ATM.
>
> It has taken a while for this to soak in.  This is actually quite an
> insidious issue.  If I give an example and perhaps we can come to a
> solution.  This example is based on the code (and assumptions) in
> mm/slub.c
>
> If a developer has an object that they wish to add to sysfs they go
> ahead and embed a kobject in it.  Correctly set up a ktype including
> release function that just frees the object (using container of).  Now
> assume that the object is already set up and in use when we go to set up
> the sysfs entry.  If kobject_init_and_add() fails and we correctly call
> kobject_put() the containing object will be free'd.  Yet the calling
> code may not be done with the object, more to the point just because
> sysfs setup fails the object is now unusable.  Besides the interesting
> theoretical discussion this means we cannot just go and willy-nilly add
> calls to kobject_put() in the error path of kobject_init_and_add() if
> the original code was not written under the assumption that the release
> method could be called during the error path (I have found 2 places at
> least where behaviour of calling the release method is non-trivial to
> ascertain).

Well, generally speaking, you can add kobject_put() somewhere only if
invoking the ->release() callback for the ktype of the kobject in
question is safe at that point.

Now, it may be unsafe for two reasons: there may be active references
to the memory that would be freed by ->release() along with the
kobject (as you said above) or the code in ->release() may have
assumed some initialization to take place before it runs and so
->release() should not be invoked before completing that
initialization.

> I guess, as Greg said, its just a matter that reference counting within
> the kernel is a hard problem.  So we fix the easy ones and then look a
> bit harder at the hard ones ...
>
> Any better suggestion?

Well, I would say we fix all of them, but we are careful enough to
understand what's going on.

And the more complex cases will take more work (and time) to fix.  As
always. :-)

Re: kobject_init_and_add() confusion

[Petr Mladek]

On Fri 2019-05-10 12:35:38, Tobin C. Harding wrote:
> On Wed, May 01, 2019 at 09:54:16AM +0200, Rafael J. Wysocki wrote:
> > On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@tobin.cc> wrote:
> > > TODO
> > > ----
> > >
> > > - Fix all the callsites to kobject_init_and_add()
> > > - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> > > - Add a section to Documentation/kobject.txt [optional]
> > > - Add a sample usage file under samples/kobject [optional]
> > 
> > The plan sounds good to me, but there is one thing to note IMO:
> > kobject_cleanup() invokes the ->release() callback for the ktype, so
> > these callbacks need to be able to cope with kobjects after a failing
> > kobject_add() which may not be entirely obvious to developers
> > introducing them ATM.
> 
> It has taken a while for this to soak in.  This is actually quite an
> insidious issue.  If I give an example and perhaps we can come to a
> solution.  This example is based on the code (and assumptions) in
> mm/slub.c
> 
> If a developer has an object that they wish to add to sysfs they go
> ahead and embed a kobject in it.  Correctly set up a ktype including
> release function that just frees the object (using container of).  Now
> assume that the object is already set up and in use when we go to set up
> the sysfs entry.

It would say that this is a bad design. I see the creation of the sysfs
entry as part of the initialization. The object should not be made
usable before it is fully initialized.


> If kobject_init_and_add() fails and we correctly call
> kobject_put() the containing object will be free'd.  Yet the calling
> code may not be done with the object, more to the point just because
> sysfs setup fails the object is now unusable.  Besides the interesting
> theoretical discussion this means we cannot just go and willy-nilly add
> calls to kobject_put() in the error path of kobject_init_and_add() if
> the original code was not written under the assumption that the release
> method could be called during the error path (I have found 2 places at
> least where behaviour of calling the release method is non-trivial to
> ascertain).

kobject usage is complicated and it is easy to make it wrong. I think
that this is motivation to improve the documentation and adding
good examples.

> I guess, as Greg said, its just a matter that reference counting within
> the kernel is a hard problem.  So we fix the easy ones and then look a
> bit harder at the hard ones ...

The people working on the affected subsystem should be able to help.
They might have misunderstood kobjects. But they should be more
familiar with the other dependencies.

Thanks for working on it.

Best Regards,
Petr

Re: kobject_init_and_add() confusion

[Tobin C. Harding]

On Fri, May 10, 2019, at 19:40, Petr Mladek wrote:
> On Fri 2019-05-10 12:35:38, Tobin C. Harding wrote:
> > On Wed, May 01, 2019 at 09:54:16AM +0200, Rafael J. Wysocki wrote:
> > > On Wed, May 1, 2019 at 1:38 AM Tobin C. Harding <me@tobin.cc> wrote:
> > > > TODO
> > > > ----
> > > >
> > > > - Fix all the callsites to kobject_init_and_add()
> > > > - Further clarify the function docstring for kobject_init_and_add() [perhaps]
> > > > - Add a section to Documentation/kobject.txt [optional]
> > > > - Add a sample usage file under samples/kobject [optional]
> > > 
> > > The plan sounds good to me, but there is one thing to note IMO:
> > > kobject_cleanup() invokes the ->release() callback for the ktype, so
> > > these callbacks need to be able to cope with kobjects after a failing
> > > kobject_add() which may not be entirely obvious to developers
> > > introducing them ATM.
> > 
> > It has taken a while for this to soak in.  This is actually quite an
> > insidious issue.  If I give an example and perhaps we can come to a
> > solution.  This example is based on the code (and assumptions) in
> > mm/slub.c
> > 
> > If a developer has an object that they wish to add to sysfs they go
> > ahead and embed a kobject in it.  Correctly set up a ktype including
> > release function that just frees the object (using container of).  Now
> > assume that the object is already set up and in use when we go to set up
> > the sysfs entry.
> 
> It would say that this is a bad design. I see the creation of the sysfs
> entry as part of the initialization. The object should not be made
> usable before it is fully initialized.

It may be a case of my lack of understanding of object lifecycles here and not bad design, if as you say creation of sysfs is always part of initialisation then the problem I describe above should not exist (and it may well not, assumptions behind code are hard to grok).
 
> > If kobject_init_and_add() fails and we correctly call
> > kobject_put() the containing object will be free'd.  Yet the calling
> > code may not be done with the object, more to the point just because
> > sysfs setup fails the object is now unusable.  Besides the interesting
> > theoretical discussion this means we cannot just go and willy-nilly add
> > calls to kobject_put() in the error path of kobject_init_and_add() if
> > the original code was not written under the assumption that the release
> > method could be called during the error path (I have found 2 places at
> > least where behaviour of calling the release method is non-trivial to
> > ascertain).
> 
> kobject usage is complicated and it is easy to make it wrong. I think
> that this is motivation to improve the documentation and adding
> good examples.

Cool, I did work on adding your example from last week into samples/kobject but I wasn't able to come up with anything that I was totally happy with.  Hard to use API needs minimal concise correct examples right, I'm going to keep at that as I learn more from seeing/patching current kobject code.

> > I guess, as Greg said, its just a matter that reference counting within
> > the kernel is a hard problem.  So we fix the easy ones and then look a
> > bit harder at the hard ones ...
> 
> The people working on the affected subsystem should be able to help.
> They might have misunderstood kobjects. But they should be more
> familiar with the other dependencies.

Sure thing.

> Thanks for working on it.

Things that bend ones brain are the funnest to work on ;)

Cheers,
Tobin.