kobjects: mark cleaned up kobjects as unitialized

kobjects: mark cleaned up kobjects as unitialized

[eran liberty]

Hi Greg & Balaji,

After diving into the LDKM and failed to spot the point where you
actually un-initialize the 'state_initialized' of a kobject... and
since I have statically allocated object which trip over this very
same trap...

Google-ing for others who fell into this trap, I found your thread/patch at:
http://lkml.org/lkml/2008/3/8/155
and
http://lkml.indiana.edu/hypermail/linux/kernel/0902.0/01969.html

I noticed this patch did not make it into the mainline.

Is this patch still valid?
Is there some other, better way to do it by the book?

Right now I by-pass the problem by memset-ing the whole object after I
release it... but I feel this is a bit brutal.

-- Liberty

Re: kobjects: mark cleaned up kobjects as unitialized

[Greg KH]

On Wed, Nov 25, 2009 at 11:27:58AM +0200, eran liberty wrote:
> Hi Greg & Balaji,
> 
> After diving into the LDKM and failed to spot the point where you
> actually un-initialize the 'state_initialized' of a kobject... and
> since I have statically allocated object which trip over this very
> same trap...

Ah, there's your problem, don't statically allocate a kobject.  Fix that
and your issue goes away, right?

> Google-ing for others who fell into this trap, I found your thread/patch at:
> http://lkml.org/lkml/2008/3/8/155
> and
> http://lkml.indiana.edu/hypermail/linux/kernel/0902.0/01969.html
> 
> I noticed this patch did not make it into the mainline.
> 
> Is this patch still valid?
> Is there some other, better way to do it by the book?

Do not statically allocate a kobject.

> Right now I by-pass the problem by memset-ing the whole object after I
> release it... but I feel this is a bit brutal.

You should be freeing your memory in your release function.

Do you have a pointer to your code somewhere?

thanks,

greg k-h

Re: kobjects: mark cleaned up kobjects as unitialized

[eran liberty]

Hi Greg,

On Wed, Nov 25, 2009 at 2:35 PM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Nov 25, 2009 at 11:27:58AM +0200, eran liberty wrote:
>> Hi Greg & Balaji,
>>
>> After diving into the LDKM and failed to spot the point where you
>> actually un-initialize the 'state_initialized' of a kobject... and
>> since I have statically allocated object which trip over this very
>> same trap...
>
> Ah, there's your problem, don't statically allocate a kobject.  Fix that
> and your issue goes away, right?

right... but... I want to :).

Is there a Linux directive saying 'thus shall not statically allocate
(or reuse in any other way) kobjects"?

>
>> Google-ing for others who fell into this trap, I found your thread/patch at:
>> http://lkml.org/lkml/2008/3/8/155
>> and
>> http://lkml.indiana.edu/hypermail/linux/kernel/0902.0/01969.html
>>
>> I noticed this patch did not make it into the mainline.
>>
>> Is this patch still valid?

Why was your proposed patch not merged?
Is there some logic behind not having it?

>> Is there some other, better way to do it by the book?
>
> Do not statically allocate a kobject.
>
>> Right now I by-pass the problem by memset-ing the whole object after I
>> release it... but I feel this is a bit brutal.

Assuming I will keep it static and clear the status_initialize bit (by
memset-ing the whole object) after the kobject was released... am I
doing something wrong? should i expect other bad phenomena?

>
> You should be freeing your memory in your release function.
>
Should I free the object itself in release() function?
In OO-like thinking I would expect release() to be a cleanup function
for the device to be used, while the kfree() be done by the same
object which did the kmalloc()

> Do you have a pointer to your code somewhere?
>

I will pack something and send you (i dont think Mr. vger will
tolerate attachments :) )
but basically you can demonstrate the problem with this simple code:

struct device dev;
while(1) {
  dev->release = dummy_release_function_that_does_nothing;
  device_register(&dev); /* will fail on second iteration without the
memset!! ?? */
  device_unregister(&dev);
  memset(&dev,0,sizeof(struct device)); /* should be unnecessary  */
}

> thanks,
>
> greg k-h
>

thanks,
-- Liberty

Re: kobjects: mark cleaned up kobjects as unitialized

[Greg KH]

On Wed, Nov 25, 2009 at 04:27:59PM +0200, eran liberty wrote:
> Hi Greg,
> 
> On Wed, Nov 25, 2009 at 2:35 PM, Greg KH <gregkh@suse.de> wrote:
> > On Wed, Nov 25, 2009 at 11:27:58AM +0200, eran liberty wrote:
> >> Hi Greg & Balaji,
> >>
> >> After diving into the LDKM and failed to spot the point where you
> >> actually un-initialize the 'state_initialized' of a kobject... and
> >> since I have statically allocated object which trip over this very
> >> same trap...
> >
> > Ah, there's your problem, don't statically allocate a kobject.  Fix that
> > and your issue goes away, right?
> 
> right... but... I want to :).

No you do not.  Seriously, why would you want a static structure for
something that is, by the very nature of what it is supposed to do,
dynamic?

> Is there a Linux directive saying 'thus shall not statically allocate
> (or reuse in any other way) kobjects"?

Yes there is: Documentation/kobject.txt says:
	Because kobjects are dynamic, they must not be declared
	statically or on the stack, but instead, always allocated
	dynamically.  Future versions of the kernel will contain a
	run-time check for kobjects that are created statically and will
	warn the developer of this improper usage.

Doesn't anyone read documentation anymore?  :(

> >> Google-ing for others who fell into this trap, I found your thread/patch at:
> >> http://lkml.org/lkml/2008/3/8/155
> >> and
> >> http://lkml.indiana.edu/hypermail/linux/kernel/0902.0/01969.html
> >>
> >> I noticed this patch did not make it into the mainline.
> >>
> >> Is this patch still valid?
> 
> Why was your proposed patch not merged?
> Is there some logic behind not having it?

Because we realized the root problem was static kobjects, which we do
not support.

> >> Is there some other, better way to do it by the book?
> >
> > Do not statically allocate a kobject.
> >
> >> Right now I by-pass the problem by memset-ing the whole object after I
> >> release it... but I feel this is a bit brutal.
> 
> Assuming I will keep it static and clear the status_initialize bit (by
> memset-ing the whole object) after the kobject was released... am I
> doing something wrong? should i expect other bad phenomena?

Again, DO NOT CREATE A STATIC KOBJECT.

Please.  Pretty please?  Pretty please with a cherry on top?

Ok, how about this, let's flip this the other way around, why do you
feel that you have to use a static kobject?

> > You should be freeing your memory in your release function.
> >
> Should I free the object itself in release() function?

Yes.

> In OO-like thinking I would expect release() to be a cleanup function
> for the device to be used, while the kfree() be done by the same
> object which did the kmalloc()

No, the object destroys itself when the last reference to it is dropped,
which is a very OO-like way of doing things (smart pointers and all of
that.)

Please read the above referenced document about kobject and how they
operate.  If you have any questions after doing that, please let me
know.

Oh, and another thing, your really do not want to be using a "raw"
kobject, use 'struct device' instead :)

> 
> > Do you have a pointer to your code somewhere?
> >
> 
> I will pack something and send you (i dont think Mr. vger will
> tolerate attachments :) )

Inline is the way to do it, I don't want a tarball either.

> but basically you can demonstrate the problem with this simple code:
> 
> struct device dev;
> while(1) {
>   dev->release = dummy_release_function_that_does_nothing;

WTF!!!

Ugh, if I see one more person try to "trick" the kernel from reporting
this error by providing a "dummy" release function, I'm going to go find
a kitten to kick.

Seriously, why are you blatently ignoring the warning that the kernel is
trying to be nice and give to you?  Do we just invent messages that need
to be worked around?

>   device_register(&dev); /* will fail on second iteration without the
> memset!! ?? */

Yup, don't do that.

>   device_unregister(&dev);
>   memset(&dev,0,sizeof(struct device)); /* should be unnecessary  */

No, this code is broken.

{sigh}  Doesn't anyone read the archives or documentation anymore?

greg k-h

Re: kobjects: mark cleaned up kobjects as unitialized

[Greg KH]

On Wed, Nov 25, 2009 at 07:27:47AM -0800, Greg KH wrote:
> > but basically you can demonstrate the problem with this simple code:
> > 
> > struct device dev;
> > while(1) {
> >   dev->release = dummy_release_function_that_does_nothing;
> 
> WTF!!!
> 
> Ugh, if I see one more person try to "trick" the kernel from reporting
> this error by providing a "dummy" release function, I'm going to go find
> a kitten to kick.
> 
> Seriously, why are you blatently ignoring the warning that the kernel is
> trying to be nice and give to you?  Do we just invent messages that need
> to be worked around?

Oh, and before anyone thinks that I'm being too harsh here, again, refer
to Documentation/kobject.txt:
	Note that the kernel will warn you if you forget to provide a
	release() method.  Do not try to get rid of this warning by
	providing an "empty" release function; you will be mocked
	mercilessly by the kobject maintainer if you attempt this.

Hence my mocking.

thanks,

greg k-h