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