[PATCH] Kobjects: drop child->parent ref at unregistration

[PATCH] Kobjects: drop child->parent ref at unregistration

[Alan Stern]

This patch (as1015) reverts changes that were made to the driver core
about four years ago.  The intent back then was to avoid certain kinds
of invalid memory accesses by leaving kernel objects allocated as long
as any of their children were still allocated.  The original and
correct approach was to wait only as long as any children were still
_registered_; that's what this patch reinstates.

This fixes a problem in the SCSI core made visible by the class_device
to regular device conversion: A reference loop (scsi_device holds
reference to request_queue, which is the child of a gendisk, which is
the child of the scsi_device) prevents the data structures from being
released, even though they are deregistered okay.

It's possible that this change will cause a few bugs to surface,
things that have been hidden for several years.  They can be fixed
easily enough by having the child device take an explicit reference to
the parent whenever needed.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Greg:

I'm formally submitting this so that it can get some testing in your 
development tree and in -mm.  So far everyone who has considered the 
matter thinks that this is a good change.  Any bugs it has papered over 
should be fixable.

Alan Stern


Index: usb-2.6/lib/kobject.c
===================================================================
--- usb-2.6.orig/lib/kobject.c
+++ usb-2.6/lib/kobject.c
@@ -206,12 +206,16 @@ void kobject_init(struct kobject * kobj)
 
 static void unlink(struct kobject * kobj)
 {
+	struct kobject *parent = kobj->parent;
+
 	if (kobj->kset) {
 		spin_lock(&kobj->kset->list_lock);
 		list_del_init(&kobj->entry);
 		spin_unlock(&kobj->kset->list_lock);
 	}
+	kobj->parent = NULL;
 	kobject_put(kobj);
+	kobject_put(parent);
 }
 
 /**
@@ -262,7 +266,6 @@ int kobject_add(struct kobject * kobj)
 	if (error) {
 		/* unlink does the kobject_put() for us */
 		unlink(kobj);
-		kobject_put(parent);
 
 		/* be noisy on error issues */
 		if (error == -EEXIST)
@@ -516,7 +519,6 @@ void kobject_cleanup(struct kobject * ko
 {
 	struct kobj_type * t = get_ktype(kobj);
 	struct kset * s = kobj->kset;
-	struct kobject * parent = kobj->parent;
 	const char *name = kobj->k_name;
 
 	pr_debug("kobject %s: cleaning up\n",kobject_name(kobj));
@@ -533,7 +535,6 @@ void kobject_cleanup(struct kobject * ko
 	}
 	if (s)
 		kset_put(s);
-	kobject_put(parent);
 }
 
 static void kobject_release(struct kref *kref)

Re: [PATCH] Kobjects: drop child->parent ref at unregistration

[Andrew Morton]

On Mon, 19 Nov 2007 10:53:40 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> This patch (as1015) reverts changes that were made to the driver core
> about four years ago.  The intent back then was to avoid certain kinds
> of invalid memory accesses by leaving kernel objects allocated as long
> as any of their children were still allocated.  The original and
> correct approach was to wait only as long as any children were still
> _registered_; that's what this patch reinstates.

What happened with this?

> This fixes a problem in the SCSI core made visible by the class_device
> to regular device conversion: A reference loop (scsi_device holds
> reference to request_queue, which is the child of a gendisk, which is
> the child of the scsi_device) prevents the data structures from being
> released, even though they are deregistered okay.
> 
> It's possible that this change will cause a few bugs to surface,
> things that have been hidden for several years.  They can be fixed
> easily enough by having the child device take an explicit reference to
> the parent whenever needed.
> 

How will such bugs manifest?  Ideally via a nice printk and a stack trace
followed by damage avoidance.

If it's via a mysterious crash or something similarly obscure then can we
improve that?

Re: [PATCH] Kobjects: drop child->parent ref at unregistration

[Alan Stern]

On Mon, 26 Nov 2007, Andrew Morton wrote:

> On Mon, 19 Nov 2007 10:53:40 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > This patch (as1015) reverts changes that were made to the driver core
> > about four years ago.  The intent back then was to avoid certain kinds
> > of invalid memory accesses by leaving kernel objects allocated as long
> > as any of their children were still allocated.  The original and
> > correct approach was to wait only as long as any children were still
> > _registered_; that's what this patch reinstates.
> 
> What happened with this?

As far as I know, it's on Greg's queue.

> > This fixes a problem in the SCSI core made visible by the class_device
> > to regular device conversion: A reference loop (scsi_device holds
> > reference to request_queue, which is the child of a gendisk, which is
> > the child of the scsi_device) prevents the data structures from being
> > released, even though they are deregistered okay.
> > 
> > It's possible that this change will cause a few bugs to surface,
> > things that have been hidden for several years.  They can be fixed
> > easily enough by having the child device take an explicit reference to
> > the parent whenever needed.
> > 
> 
> How will such bugs manifest?  Ideally via a nice printk and a stack trace
> followed by damage avoidance.

They will manifest in the same way as any other use-after-free bug: an 
oops message and either death of the current process or a system hang.

Obviously I'm not aware of any such bugs -- if I were, I'd fix them.  
Greg has expressed concern that some USB serial drivers might have this 
problem.  I'll do what testing I can (not much because I don't have any 
USB serial devices).

> If it's via a mysterious crash or something similarly obscure then can we
> improve that?

I can't think of anything offhand.  Maybe someone else can.

Alan Stern

Re: [PATCH] Kobjects: drop child->parent ref at unregistration

[Greg KH]

On Mon, Nov 26, 2007 at 09:29:36PM -0500, Alan Stern wrote:
> On Mon, 26 Nov 2007, Andrew Morton wrote:
> 
> > On Mon, 19 Nov 2007 10:53:40 -0500 (EST)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > This patch (as1015) reverts changes that were made to the driver core
> > > about four years ago.  The intent back then was to avoid certain kinds
> > > of invalid memory accesses by leaving kernel objects allocated as long
> > > as any of their children were still allocated.  The original and
> > > correct approach was to wait only as long as any children were still
> > > _registered_; that's what this patch reinstates.
> > 
> > What happened with this?
> 
> As far as I know, it's on Greg's queue.

Yes, it's in my queue still.  Kay and Alan want the patch as it fixes
the new /sys/block -> /sys/class/block patches.  When I add that one
back to my tree (Kay has debugged your old G5 problem now), I'll either
add this patch too, or figure out why it should not be needed.

For now, I'd recommend dropping it from your tree Andrew, as it might
cause odd issues on device removal (not quite sure though...)

thanks,

greg k-h

Re: [PATCH] Kobjects: drop child->parent ref at unregistration

[Alan Stern]

On Tue, 27 Nov 2007, Greg KH wrote:

> For now, I'd recommend dropping it from your tree Andrew, as it might
> cause odd issues on device removal (not quite sure though...)

But then there's the counter-argument: If the patch does cause any odd 
issues to shake loose, learning about them in advance by exposing the 
patch in the -mm tree would be worthwhile.

Sounds like it's time for an executive decision...  :-)

Alan Stern

P.S.: I did test the patch by running the g_serial gadget with
usbserial loaded on the host.  Disconnecting the gadget while a user
process held the /dev/ttyUSB0 file open didn't cause any problems.

Re: [PATCH] Kobjects: drop child->parent ref at unregistration

[Greg KH]

On Tue, Nov 27, 2007 at 12:41:23PM -0500, Alan Stern wrote:
> On Tue, 27 Nov 2007, Greg KH wrote:
> 
> > For now, I'd recommend dropping it from your tree Andrew, as it might
> > cause odd issues on device removal (not quite sure though...)
> 
> But then there's the counter-argument: If the patch does cause any odd 
> issues to shake loose, learning about them in advance by exposing the 
> patch in the -mm tree would be worthwhile.
> 
> Sounds like it's time for an executive decision...  :-)

Heh.  I'm not ignoring the patch, and will apply it if I test it out and
see that it doesn't break anything.  Then I will let it bake in -mm for
a while.

I just don't want to rush it as this area of the kobject core is nasty
and full of tricks that I always forget.

Let me finish writing up this documentation on what I do remember and
then I'll attack the block device patch and this one.

thanks,

greg k-h

Re: [PATCH] Kobjects: drop child->parent ref at unregistration

[Alan Stern]

On Tue, 27 Nov 2007, Greg KH wrote:

> Let me finish writing up this documentation on what I do remember and
> then I'll attack the block device patch and this one.

Be sure to CC: me when your documentation patch is sent out.

Alan Stern

patch kobject-drop-child-parent-ref-at-unregistration.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: Kobject: drop child->parent ref at unregistration

to my gregkh-2.6 tree.  Its filename is

     kobject-drop-child-parent-ref-at-unregistration.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


>From stern@rowland.harvard.edu  Thu Nov 29 13:12:08 2007
From: Alan Stern <stern@rowland.harvard.edu>
Date: Mon, 19 Nov 2007 10:53:40 -0500 (EST)
Subject: Kobject: drop child->parent ref at unregistration
To: Greg KH <greg@kroah.com>, Kay Sievers <kay.sievers@vrfy.org>
Cc: Kernel development list <linux-kernel@vger.kernel.org>
Message-ID: <Pine.LNX.4.44L0.0711191047230.4806-100000@iolanthe.rowland.org>


This patch (as1015) reverts changes that were made to the driver core
about four years ago.  The intent back then was to avoid certain kinds
of invalid memory accesses by leaving kernel objects allocated as long
as any of their children were still allocated.  The original and
correct approach was to wait only as long as any children were still
_registered_; that's what this patch reinstates.

This fixes a problem in the SCSI core made visible by the class_device
to regular device conversion: A reference loop (scsi_device holds
reference to request_queue, which is the child of a gendisk, which is
the child of the scsi_device) prevents the data structures from being
released, even though they are deregistered okay.

It's possible that this change will cause a few bugs to surface,
things that have been hidden for several years.  They can be fixed
easily enough by having the child device take an explicit reference to
the parent whenever needed.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


---
 lib/kobject.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -149,12 +149,16 @@ void kobject_init(struct kobject * kobj)
 
 static void unlink(struct kobject * kobj)
 {
+	struct kobject *parent = kobj->parent;
+
 	if (kobj->kset) {
 		spin_lock(&kobj->kset->list_lock);
 		list_del_init(&kobj->entry);
 		spin_unlock(&kobj->kset->list_lock);
 	}
+	kobj->parent = NULL;
 	kobject_put(kobj);
+	kobject_put(parent);
 }
 
 /**
@@ -208,7 +212,6 @@ int kobject_add(struct kobject * kobj)
 	if (error) {
 		/* unlink does the kobject_put() for us */
 		unlink(kobj);
-		kobject_put(parent);
 
 		/* be noisy on error issues */
 		if (error == -EEXIST)
@@ -463,7 +466,6 @@ void kobject_cleanup(struct kobject * ko
 {
 	struct kobj_type * t = get_ktype(kobj);
 	struct kset * s = kobj->kset;
-	struct kobject * parent = kobj->parent;
 	const char *name = kobj->k_name;
 
 	pr_debug("kobject: '%s' (%p): %s\n",
@@ -477,7 +479,6 @@ void kobject_cleanup(struct kobject * ko
 	}
 	if (s)
 		kset_put(s);
-	kobject_put(parent);
 }
 
 static void kobject_release(struct kref *kref)


Patches currently in gregkh-2.6 which might be from stern@rowland.harvard.edu are

driver/pm-acquire-device-locks-prior-to-suspending.patch
driver/create-sys-...-power-when-config_pm-is-set.patch
driver/driver-core-fix-race-in-__device_release_driver.patch
driver/driver-core-fix-class-glue-dir-cleanup-logic.patch
driver/kobject-drop-child-parent-ref-at-unregistration.patch
usb/usb-add-support-for-an-older-firmware-revision-for-the-nikon-d200.patch
usb/usb-fix-priority-mistakes-in-drivers-usb-core-hub.c.patch
usb/usb-fix-signr-comment-in-usbdevice_fs.h.patch
usb/usb-mailing-lists-have-changed.patch
usb/usb-power-management-documenation-update.patch
usb/usb-hcd-avoid-duplicate-local_irq_disable.patch
usb/usb-usb-mon-mon_bin.c-cleanups.patch
usb/usb-keep-track-of-whether-interface-sysfs-files-exist.patch
usb/usb-uevent-environment-key-fix.patch
usb/usb-autosuspend-for-cdc-acm.patch
usb/usb-fix-up-ehci-startup-synchronization.patch
usb/usb-usb-storage-new-lockable-subclass-0x07.patch
usb/usb-don-t-change-hc-power-state-for-a-freeze.patch
usb/usb-dummy_hcd-don-t-register-drivers-on-the-platform-bus.patch
usb/usb-force-handover-port-to-companion-when-hub_port_connect_change-fails.patch
usb/usb-make-ksuspend_usbd-thread-non-freezable.patch
usb/usb-usb-storage-unusual_devs-entry-for-jetflash-ts1gjf2a.patch
usb/usb-storage-always-set-the-allow_restart-flag.patch