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 <[email protected]>
---
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)
On Mon, 19 Nov 2007 10:53:40 -0500 (EST)
Alan Stern <[email protected]> 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?
On Mon, 26 Nov 2007, Andrew Morton wrote:
> On Mon, 19 Nov 2007 10:53:40 -0500 (EST)
> Alan Stern <[email protected]> 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
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 <[email protected]> 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
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.
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
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
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 [email protected] Thu Nov 29 13:12:08 2007
From: Alan Stern <[email protected]>
Date: Mon, 19 Nov 2007 10:53:40 -0500 (EST)
Subject: Kobject: drop child->parent ref at unregistration
To: Greg KH <[email protected]>, Kay Sievers <[email protected]>
Cc: Kernel development list <[email protected]>
Message-ID: <[email protected]>
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 <[email protected]>
Cc: Kay Sievers <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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 [email protected] 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