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

[ message continues ]

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



No, please just dynamically create your kobject.  It's easier than ever

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

[ message continues ]

Hi Greg, ...


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!

--

[ message continues ]

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

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

thanks,

greg k-h
--

[ message continues ]

Hi Greg, ...


Indeed, I see that now.  Sorry for the noise, I should

Thanks!

Arthur
--

[ message continues ]

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

[ message continues ]

Hi Doug, ...


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.

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

[ message continues ]