Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Tejun Heo]

[adding back linux-usb-devel.  please don't drop cc]
[also adding lkml and Greg K-H]

Chris Rankin wrote:
> --- Tejun Heo <htejun@gmail.com> wrote:
>> Okay, here's patch against 2.6.20.11.  Let's hope we're not chasing
>> something which is already fixed.
> 
> Hooray! The trick was to trace the "dev" file instead of the endpoint directory itself. The
> compressed oops is attached.

Alright, it took a lot of work but the winner is the "dev" node.  Gee...
We could have caught this earlier unless I forgot to print the name of
the last component in the initial debug patch.  Well, better late than
never.  :-(

Anyways, the problem is that the attribute "dev" is dynamically
allocated using kmalloc() in device_add() and freed immediately after
the file is removed in device_del().  It's basically assuming
immediate-disconnect but that apparently isn't true yet.

The dynamic allocation is to set the owner of the attribute to the owner
of the device being registered.  This is twisted in that the field is
added to work around sysfs/module lifetime problem but the workaround
itself is broken with regard to lifetime rules.  Aieeeeeeee...  This
sort of things are why I implemented immediate disconnect on sysfs in
the first place.

So, we can fix the problem Chris is seeing by breaking module unload (by
allowing it to unload too early).  It doesn't sound too hot but module
unloading race is much less likely than sysfs node deletion/open race.

Thanks.

-- 
tejun

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Chris Rankin]

--- Tejun Heo <htejun@gmail.com> wrote:
> So, we can fix the problem Chris is seeing by breaking module unload (by
> allowing it to unload too early).  It doesn't sound too hot but module
> unloading race is much less likely than sysfs node deletion/open race.

Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these
days are things like "microcode", which the init scripts take care of as part of the boot-up
process.

Thanks for all that hard work,
Cheers,
Chris



      ___________________________________________________________
Yahoo! Answers - Got a question? Someone out there knows the answer. Try it
now.
http://uk.answers.yahoo.com/

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

Chris Rankin wrote:
> --- Tejun Heo <htejun@gmail.com> wrote:
>> So, we can fix the problem Chris is seeing by breaking module unload (by
>> allowing it to unload too early).  It doesn't sound too hot but module
>> unloading race is much less likely than sysfs node deletion/open race.
> 
> Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these
> days are things like "microcode", which the init scripts take care of as part of the boot-up
> process.

Okay, here's a half-assed fix.  With this patch applied, if you try to
unload a module while you're opening it's dev attribute, kernel will
oops later when the file is accessed or closed later but it should fix
the bug winecfg triggers.  I really dunno how to fix this the right way
in the stable kernel.  Better ideas?  Anyone?

-- 
tejun

[-- Attachment #2: sysfs-race-fix.patch --]
[-- Type: text/x-patch, Size: 2413 bytes --]

---
 drivers/base/core.c    |   33 ++++++++-------------------------
 include/linux/device.h |    1 -
 2 files changed, 8 insertions(+), 26 deletions(-)

Index: tree0/drivers/base/core.c
===================================================================
--- tree0.orig/drivers/base/core.c
+++ tree0/drivers/base/core.c
@@ -287,6 +287,8 @@ static ssize_t show_dev(struct device *d
 	return print_dev_t(buf, dev->devt);
 }
 
+static struct device_attribute devt_attr = __ATTR(dev, S_IRUGO, show_dev, NULL );
+
 /*
  *	devices_subsys - structure to be registered with kobject core.
  */
@@ -490,24 +492,9 @@ int device_add(struct device *dev)
 		goto attrError;
 
 	if (MAJOR(dev->devt)) {
-		struct device_attribute *attr;
-		attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-		if (!attr) {
-			error = -ENOMEM;
-			goto ueventattrError;
-		}
-		attr->attr.name = "dev";
-		attr->attr.mode = S_IRUGO;
-		if (dev->driver)
-			attr->attr.owner = dev->driver->owner;
-		attr->show = show_dev;
-		error = device_create_file(dev, attr);
-		if (error) {
-			kfree(attr);
+		error = device_create_file(dev, &devt_attr);
+		if (error)
 			goto ueventattrError;
-		}
-
-		dev->devt_attr = attr;
 	}
 
 	if (dev->class) {
@@ -568,10 +555,8 @@ int device_add(struct device *dev)
  GroupError:
  	device_remove_attrs(dev);
  AttrsError:
-	if (dev->devt_attr) {
-		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
+	if (MAJOR(dev->devt))
+		device_remove_file(dev, &devt_attr);
  ueventattrError:
 	device_remove_file(dev, &dev->uevent_attr);
  attrError:
@@ -650,10 +635,8 @@ void device_del(struct device * dev)
 
 	if (parent)
 		klist_del(&dev->knode_parent);
-	if (dev->devt_attr) {
-		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
+	if (MAJOR(dev->devt))
+		device_remove_file(dev, &devt_attr);
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
 		/* If this is not a "fake" compatible device, remove the
Index: tree0/include/linux/device.h
===================================================================
--- tree0.orig/include/linux/device.h
+++ tree0/include/linux/device.h
@@ -357,7 +357,6 @@ struct device {
 	char	bus_id[BUS_ID_SIZE];	/* position on parent bus */
 	unsigned		is_registered:1;
 	struct device_attribute uevent_attr;
-	struct device_attribute *devt_attr;
 
 	struct semaphore	sem;	/* semaphore to synchronize calls to
 					 * its driver.

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Chris Rankin]

--- Tejun Heo <htejun@gmail.com> wrote:
> Okay, here's a half-assed fix.  With this patch applied, if you try to
> unload a module while you're opening it's dev attribute, kernel will
> oops later when the file is accessed or closed later but it should fix
> the bug winecfg triggers.  I really dunno how to fix this the right way
> in the stable kernel.  Better ideas?  Anyone?

How about a WARN() and a small(?) memory leak? Better than an oops, surely?

Cheers,
Chris



      ___________________________________________________________ 
Yahoo! Mail is the world's favourite email. Don't settle for less, sign up for
your free account today http://uk.rd.yahoo.com/evt=44106/*http://uk.docs.yahoo.com/mail/winter07.html

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Tejun Heo]

Chris Rankin wrote:
> --- Tejun Heo <htejun@gmail.com> wrote:
>> Okay, here's a half-assed fix.  With this patch applied, if you try to
>> unload a module while you're opening it's dev attribute, kernel will
>> oops later when the file is accessed or closed later but it should fix
>> the bug winecfg triggers.  I really dunno how to fix this the right way
>> in the stable kernel.  Better ideas?  Anyone?
> 
> How about a WARN() and a small(?) memory leak? Better than an oops, surely?

Device node creation/deletion can be quite often depending on
configuration, so I don't think we can afford memory leak here.  It can
develop into a big problem for long running hosts.  IMHO, just
introducing module unload/deletion race is much better.  It's the lesser
evil, difficult to trigger and already broken in other places anyway.

I think we need to hear what other people are thinking about it.  Cc'ing
Maneesh, Dmitry and Cornelia.  The whole thread can be read at...

  http://thread.gmane.org/gmane.linux.usb.devel/53559
  http://thread.gmane.org/gmane.linux.usb.devel/53846

The thread is rather long but just reading the message from the second
URL should be enough.  The problem is that dev->devt_attr (class dev has
the same problem) is deallocated when the device is deleted.  If the dev
sysfs attribute has users at that point, the dev sysfs node is left with
garbled struct attribute causing oops later.

IMHO, the proper fix for this is immediate-disconnect which is no in -mm
as the problem is caused by expecting immediate-disconnect behavior when
it isn't implemented.

As written above, I think it's better to risk module unload / sysfs race
than keeping the current sysfs deletion / open race.  What do you guys
think?

Thanks.

-- 
tejun

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Dmitry Torokhov]

Hi Tejun,

On 5/9/07, Tejun Heo <htejun@gmail.com> wrote:
> Chris Rankin wrote:
> > --- Tejun Heo <htejun@gmail.com> wrote:
> >> Okay, here's a half-assed fix.  With this patch applied, if you try to
> >> unload a module while you're opening it's dev attribute, kernel will
> >> oops later when the file is accessed or closed later but it should fix
> >> the bug winecfg triggers.  I really dunno how to fix this the right way
> >> in the stable kernel.  Better ideas?  Anyone?
> >
> > How about a WARN() and a small(?) memory leak? Better than an oops, surely?
>
> Device node creation/deletion can be quite often depending on
> configuration, so I don't think we can afford memory leak here.  It can
> develop into a big problem for long running hosts.  IMHO, just
> introducing module unload/deletion race is much better.  It's the lesser
> evil, difficult to trigger and already broken in other places anyway.
>
> I think we need to hear what other people are thinking about it.  Cc'ing
> Maneesh, Dmitry and Cornelia.  The whole thread can be read at...
>
>  http://thread.gmane.org/gmane.linux.usb.devel/53559
>  http://thread.gmane.org/gmane.linux.usb.devel/53846
>
> The thread is rather long but just reading the message from the second
> URL should be enough.  The problem is that dev->devt_attr (class dev has
> the same problem) is deallocated when the device is deleted.  If the dev
> sysfs attribute has users at that point, the dev sysfs node is left with
> garbled struct attribute causing oops later.
>
> IMHO, the proper fix for this is immediate-disconnect which is no in -mm
> as the problem is caused by expecting immediate-disconnect behavior when
> it isn't implemented.
>
> As written above, I think it's better to risk module unload / sysfs race
> than keeping the current sysfs deletion / open race.  What do you guys
> think?
>

How about embedding struct attribute fro devt into struct
[class_]device for now? It is not too big and device is still going to
be pinned into memory while there are sysfs users... I don't like
fattening of device structures but leaks and/or oopses are worse in my
book.

-- 
Dmitry

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

Dmitry Torokhov wrote:
>> As written above, I think it's better to risk module unload / sysfs race
>> than keeping the current sysfs deletion / open race.  What do you guys
>> think?
>>
> 
> How about embedding struct attribute fro devt into struct
> [class_]device for now? It is not too big and device is still going to
> be pinned into memory while there are sysfs users... I don't like
> fattening of device structures but leaks and/or oopses are worse in my
> book.

Right, your book is apparently much better than mine.  Actually, we can
just free devt_attr in device_release().  Looking at the code, class
device is already doing it that way, so here's the full-assed fix.
Chris, can you please test the attached patch?

Thanks.

-- 
tejun

[-- Attachment #2: fix-devt_attr-release --]
[-- Type: text/plain, Size: 857 bytes --]

---
 drivers/base/core.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: tree0/drivers/base/core.c
===================================================================
--- tree0.orig/drivers/base/core.c
+++ tree0/drivers/base/core.c
@@ -93,6 +93,9 @@ static void device_release(struct kobjec
 {
 	struct device * dev = to_dev(kobj);
 
+	kfree(dev->devt_attr);
+	dev->devt_attr = NULL;
+
 	if (dev->release)
 		dev->release(dev);
 	else if (dev->class && dev->class->dev_release)
@@ -650,10 +653,8 @@ void device_del(struct device * dev)
 
 	if (parent)
 		klist_del(&dev->knode_parent);
-	if (dev->devt_attr) {
+	if (dev->devt_attr)
 		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
 		/* If this is not a "fake" compatible device, remove the

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Chris Rankin]

--- Tejun Heo <htejun@gmail.com> wrote:
> Looking at the code, class
> device is already doing it that way, so here's the full-assed fix.
> Chris, can you please test the attached patch?

Tejun,

So far so good; my 2.6.20.11+patch kernel hasn't oopsed yet. I'm going to start using this kernel
as my default boot and see how it goes.

Cheers,
Chris


      ___________________________________________________________
Yahoo! Answers - Got a question? Someone out there knows the answer. Try it
now.
http://uk.answers.yahoo.com/

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Greg KH]

On Wed, May 09, 2007 at 03:30:41PM +0200, Tejun Heo wrote:
> Chris Rankin wrote:
> > --- Tejun Heo <htejun@gmail.com> wrote:
> >> So, we can fix the problem Chris is seeing by breaking module unload (by
> >> allowing it to unload too early).  It doesn't sound too hot but module
> >> unloading race is much less likely than sysfs node deletion/open race.
> > 
> > Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these
> > days are things like "microcode", which the init scripts take care of as part of the boot-up
> > process.
> 
> Okay, here's a half-assed fix.  With this patch applied, if you try to
> unload a module while you're opening it's dev attribute, kernel will
> oops later when the file is accessed or closed later but it should fix
> the bug winecfg triggers.  I really dunno how to fix this the right way
> in the stable kernel.  Better ideas?  Anyone?

This looks like the correct fix, the only reason I made this dynamic was
to properly set up the module owner.  And now that you have removed that
link in your sysfs rework, it can go and become static again.

Oh, and this isn't a "device node", it's just a text file that contains
the major:minor number for a device node to be created from.

So, care to fix up the class code too and send a patch with a
signed-off-by:?

thanks,

greg k-h

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Tejun Heo]

Greg KH wrote:
> On Wed, May 09, 2007 at 03:30:41PM +0200, Tejun Heo wrote:
>> Chris Rankin wrote:
>>> --- Tejun Heo <htejun@gmail.com> wrote:
>>>> So, we can fix the problem Chris is seeing by breaking module unload (by
>>>> allowing it to unload too early).  It doesn't sound too hot but module
>>>> unloading race is much less likely than sysfs node deletion/open race.
>>> Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these
>>> days are things like "microcode", which the init scripts take care of as part of the boot-up
>>> process.
>> Okay, here's a half-assed fix.  With this patch applied, if you try to
>> unload a module while you're opening it's dev attribute, kernel will
>> oops later when the file is accessed or closed later but it should fix
>> the bug winecfg triggers.  I really dunno how to fix this the right way
>> in the stable kernel.  Better ideas?  Anyone?
> 
> This looks like the correct fix, the only reason I made this dynamic was
> to properly set up the module owner.  And now that you have removed that
> link in your sysfs rework, it can go and become static again.
> 
> Oh, and this isn't a "device node", it's just a text file that contains
> the major:minor number for a device node to be created from.
> 
> So, care to fix up the class code too and send a patch with a
> signed-off-by:?

I think the proper fix for -stable is to free the structure in
device_release().  I asked Chris to test it.  After he confirms, I'll
send a proper patch with S-O-B.

For -mm, yeap, we can simply make the whole thing static.  I'll send
patch for that soon.

Thanks.

-- 
tejun

Re: [linux-usb-devel] Bug creating USB endpoints in 2.6.20.x (kernel bug 8198)

[Greg KH]

On Wed, May 09, 2007 at 05:01:25PM +0200, Tejun Heo wrote:
> Greg KH wrote:
> > On Wed, May 09, 2007 at 03:30:41PM +0200, Tejun Heo wrote:
> >> Chris Rankin wrote:
> >>> --- Tejun Heo <htejun@gmail.com> wrote:
> >>>> So, we can fix the problem Chris is seeing by breaking module unload (by
> >>>> allowing it to unload too early).  It doesn't sound too hot but module
> >>>> unloading race is much less likely than sysfs node deletion/open race.
> >>> Yikes! Just temporary breakage, I hope :-)!! The only modules I unload on a regular basis these
> >>> days are things like "microcode", which the init scripts take care of as part of the boot-up
> >>> process.
> >> Okay, here's a half-assed fix.  With this patch applied, if you try to
> >> unload a module while you're opening it's dev attribute, kernel will
> >> oops later when the file is accessed or closed later but it should fix
> >> the bug winecfg triggers.  I really dunno how to fix this the right way
> >> in the stable kernel.  Better ideas?  Anyone?
> > 
> > This looks like the correct fix, the only reason I made this dynamic was
> > to properly set up the module owner.  And now that you have removed that
> > link in your sysfs rework, it can go and become static again.
> > 
> > Oh, and this isn't a "device node", it's just a text file that contains
> > the major:minor number for a device node to be created from.
> > 
> > So, care to fix up the class code too and send a patch with a
> > signed-off-by:?
> 
> I think the proper fix for -stable is to free the structure in
> device_release().  I asked Chris to test it.  After he confirms, I'll
> send a proper patch with S-O-B.

Ah, yes, that's the correct fix for this.

thanks,

greg k-h

[PATCH] driver-core: don't free devt_attr till the device is released

[Tejun Heo]

Currently, devt_attr for the "dev" file is freed immediately on device
removal, but if the "dev" sysfs file is open when a device is removed,
sysfs will access its attribute structure for further access including
close resulting in jumping to garbled address.  Fix it by postponing
freeing devt_attr to device release time.

Note that devt_attr for class_device is already freed on release.

This bug is reported by Chris Rankin as bugzilla bug#8198.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Chris Rankin <rankincj@yahoo.com>
---
Applies well to 2.6.20 and 21.  As sysfs-immediate-disconnect doesn't
seem to be included in 2.6.22, this should be included in linus#master
too (applies well there as well).

* This is the second post.  Something went wrong with the recipients
  list on the first posting.  Both are same.

Thanks.

 drivers/base/core.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: tree0/drivers/base/core.c
===================================================================
--- tree0.orig/drivers/base/core.c
+++ tree0/drivers/base/core.c
@@ -93,6 +93,9 @@ static void device_release(struct kobjec
 {
 	struct device * dev = to_dev(kobj);
 
+	kfree(dev->devt_attr);
+	dev->devt_attr = NULL;
+
 	if (dev->release)
 		dev->release(dev);
 	else if (dev->class && dev->class->dev_release)
@@ -650,10 +653,8 @@ void device_del(struct device * dev)
 
 	if (parent)
 		klist_del(&dev->knode_parent);
-	if (dev->devt_attr) {
+	if (dev->devt_attr)
 		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
 		/* If this is not a "fake" compatible device, remove the

Re: [PATCH] driver-core: don't free devt_attr till the device is released

[Greg KH]

On Thu, May 10, 2007 at 04:45:17PM +0200, Tejun Heo wrote:
> Currently, devt_attr for the "dev" file is freed immediately on device
> removal, but if the "dev" sysfs file is open when a device is removed,
> sysfs will access its attribute structure for further access including
> close resulting in jumping to garbled address.  Fix it by postponing
> freeing devt_attr to device release time.
> 
> Note that devt_attr for class_device is already freed on release.
> 
> This bug is reported by Chris Rankin as bugzilla bug#8198.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Chris Rankin <rankincj@yahoo.com>
> ---
> Applies well to 2.6.20 and 21.  As sysfs-immediate-disconnect doesn't
> seem to be included in 2.6.22, this should be included in linus#master
> too (applies well there as well).

As I don't think we should be adding your sysfs rework to 2.6.22 just
yet, any objections to me just sending this to Linus for 2.6.22 and
waiting on your previous one for when the whole sysfs rework patchset is
sent?

thanks,

greg k-h

Re: [PATCH] driver-core: don't free devt_attr till the device is released

[Tejun Heo]

Greg KH wrote:
>> Applies well to 2.6.20 and 21.  As sysfs-immediate-disconnect doesn't
>> seem to be included in 2.6.22, this should be included in linus#master
>> too (applies well there as well).
> 
> As I don't think we should be adding your sysfs rework to 2.6.22 just
> yet, any objections to me just sending this to Linus for 2.6.22 and
> waiting on your previous one for when the whole sysfs rework patchset is
> sent?

No objection at all.  That actually is exactly my intention.  The
make-attrs-static patch will conflict with this fix tho.  Just let me
know when it breaks, I'll post updated one.

Thanks.

-- 
tejun

Re: [PATCH] driver-core: don't free devt_attr till the device is released

[Greg KH]

On Thu, May 10, 2007 at 05:13:10PM +0200, Tejun Heo wrote:
> Greg KH wrote:
> >> Applies well to 2.6.20 and 21.  As sysfs-immediate-disconnect doesn't
> >> seem to be included in 2.6.22, this should be included in linus#master
> >> too (applies well there as well).
> > 
> > As I don't think we should be adding your sysfs rework to 2.6.22 just
> > yet, any objections to me just sending this to Linus for 2.6.22 and
> > waiting on your previous one for when the whole sysfs rework patchset is
> > sent?
> 
> No objection at all.  That actually is exactly my intention.  The
> make-attrs-static patch will conflict with this fix tho.  Just let me
> know when it breaks, I'll post updated one.

If it breaks, I'll fix up the merge issues, don't worry about it :)

Thanks again for taking the time and tracking it down and fixing this.

greg k-h

Re: [PATCH] driver-core: don't free devt_attr till the device is released

[Kay Sievers]

On 5/10/07, Tejun Heo <htejun@gmail.com> wrote:
> Currently, devt_attr for the "dev" file is freed immediately on device
> removal, but if the "dev" sysfs file is open when a device is removed,
> sysfs will access its attribute structure for further access including
> close resulting in jumping to garbled address.  Fix it by postponing
> freeing devt_attr to device release time.
>
> Note that devt_attr for class_device is already freed on release.

Hi Tejun,
your rework removes the "owner" field from the attributes. I think we
kept the "dev" and "uevent" attribute as part of "struct device" only
to be able to assign it the actual owner of the module that has
created the device. The attribute can probably just live as one
instance statically in the driver core now?

Thanks,
Kay

Re: [PATCH] driver-core: don't free devt_attr till the device is released

[Tejun Heo]

Kay Sievers wrote:
> On 5/10/07, Tejun Heo <htejun@gmail.com> wrote:
>> Currently, devt_attr for the "dev" file is freed immediately on device
>> removal, but if the "dev" sysfs file is open when a device is removed,
>> sysfs will access its attribute structure for further access including
>> close resulting in jumping to garbled address.  Fix it by postponing
>> freeing devt_attr to device release time.
>>
>> Note that devt_attr for class_device is already freed on release.
> 
> Hi Tejun,
> your rework removes the "owner" field from the attributes. I think we
> kept the "dev" and "uevent" attribute as part of "struct device" only
> to be able to assign it the actual owner of the module that has
> created the device. The attribute can probably just live as one
> instance statically in the driver core now?

Yeah, that's -mm and this is for -stable and -22.  For -mm, we can just
make all those attributes static which is the other patch is this
thread.  :-)

-- 
tejun

Re: [PATCH] driver-core: don't free devt_attr till the device is released

[Alan Stern]

On Thu, 10 May 2007, Tejun Heo wrote:

> Currently, devt_attr for the "dev" file is freed immediately on device
> removal, but if the "dev" sysfs file is open when a device is removed,
> sysfs will access its attribute structure for further access including
> close resulting in jumping to garbled address.  Fix it by postponing
> freeing devt_attr to device release time.
> 
> Note that devt_attr for class_device is already freed on release.
> 
> This bug is reported by Chris Rankin as bugzilla bug#8198.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Chris Rankin <rankincj@yahoo.com>
> ---
> Applies well to 2.6.20 and 21.  As sysfs-immediate-disconnect doesn't
> seem to be included in 2.6.22, this should be included in linus#master
> too (applies well there as well).

Although sysfs-immediate-disconnect may not be included in 2.6.22, the old 
attribute-orphan code by Oliver Neukum is present there and also in 
2.6.21.  Shouldn't that suffice?

Alan Stern

Re: [PATCH] driver-core: don't free devt_attr till the device is released

[Tejun Heo]

Alan Stern wrote:
> On Thu, 10 May 2007, Tejun Heo wrote:
> 
>> Currently, devt_attr for the "dev" file is freed immediately on device
>> removal, but if the "dev" sysfs file is open when a device is removed,
>> sysfs will access its attribute structure for further access including
>> close resulting in jumping to garbled address.  Fix it by postponing
>> freeing devt_attr to device release time.
>>
>> Note that devt_attr for class_device is already freed on release.
>>
>> This bug is reported by Chris Rankin as bugzilla bug#8198.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> Cc: Chris Rankin <rankincj@yahoo.com>
>> ---
>> Applies well to 2.6.20 and 21.  As sysfs-immediate-disconnect doesn't
>> seem to be included in 2.6.22, this should be included in linus#master
>> too (applies well there as well).
> 
> Although sysfs-immediate-disconnect may not be included in 2.6.22, the old 
> attribute-orphan code by Oliver Neukum is present there and also in 
> 2.6.21.  Shouldn't that suffice?

sysfs_release() still needs to deference attr->owner to put it, so I
think there's the same problem even with attribute-orphan.  We end up
calling module_put() on garbage.

-- 
tejun

[PATCH 2.6.21-mm2] driver-core: make devt_attr and uevent_attr static

[Tejun Heo]

devt_attr and uevent_attr are either allocated dynamically with or
embedded in device and class_device as they needed their owner field
set to the module implementing the driver.  Now that sysfs implements
immediate disconnect and owner field removed from struct attribute,
there is no reason to do this.  Remove these attributes from
[class_]device and use static attribute structures instead.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/base/class.c   |   44 ++++++++++++++++----------------------------
 drivers/base/core.c    |   45 +++++++++++++++------------------------------
 include/linux/device.h |    5 -----
 3 files changed, 31 insertions(+), 63 deletions(-)

Index: tree0/drivers/base/class.c
===================================================================
--- tree0.orig/drivers/base/class.c
+++ tree0/drivers/base/class.c
@@ -312,9 +312,6 @@ static void class_dev_release(struct kob
 
 	pr_debug("device class '%s': release.\n", cd->class_id);
 
-	kfree(cd->devt_attr);
-	cd->devt_attr = NULL;
-
 	if (cd->release)
 		cd->release(cd);
 	else if (cls->release)
@@ -564,6 +561,9 @@ static ssize_t show_dev(struct class_dev
 	return print_dev_t(buf, class_dev->devt);
 }
 
+static struct class_device_attribute class_devt_attr =
+	__ATTR(dev, S_IRUGO, show_dev, NULL);
+
 static ssize_t store_uevent(struct class_device *class_dev,
 			    const char *buf, size_t count)
 {
@@ -571,6 +571,9 @@ static ssize_t store_uevent(struct class
 	return count;
 }
 
+static struct class_device_attribute class_uevent_attr =
+	__ATTR(uevent, S_IWUSR, NULL, store_uevent);
+
 void class_device_initialize(struct class_device *class_dev)
 {
 	kobj_set_kset_s(class_dev, class_obj_subsys);
@@ -620,30 +623,15 @@ int class_device_add(struct class_device
 				  &parent_class->subsys.kobj, "subsystem");
 	if (error)
 		goto out3;
-	class_dev->uevent_attr.attr.name = "uevent";
-	class_dev->uevent_attr.attr.mode = S_IWUSR;
-	class_dev->uevent_attr.store = store_uevent;
-	error = class_device_create_file(class_dev, &class_dev->uevent_attr);
+
+	error = class_device_create_file(class_dev, &class_uevent_attr);
 	if (error)
 		goto out3;
 
 	if (MAJOR(class_dev->devt)) {
-		struct class_device_attribute *attr;
-		attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-		if (!attr) {
-			error = -ENOMEM;
-			goto out4;
-		}
-		attr->attr.name = "dev";
-		attr->attr.mode = S_IRUGO;
-		attr->show = show_dev;
-		error = class_device_create_file(class_dev, attr);
-		if (error) {
-			kfree(attr);
+		error = class_device_create_file(class_dev, &class_devt_attr);
+		if (error)
 			goto out4;
-		}
-
-		class_dev->devt_attr = attr;
 	}
 
 	error = class_device_add_attrs(class_dev);
@@ -686,10 +674,10 @@ int class_device_add(struct class_device
  out6:
 	class_device_remove_attrs(class_dev);
  out5:
-	if (class_dev->devt_attr)
-		class_device_remove_file(class_dev, class_dev->devt_attr);
+	if (MAJOR(class_dev->devt))
+		class_device_remove_file(class_dev, &class_devt_attr);
  out4:
-	class_device_remove_file(class_dev, &class_dev->uevent_attr);
+	class_device_remove_file(class_dev, &class_uevent_attr);
  out3:
 	kobject_del(&class_dev->kobj);
  out2:
@@ -789,9 +777,9 @@ void class_device_del(struct class_devic
 		sysfs_remove_link(&class_dev->kobj, "device");
 	}
 	sysfs_remove_link(&class_dev->kobj, "subsystem");
-	class_device_remove_file(class_dev, &class_dev->uevent_attr);
-	if (class_dev->devt_attr)
-		class_device_remove_file(class_dev, class_dev->devt_attr);
+	class_device_remove_file(class_dev, &class_uevent_attr);
+	if (MAJOR(class_dev->devt))
+		class_device_remove_file(class_dev, &class_devt_attr);
 	class_device_remove_attrs(class_dev);
 	class_device_remove_groups(class_dev);
 
Index: tree0/drivers/base/core.c
===================================================================
--- tree0.orig/drivers/base/core.c
+++ tree0/drivers/base/core.c
@@ -308,6 +308,9 @@ static ssize_t store_uevent(struct devic
 	return count;
 }
 
+static struct device_attribute uevent_attr =
+	__ATTR(uevent, S_IRUGO | S_IWUSR, show_uevent, store_uevent);
+
 static int device_add_attributes(struct device *dev,
 				 struct device_attribute *attrs)
 {
@@ -421,6 +424,9 @@ static ssize_t show_dev(struct device *d
 	return print_dev_t(buf, dev->devt);
 }
 
+static struct device_attribute devt_attr =
+	__ATTR(dev, S_IRUGO, show_dev, NULL);
+
 /*
  *	devices_subsys - structure to be registered with kobject core.
  */
@@ -679,31 +685,14 @@ int device_add(struct device *dev)
 		blocking_notifier_call_chain(&dev->bus->bus_notifier,
 					     BUS_NOTIFY_ADD_DEVICE, dev);
 
-	dev->uevent_attr.attr.name = "uevent";
-	dev->uevent_attr.attr.mode = S_IRUGO | S_IWUSR;
-	dev->uevent_attr.store = store_uevent;
-	dev->uevent_attr.show = show_uevent;
-	error = device_create_file(dev, &dev->uevent_attr);
+	error = device_create_file(dev, &uevent_attr);
 	if (error)
 		goto attrError;
 
 	if (MAJOR(dev->devt)) {
-		struct device_attribute *attr;
-		attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-		if (!attr) {
-			error = -ENOMEM;
-			goto ueventattrError;
-		}
-		attr->attr.name = "dev";
-		attr->attr.mode = S_IRUGO;
-		attr->show = show_dev;
-		error = device_create_file(dev, attr);
-		if (error) {
-			kfree(attr);
+		error = device_create_file(dev, &devt_attr);
+		if (error)
 			goto ueventattrError;
-		}
-
-		dev->devt_attr = attr;
 	}
 
 	if (dev->class) {
@@ -761,10 +750,8 @@ int device_add(struct device *dev)
 					     BUS_NOTIFY_DEL_DEVICE, dev);
 	device_remove_attrs(dev);
  AttrsError:
-	if (dev->devt_attr) {
-		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
+	if (MAJOR(dev->devt))
+		device_remove_file(dev, &devt_attr);
 
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
@@ -786,7 +773,7 @@ int device_add(struct device *dev)
 		}
 	}
  ueventattrError:
-	device_remove_file(dev, &dev->uevent_attr);
+	device_remove_file(dev, &uevent_attr);
  attrError:
 	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
 	kobject_del(&dev->kobj);
@@ -864,10 +851,8 @@ void device_del(struct device * dev)
 
 	if (parent)
 		klist_del(&dev->knode_parent);
-	if (dev->devt_attr) {
-		device_remove_file(dev, dev->devt_attr);
-		kfree(dev->devt_attr);
-	}
+	if (MAJOR(dev->devt))
+		device_remove_file(dev, &devt_attr);
 	if (dev->class) {
 		sysfs_remove_link(&dev->kobj, "subsystem");
 		/* If this is not a "fake" compatible device, remove the
@@ -921,7 +906,7 @@ void device_del(struct device * dev)
 			up(&dev->class->sem);
 		}
 	}
-	device_remove_file(dev, &dev->uevent_attr);
+	device_remove_file(dev, &uevent_attr);
 	device_remove_attrs(dev);
 	bus_remove_device(dev);
 
Index: tree0/include/linux/device.h
===================================================================
--- tree0.orig/include/linux/device.h
+++ tree0/include/linux/device.h
@@ -240,7 +240,6 @@ extern int __must_check class_device_cre
  * @devt: for internal use by the driver core only.
  * @node: for internal use by the driver core only.
  * @kobj: for internal use by the driver core only.
- * @devt_attr: for internal use by the driver core only.
  * @groups: optional additional groups to be created
  * @dev: if set, a symlink to the struct device is created in the sysfs
  * directory for this struct class device.
@@ -265,8 +264,6 @@ struct class_device {
 	struct kobject		kobj;
 	struct class		* class;	/* required */
 	dev_t			devt;		/* dev_t, creates the sysfs "dev" */
-	struct class_device_attribute *devt_attr;
-	struct class_device_attribute uevent_attr;
 	struct device		* dev;		/* not necessary, but nice to have */
 	void			* class_data;	/* class-specific data */
 	struct class_device	*parent;	/* parent of this child device, if there is one */
@@ -421,8 +418,6 @@ struct device {
 	struct device_type	*type;
 	unsigned		is_registered:1;
 	unsigned		uevent_suppress:1;
-	struct device_attribute uevent_attr;
-	struct device_attribute *devt_attr;
 
 	struct semaphore	sem;	/* semaphore to synchronize calls to
 					 * its driver.