on static kobjects and double frees...

on static kobjects and double frees...

[Arthur Jones]

Hi Greg,  The edac pci sysfs generic layer uses a static
kobject as a placeholder parent where edac pci drivers
are inserted.  An atomic count is used to know when
to kobject_init_add_add or kobject_put the static kobject.
The issue with this is that the name gets double freed
on the second module load as edac does not clear it, and
kobject_cleanup does not clear it.

The quick fix was to clear the static kobj name before
calling kobject_init, but that seems a bit fragile as it
involves knowing the internals of kobject_put.  Perhaps
the name should be cleared before calling the kobject
release method?  Something like this (not even compile
tested):

diff --git a/lib/kobject.c b/lib/kobject.c
index 718e510..7dfe906 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -552,6 +552,9 @@ static void kobject_cleanup(struct kobject *kobj)
 	if (t && t->release) {
 		pr_debug("kobject: '%s' (%p): calling ktype release\n",
 			 kobject_name(kobj), kobj);
+
+		/* avoid double free with static kobjects... */
+		kobj->name = NULL;
 		t->release(kobj);
 	}
 
What do you think?  I'm happy to implement and test whatever
you think is best...

The edac code in question is drivers/edac/edac_pci_sysfs.c,
the static kobject is called edac_pci_top_main_kobj...

Arthur

Re: on static kobjects and double frees...

[Greg KH]

On Tue, Jun 10, 2008 at 08:58:50AM -0700, Arthur Jones wrote:
> Hi Greg,  The edac pci sysfs generic layer uses a static
> kobject as a placeholder parent where edac pci drivers
> are inserted.

Hm, stop right there.

kobjects are not supposed to be static, bad things happen if you do that
(including the kernel itself will warn you about them, unless you gave
it an empty release function, and if so, then see
Documentation/kobject.txt and prepare to be mocked...)

> An atomic count is used to know when to kobject_init_add_add or
> kobject_put the static kobject.

No, the kobject itself should handle stuff like this.

> The issue with this is that the name gets double freed
> on the second module load as edac does not clear it, and
> kobject_cleanup does not clear it.

Yup, that's because you should not be doing this :)

> The quick fix was to clear the static kobj name before
> calling kobject_init, but that seems a bit fragile as it
> involves knowing the internals of kobject_put.  Perhaps
> the name should be cleared before calling the kobject
> release method?  Something like this (not even compile
> tested):

No, please just dynamically create your kobject.  It's easier than ever
to do this today (just one function call!).

> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 718e510..7dfe906 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -552,6 +552,9 @@ static void kobject_cleanup(struct kobject *kobj)
>  	if (t && t->release) {
>  		pr_debug("kobject: '%s' (%p): calling ktype release\n",
>  			 kobject_name(kobj), kobj);
> +
> +		/* avoid double free with static kobjects... */
> +		kobj->name = NULL;
>  		t->release(kobj);
>  	}
>  
> What do you think?  I'm happy to implement and test whatever
> you think is best...
> 
> The edac code in question is drivers/edac/edac_pci_sysfs.c,
> the static kobject is called edac_pci_top_main_kobj...

In looking at that code, I really don't understand what you are trying
to do with this "tracking" kobject.  Why do that at all, and not just
create a kobject and hang things off of it if you want to have that.
The whole lifetime of it will be properly handled automatically if you
do that.

thanks,

greg k-h

Re: on static kobjects and double frees...

[Arthur Jones]

Hi Greg, ...

On Tue, Jun 10, 2008 at 09:23:41AM -0700, Greg KH wrote:
> On Tue, Jun 10, 2008 at 08:58:50AM -0700, Arthur Jones wrote:
> > Hi Greg,  The edac pci sysfs generic layer uses a static
> > kobject as a placeholder parent where edac pci drivers
> > are inserted.
> 
> Hm, stop right there.
> 
> kobjects are not supposed to be static, bad things happen if you do that
> (including the kernel itself will warn you about them, unless you gave
> it an empty release function, and if so, then see
> Documentation/kobject.txt and prepare to be mocked...)

OK, I wondered about this, but I didn't see anything
in Documentation/kobject.txt that said that kobjects
can not be static.  But now that I've fixed the double
free bug, I'm seeing the warning you mentioned above...

I don't know how the current code came to be, so I
can't speak to your issues below.  But, with these
pointers, I think I can get things cleaned up properly.

Thank you!

Arthur

> > An atomic count is used to know when to kobject_init_add_add or
> > kobject_put the static kobject.
> 
> No, the kobject itself should handle stuff like this.
> 
> > The issue with this is that the name gets double freed
> > on the second module load as edac does not clear it, and
> > kobject_cleanup does not clear it.
> 
> Yup, that's because you should not be doing this :)
> 
> > The quick fix was to clear the static kobj name before
> > calling kobject_init, but that seems a bit fragile as it
> > involves knowing the internals of kobject_put.  Perhaps
> > the name should be cleared before calling the kobject
> > release method?  Something like this (not even compile
> > tested):
> 
> No, please just dynamically create your kobject.  It's easier than ever
> to do this today (just one function call!).
> 
> >
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 718e510..7dfe906 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -552,6 +552,9 @@ static void kobject_cleanup(struct kobject *kobj)
> >       if (t && t->release) {
> >               pr_debug("kobject: '%s' (%p): calling ktype release\n",
> >                        kobject_name(kobj), kobj);
> > +
> > +             /* avoid double free with static kobjects... */
> > +             kobj->name = NULL;
> >               t->release(kobj);
> >       }
> >
> > What do you think?  I'm happy to implement and test whatever
> > you think is best...
> >
> > The edac code in question is drivers/edac/edac_pci_sysfs.c,
> > the static kobject is called edac_pci_top_main_kobj...
> 
> In looking at that code, I really don't understand what you are trying
> to do with this "tracking" kobject.  Why do that at all, and not just
> create a kobject and hang things off of it if you want to have that.
> The whole lifetime of it will be properly handled automatically if you
> do that.
> 
> thanks,
> 
> greg k-h

Re: on static kobjects and double frees...

[Greg KH]

On Tue, Jun 10, 2008 at 09:38:00AM -0700, Arthur Jones wrote:
> Hi Greg, ...
> 
> On Tue, Jun 10, 2008 at 09:23:41AM -0700, Greg KH wrote:
> > On Tue, Jun 10, 2008 at 08:58:50AM -0700, Arthur Jones wrote:
> > > Hi Greg,  The edac pci sysfs generic layer uses a static
> > > kobject as a placeholder parent where edac pci drivers
> > > are inserted.
> > 
> > Hm, stop right there.
> > 
> > kobjects are not supposed to be static, bad things happen if you do that
> > (including the kernel itself will warn you about them, unless you gave
> > it an empty release function, and if so, then see
> > Documentation/kobject.txt and prepare to be mocked...)
> 
> OK, I wondered about this, but I didn't see anything
> in Documentation/kobject.txt that said that kobjects
> can not be static.  But now that I've fixed the double
> free bug, I'm seeing the warning you mentioned above...

Hm, there is the following text in that file:
	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.

> I don't know how the current code came to be, so I
> can't speak to your issues below.  But, with these
> pointers, I think I can get things cleaned up properly.

Great, if you want me to review it, I'd be glad to do so.

thanks,

greg k-h

Re: on static kobjects and double frees...

[Arthur Jones]

Hi Greg, ...

On Tue, Jun 10, 2008 at 09:42:41AM -0700, Greg KH wrote:
> > [...]
> > OK, I wondered about this, but I didn't see anything
> > in Documentation/kobject.txt that said that kobjects
> > can not be static.  But now that I've fixed the double
> > free bug, I'm seeing the warning you mentioned above...
> 
> Hm, there is the following text in that file:
>         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.

Indeed, I see that now.  Sorry for the noise, I should
have read more carefully...

> > I don't know how the current code came to be, so I
> > can't speak to your issues below.  But, with these
> > pointers, I think I can get things cleaned up properly.
> 
> Great, if you want me to review it, I'd be glad to do so.

Thanks!

Arthur

Re: on static kobjects and double frees...

[Doug Thompson]

--- Arthur Jones <ajones@riverbed.com> wrote:

> Hi Greg, ...
> 
> On Tue, Jun 10, 2008 at 09:23:41AM -0700, Greg KH wrote:
> > On Tue, Jun 10, 2008 at 08:58:50AM -0700, Arthur Jones wrote:
> > > Hi Greg,  The edac pci sysfs generic layer uses a static
> > > kobject as a placeholder parent where edac pci drivers
> > > are inserted.
> > 
> > Hm, stop right there.
> > 
> > kobjects are not supposed to be static, bad things happen if you do that
> > (including the kernel itself will warn you about them, unless you gave
> > it an empty release function, and if so, then see
> > Documentation/kobject.txt and prepare to be mocked...)

The edac_pci was my first coding with kobjects sometime ago and I have not reviewed them for quite
awhile, since I have been working more on memory controllers. Thus it was my bad.

Arthur, thanks for tracking that down and reviewing it. All the memory controller kobjects are all
dynamic. The edac PCI code needed to be refactored and it looks like you did it.

thanks again

doug t


For the patch:

Acked-by:    doug thompson <dougthompson@xmission.com>




W1DUG

Re: on static kobjects and double frees...

[Arthur Jones]

Hi Doug, ...

On Tue, Jun 10, 2008 at 02:14:30PM -0700, Doug Thompson wrote:
> [...]
> Arthur, thanks for tracking that down and reviewing it. All the memory controller kobjects are all
> dynamic. [...]

You're welcome, I learned a lot about kobjects
that I should have already known...

AFAICS, mc is not quite clean yet, though, as
mc_kset is static and I don't think that is right.
It does, however, look like less work than edac_pci_sysfs.c...

> [...] The edac PCI code needed to be refactored and it looks like you did it.

I only did the bare minimum, there's a lot of kobject
stuff in there that looks suspect to me, e.g.:

* the "pci" object should be a kset rather than a kobject?
* if the "pci" object is a kset, can the whole global list
  thing in edac_pci just go away?
* do we really need the atomic_inc counter, wouldn't a mutex
  be clearer?
* can we initialize "pci" at init time thereby removing the
  need even for a mutex on the "pci" initializer?  This does
  change the sysfs semantics slightly, though, as the "pci"
  object would no longer be created lazily.  I'm not sure if
  that's a problem.

and other minor things.  It looks to me like it could
use a thorough scrubbing.  The good news is that I don't
think the API would change much if at all.  I can have a
closer look if you are interested...

Arthur