Hello, Greg, Kay, Guthrie.
This problem was reported in bko#14023. (aiee.. again, sorry about
the delay)
http://bugzilla.kernel.org/show_bug.cgi?id=14023
sysfs is creating several devices in cuse class concurrently and with
CONFIG_SYSFS_DEPRECATED turned off, it triggers the following oops.
BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
IP: [<ffffffff81158b0a>] sysfs_addrm_start+0x4a/0xf0
PGD 75bb067 PUD 75be067 PMD 0
Oops: 0000 [#1] PREEMPT SMP
last sysfs file: /sys/devices/system/cpu/cpu7/topology/core_siblings
CPU 1
Modules linked in: cuse fuse
Pid: 4737, comm: osspd Not tainted 2.6.31-work #77
RIP: 0010:[<ffffffff81158b0a>] [<ffffffff81158b0a>] sysfs_addrm_start+0x4a/0xf0
RSP: 0018:ffff88000042f8f8 EFLAGS: 00010296
RAX: ffff88000042ffd8 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff880007eef660 RDI: 0000000000000001
RBP: ffff88000042f918 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000001 R11: ffffffff81158b0a R12: ffff88000042f928
R13: 00000000fffffff4 R14: 0000000000000000 R15: ffff88000042f9a0
FS: 00007fe93905a950(0000) GS:ffff880008600000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000038 CR3: 00000000077c9000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process osspd (pid: 4737, threadinfo ffff88000042e000, task ffff880007eef040)
Stack:
ffff880005da10e8 0000000011cc8d6e ffff88000042f928 ffff880003d28a28
<0> ffff88000042f988 ffffffff811592d7 0000000000000000 0000000000000000
<0> 0000000000000000 0000000000000000 ffff88000042f958 0000000011cc8d6e
Call Trace:
[<ffffffff811592d7>] create_dir+0x67/0xe0
[<ffffffff811593a8>] sysfs_create_dir+0x58/0xb0
[<ffffffff8128ca7c>] ? kobject_add_internal+0xcc/0x220
[<ffffffff812942e1>] ? vsnprintf+0x3c1/0xb90
[<ffffffff8128cab7>] kobject_add_internal+0x107/0x220
[<ffffffff8128cd37>] kobject_add_varg+0x47/0x80
[<ffffffff8128ce53>] kobject_add+0x53/0x90
[<ffffffff81357d84>] device_add+0xd4/0x690
[<ffffffff81356c2b>] ? dev_set_name+0x4b/0x70
[<ffffffffa001a884>] cuse_process_init_reply+0x2b4/0x420 [cuse]
...
The problem is that kobject_add_internal() first adds a kobject to the
kset and then try to create sysfs directory for it. If the creation
fails, it remove the kobject from the kset. get_device_parent()
accesses class_dirs kset while only holding class_dirs.list_lock to
see whether the cuse class dir exists. But when it exists, it may not
have finished initialization yet or may fail and get removed soon. In
the above case, the former happened so the second one ends up trying
to create subdirectory under null sysfs_dirent.
Patch attached at the end of this email solves this problem in an ugly
way.
<rant>
One of the things I really dislike about these k* stuff is that the
API advertises much more than they're actually capable of. The
encapsulated synchronization might look like a good idea but it fails
horribly in practice because these things are always used in
connection with other objects. The API just tricks developers to
think that all the complexities have been handled and nicely
encapsulated when the reality is those are just hidden under giant
pile of abstract crap. I really hope someday driver model can do away
with all these k* stuff and use just necessary amount of abstraction
like other sane kernel subsystems. :-(
</rant>
Thanks.
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6bee6af..71938ed 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -596,6 +596,7 @@ static struct kobject *get_device_parent(struct device *dev,
int retval;
if (dev->class) {
+ static DEFINE_MUTEX(gdp_mutex);
struct kobject *kobj = NULL;
struct kobject *parent_kobj;
struct kobject *k;
@@ -612,6 +613,8 @@ static struct kobject *get_device_parent(struct device *dev,
else
parent_kobj = &parent->kobj;
+ mutex_lock(&gdp_mutex);
+
/* find our class-directory at the parent and reference it */
spin_lock(&dev->class->p->class_dirs.list_lock);
list_for_each_entry(k, &dev->class->p->class_dirs.list, entry)
@@ -620,20 +623,26 @@ static struct kobject *get_device_parent(struct device *dev,
break;
}
spin_unlock(&dev->class->p->class_dirs.list_lock);
- if (kobj)
+ if (kobj) {
+ mutex_unlock(&gdp_mutex);
return kobj;
+ }
/* or create a new class-directory at the parent device */
k = kobject_create();
- if (!k)
+ if (!k) {
+ mutex_unlock(&gdp_mutex);
return NULL;
+ }
k->kset = &dev->class->p->class_dirs;
retval = kobject_add(k, parent_kobj, "%s", dev->class->name);
if (retval < 0) {
+ mutex_unlock(&gdp_mutex);
kobject_put(k);
return NULL;
}
/* do not emit an uevent for this simple "glue" directory */
+ mutex_unlock(&gdp_mutex);
return k;
}
--
tejun
Tejun Heo wrote:
> Hello, Greg, Kay, Guthrie.
>
> This problem was reported in bko#14023. (aiee.. again, sorry about
> the delay)
>
> http://bugzilla.kernel.org/show_bug.cgi?id=14023
>
> sysfs is creating several devices in cuse class concurrently and with
> CONFIG_SYSFS_DEPRECATED turned off, it triggers the following oops.
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
Ping. This one needs to be fixed in -stable. It can be triggered by
other char devices too.
Thanks.
--
tejun
On Sun, Oct 04, 2009 at 09:35:35AM +0900, Tejun Heo wrote:
> Tejun Heo wrote:
> > Hello, Greg, Kay, Guthrie.
> >
> > This problem was reported in bko#14023. (aiee.. again, sorry about
> > the delay)
> >
> > http://bugzilla.kernel.org/show_bug.cgi?id=14023
> >
> > sysfs is creating several devices in cuse class concurrently and with
> > CONFIG_SYSFS_DEPRECATED turned off, it triggers the following oops.
> >
> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
>
> Ping. This one needs to be fixed in -stable. It can be triggered by
> other char devices too.
Sorry, been slowly catching up...
This can be triggered by char devices? Huh? How? I don't see the
failure path that is happening here.
And char devices shouldn't really be using the kobject at all, except
for a very basic reference count.
I keep threatening to rip kobject out of a char device and just use a
kref, as that is all that is really needed. Well, that and the kmap
stuff, but again, it's not a "real" kobject being used there...
Perhaps now is the time to do this.
thanks,
greg k-h
On Fri, Sep 25, 2009 at 05:22:13PM +0900, Tejun Heo wrote:
> Hello, Greg, Kay, Guthrie.
>
> This problem was reported in bko#14023. (aiee.. again, sorry about
> the delay)
>
> http://bugzilla.kernel.org/show_bug.cgi?id=14023
>
> sysfs is creating several devices in cuse class concurrently and with
> CONFIG_SYSFS_DEPRECATED turned off, it triggers the following oops.
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
> IP: [<ffffffff81158b0a>] sysfs_addrm_start+0x4a/0xf0
> PGD 75bb067 PUD 75be067 PMD 0
> Oops: 0000 [#1] PREEMPT SMP
> last sysfs file: /sys/devices/system/cpu/cpu7/topology/core_siblings
> CPU 1
> Modules linked in: cuse fuse
> Pid: 4737, comm: osspd Not tainted 2.6.31-work #77
> RIP: 0010:[<ffffffff81158b0a>] [<ffffffff81158b0a>] sysfs_addrm_start+0x4a/0xf0
> RSP: 0018:ffff88000042f8f8 EFLAGS: 00010296
> RAX: ffff88000042ffd8 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff880007eef660 RDI: 0000000000000001
> RBP: ffff88000042f918 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000001 R11: ffffffff81158b0a R12: ffff88000042f928
> R13: 00000000fffffff4 R14: 0000000000000000 R15: ffff88000042f9a0
> FS: 00007fe93905a950(0000) GS:ffff880008600000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 0000000000000038 CR3: 00000000077c9000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process osspd (pid: 4737, threadinfo ffff88000042e000, task ffff880007eef040)
> Stack:
> ffff880005da10e8 0000000011cc8d6e ffff88000042f928 ffff880003d28a28
> <0> ffff88000042f988 ffffffff811592d7 0000000000000000 0000000000000000
> <0> 0000000000000000 0000000000000000 ffff88000042f958 0000000011cc8d6e
> Call Trace:
> [<ffffffff811592d7>] create_dir+0x67/0xe0
> [<ffffffff811593a8>] sysfs_create_dir+0x58/0xb0
> [<ffffffff8128ca7c>] ? kobject_add_internal+0xcc/0x220
> [<ffffffff812942e1>] ? vsnprintf+0x3c1/0xb90
> [<ffffffff8128cab7>] kobject_add_internal+0x107/0x220
> [<ffffffff8128cd37>] kobject_add_varg+0x47/0x80
> [<ffffffff8128ce53>] kobject_add+0x53/0x90
> [<ffffffff81357d84>] device_add+0xd4/0x690
> [<ffffffff81356c2b>] ? dev_set_name+0x4b/0x70
> [<ffffffffa001a884>] cuse_process_init_reply+0x2b4/0x420 [cuse]
> ...
>
> The problem is that kobject_add_internal() first adds a kobject to the
> kset and then try to create sysfs directory for it. If the creation
> fails,
Why would the creation fail? How are you triggering this?
> it remove the kobject from the kset. get_device_parent()
> accesses class_dirs kset while only holding class_dirs.list_lock to
> see whether the cuse class dir exists. But when it exists, it may not
> have finished initialization yet or may fail and get removed soon. In
> the above case, the former happened so the second one ends up trying
> to create subdirectory under null sysfs_dirent.
What kobjects are you using here? Shouldn't you be using a 'struct
device' instead?
> Patch attached at the end of this email solves this problem in an ugly
> way.
>
> <rant>
> One of the things I really dislike about these k* stuff is that the
> API advertises much more than they're actually capable of. The
> encapsulated synchronization might look like a good idea but it fails
> horribly in practice because these things are always used in
> connection with other objects. The API just tricks developers to
> think that all the complexities have been handled and nicely
> encapsulated when the reality is those are just hidden under giant
> pile of abstract crap. I really hope someday driver model can do away
> with all these k* stuff and use just necessary amount of abstraction
> like other sane kernel subsystems. :-(
> </rant>
I agree, if this happens, it is a failure of the driver model code.
But I don't see what the patch is solving here, how is this triggered?
thanks,
greg k-h
Greg KH wrote:
>>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
>> Ping. This one needs to be fixed in -stable. It can be triggered by
>> other char devices too.
>
> Sorry, been slowly catching up...
>
> This can be triggered by char devices? Huh? How? I don't see the
> failure path that is happening here.
Oooh, s/char/virtual/. The bug is in the path which creates a
directory under the phony parent.
> And char devices shouldn't really be using the kobject at all, except
> for a very basic reference count.
>
> I keep threatening to rip kobject out of a char device and just use a
> kref, as that is all that is really needed. Well, that and the kmap
> stuff, but again, it's not a "real" kobject being used there...
>
> Perhaps now is the time to do this.
Yay!
--
tejun
Hello,
Greg KH wrote:
> On Fri, Sep 25, 2009 at 05:22:13PM +0900, Tejun Heo wrote:
>> Hello, Greg, Kay, Guthrie.
>>
>> This problem was reported in bko#14023. (aiee.. again, sorry about
>> the delay)
>>
>> http://bugzilla.kernel.org/show_bug.cgi?id=14023
>>
>> sysfs is creating several devices in cuse class concurrently and with
>> CONFIG_SYSFS_DEPRECATED turned off, it triggers the following oops.
>>
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
>> IP: [<ffffffff81158b0a>] sysfs_addrm_start+0x4a/0xf0
>> PGD 75bb067 PUD 75be067 PMD 0
>> Oops: 0000 [#1] PREEMPT SMP
>> last sysfs file: /sys/devices/system/cpu/cpu7/topology/core_siblings
>> CPU 1
>> Modules linked in: cuse fuse
>> Pid: 4737, comm: osspd Not tainted 2.6.31-work #77
>> RIP: 0010:[<ffffffff81158b0a>] [<ffffffff81158b0a>] sysfs_addrm_start+0x4a/0xf0
>> RSP: 0018:ffff88000042f8f8 EFLAGS: 00010296
>> RAX: ffff88000042ffd8 RBX: 0000000000000000 RCX: 0000000000000000
>> RDX: 0000000000000000 RSI: ffff880007eef660 RDI: 0000000000000001
>> RBP: ffff88000042f918 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000001 R11: ffffffff81158b0a R12: ffff88000042f928
>> R13: 00000000fffffff4 R14: 0000000000000000 R15: ffff88000042f9a0
>> FS: 00007fe93905a950(0000) GS:ffff880008600000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>> CR2: 0000000000000038 CR3: 00000000077c9000 CR4: 00000000000006e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
>> Process osspd (pid: 4737, threadinfo ffff88000042e000, task ffff880007eef040)
>> Stack:
>> ffff880005da10e8 0000000011cc8d6e ffff88000042f928 ffff880003d28a28
>> <0> ffff88000042f988 ffffffff811592d7 0000000000000000 0000000000000000
>> <0> 0000000000000000 0000000000000000 ffff88000042f958 0000000011cc8d6e
>> Call Trace:
>> [<ffffffff811592d7>] create_dir+0x67/0xe0
>> [<ffffffff811593a8>] sysfs_create_dir+0x58/0xb0
>> [<ffffffff8128ca7c>] ? kobject_add_internal+0xcc/0x220
>> [<ffffffff812942e1>] ? vsnprintf+0x3c1/0xb90
>> [<ffffffff8128cab7>] kobject_add_internal+0x107/0x220
>> [<ffffffff8128cd37>] kobject_add_varg+0x47/0x80
>> [<ffffffff8128ce53>] kobject_add+0x53/0x90
>> [<ffffffff81357d84>] device_add+0xd4/0x690
>> [<ffffffff81356c2b>] ? dev_set_name+0x4b/0x70
>> [<ffffffffa001a884>] cuse_process_init_reply+0x2b4/0x420 [cuse]
>> ...
>>
>> The problem is that kobject_add_internal() first adds a kobject to the
>> kset and then try to create sysfs directory for it. If the creation
>> fails,
>
> Why would the creation fail? How are you triggering this?
The above was description of the code in kobject_add_internal() not
the actual failure case. I'll explain in detail below.
>> it remove the kobject from the kset. get_device_parent()
>> accesses class_dirs kset while only holding class_dirs.list_lock to
>> see whether the cuse class dir exists. But when it exists, it may not
>> have finished initialization yet or may fail and get removed soon. In
>> the above case, the former happened so the second one ends up trying
>> to create subdirectory under null sysfs_dirent.
>
> What kobjects are you using here? Shouldn't you be using a 'struct
> device' instead?
>
> But I don't see what the patch is solving here, how is this triggered?
The culprit is get_device_parent() with !CONFIG_SYSFS_DEPRECATED.
When two virtual device creations race, get_device_parent() can be
entered by two threads and the following can happen.
* thread0:
device_add(): setup_parent() which calls get_device_parent().
get_device_parent(): Search the class kset list and and find
out that class_dir doesn't exist yet. Proceed to
kobject_create() and then to kobject_add().
kobject_add_internal(): The kobj has kset specified. Add the
kobj to the kset and then proceed to create_dir(). kobj->sd
isn't set yet.
* thread1:
device_add(): setup_parent() which calls get_device_parent().
get_device_parent(): Search the class kset list. Voila!
There's the kobject created by thread0. Return the kobj.
device_add(): call kobjct_add() to add the kobj for this
device to the parent kobj returned by get_device_parent().
kobject_add_internal(): call create_dir() to create the
directory for this kobj.
But, at this point, the parent kobj doesn't have ->sd
initialized yet because thread0 hasn't finished, so
sysfs_create_dir() tries to create a directory under NULL
parent and BOOM.
The problem is that creation of parent isn't atomic against lookup.
It's first added to the kset and then proceeds to create_dir().
Another possible race in get_device_parent() is one thread looking up
the parent and proceeding to parent creation when the other thread
already is trying to create parent but hasn't had the chance to add it
to the kset list. The function simply doesn't have proper
synchronization.
Thanks.
--
tejun
On Tue, Oct 06, 2009 at 10:15:12AM +0900, Tejun Heo wrote:
> Greg KH wrote:
> >>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
> >> Ping. This one needs to be fixed in -stable. It can be triggered by
> >> other char devices too.
> >
> > Sorry, been slowly catching up...
> >
> > This can be triggered by char devices? Huh? How? I don't see the
> > failure path that is happening here.
>
> Oooh, s/char/virtual/. The bug is in the path which creates a
> directory under the phony parent.
>
> > And char devices shouldn't really be using the kobject at all, except
> > for a very basic reference count.
> >
> > I keep threatening to rip kobject out of a char device and just use a
> > kref, as that is all that is really needed. Well, that and the kmap
> > stuff, but again, it's not a "real" kobject being used there...
> >
> > Perhaps now is the time to do this.
>
> Yay!
Ugh, I tried to do this today but it looks like the gendisk structure
got all tied up with the kobj_map logic. Which doesn't look all too
correct to me but I'm not sure.
Kay, you did the gendisk kobject conversion, right? Any reason you tied
it into the kobj_map stuff? Or was that the way the code always was?
thanks,
greg k-h
On Tue, Oct 6, 2009 at 05:05, Greg KH <[email protected]> wrote:
> On Tue, Oct 06, 2009 at 10:15:12AM +0900, Tejun Heo wrote:
>> Greg KH wrote:
>> >>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
>> >> Ping. This one needs to be fixed in -stable. It can be triggered by
>> >> other char devices too.
>> >
>> > Sorry, been slowly catching up...
>> >
>> > This can be triggered by char devices? Huh? How? I don't see the
>> > failure path that is happening here.
>>
>> Oooh, s/char/virtual/. The bug is in the path which creates a
>> directory under the phony parent.
>>
>> > And char devices shouldn't really be using the kobject at all, except
>> > for a very basic reference count.
>> >
>> > I keep threatening to rip kobject out of a char device and just use a
>> > kref, as that is all that is really needed. Well, that and the kmap
>> > stuff, but again, it's not a "real" kobject being used there...
>> >
>> > Perhaps now is the time to do this.
>>
>> Yay!
>
> Ugh, I tried to do this today but it looks like the gendisk structure
> got all tied up with the kobj_map logic. Which doesn't look all too
> correct to me but I'm not sure.
>
> Kay, you did the gendisk kobject conversion, right? Any reason you tied
> it into the kobj_map stuff? Or was that the way the code always was?
Yeah, it was like this, we just wrapped the struct device around the
kobject. We need the map here to be able to lookup a disk by
major/minor.
Kay