Bug (?) in subsystem kset refcounts

Bug (?) in subsystem kset refcounts

[Alan Stern]

I hesitate to say this is definitely a bug, since it might be intended 
behavior.  But it is rather strange.

Subsystems included an embedded kset, which itself includes an embedded 
kobject and so is subject to reference counting.  Whenever a kobject 
belonging to the kset is destroyed, the kset's reference count is 
decremented.  However, kobjects can be added to a kset via the three 
macros

	kobj_set_kset_s, kset_set_kset_s, and subsys_set_kset

and these do _not_ increment the kset's reference count.  As a result, the 
reference count only goes down, not up, quickly becoming negative.

Now maybe this doesn't matter -- if subsystems are intended to be
permanent (i.e., never released), for example.  But it provokes a warning
message from kernels that check for reference counts going below zero,
appears to be unintended, and may cause other problems as well.

The patch below is offered as a possible solution.  I don't know that it's 
the right one, but at least it prevents the unwanted warning messages.

Alan Stern


--- a/include/linux/kobject.h.orig	Thu Sep 11 09:46:38 2003
+++ a/include/linux/kobject.h	Sat Nov  8 17:57:35 2003
@@ -168,7 +168,7 @@
  */
 
 #define kobj_set_kset_s(obj,subsys) \
-	(obj)->kobj.kset = &(subsys).kset
+	(obj)->kobj.kset = kset_get(&(subsys).kset)
 
 /**
  *	kset_set_kset_s(obj,subsys) - set kset for embedded kset.
@@ -182,7 +182,7 @@
  */
 
 #define kset_set_kset_s(obj,subsys) \
-	(obj)->kset.kobj.kset = &(subsys).kset
+	(obj)->kset.kobj.kset = kset_get(&(subsys).kset)
 
 /**
  *	subsys_set_kset(obj,subsys) - set kset for subsystem
@@ -195,7 +195,7 @@
  */
 
 #define subsys_set_kset(obj,_subsys) \
-	(obj)->subsys.kset.kobj.kset = &(_subsys).kset
+	(obj)->subsys.kset.kobj.kset = kset_get(&(_subsys).kset)
 
 extern void subsystem_init(struct subsystem *);
 extern int subsystem_register(struct subsystem *);

Re: Bug (?) in subsystem kset refcounts

[Greg KH]

On Sat, Nov 08, 2003 at 10:20:03PM -0500, Alan Stern wrote:
> I hesitate to say this is definitely a bug, since it might be intended 
> behavior.  But it is rather strange.
> 
> Subsystems included an embedded kset, which itself includes an embedded 
> kobject and so is subject to reference counting.  Whenever a kobject 
> belonging to the kset is destroyed, the kset's reference count is 
> decremented.  However, kobjects can be added to a kset via the three 
> macros
> 
> 	kobj_set_kset_s, kset_set_kset_s, and subsys_set_kset
> 
> and these do _not_ increment the kset's reference count.  As a result, the 
> reference count only goes down, not up, quickly becoming negative.

See the patch that went into Linus's tree yesterday to fix where this
would happen.

But yes, usages of these macros is touchy, and we need to get it
correct.  Your proposed patch will never allow the reference counts to
go back to zero.

Also, notice that when the kobject is initialized, the kset set by these
macros is then incremented.

thanks,

greg k-h

Re: Bug (?) in subsystem kset refcounts

[Alan Stern]

On Sat, 8 Nov 2003, Greg KH wrote:

> See the patch that went into Linus's tree yesterday to fix where this
> would happen.

Ah yes.  Glad to see the problem has been fixed.

> But yes, usages of these macros is touchy, and we need to get it
> correct.  Your proposed patch will never allow the reference counts to
> go back to zero.
> 
> Also, notice that when the kobject is initialized, the kset set by these
> macros is then incremented.

Right, I didn't notice that.

It's unfortunate that initialization of various parts of a kobject or
driver-model object is so order-sensitive.  Worse yet is the fact that
these dependencies are not mentioned in Documentation/kobject.txt,
Documentation/driver-model, include/linux/kobject.h, or
include/linux/device.h.  Wouldn't it be a good idea to add, for example to
Documentation/kobject.txt, a description of which members of struct
kobject must be initialized before calling kobject_init() and which must
be initialized before calling kobject_add()?  And of course to do the same
for all the other important structures as well.

Alan Stern

Re: Bug (?) in subsystem kset refcounts

[Greg KH]

On Sun, Nov 09, 2003 at 12:01:41PM -0500, Alan Stern wrote:
> It's unfortunate that initialization of various parts of a kobject or
> driver-model object is so order-sensitive.  Worse yet is the fact that
> these dependencies are not mentioned in Documentation/kobject.txt,
> Documentation/driver-model, include/linux/kobject.h, or
> include/linux/device.h.  Wouldn't it be a good idea to add, for example to
> Documentation/kobject.txt, a description of which members of struct
> kobject must be initialized before calling kobject_init() and which must
> be initialized before calling kobject_add()?  And of course to do the same
> for all the other important structures as well.

I agree.  More documentation is definitely needed.  I know Pat has some
documentation changes queued up, but patches to update portions that are
missing (such as these requirements) will always be gladly accepted.

thanks,

greg k-h