OOPS in configfs when doing d_delete

OOPS in configfs when doing d_delete

[Jiri Slaby]

Hi,

when configfs_attach_group fails in configfs_register_subsystem:
        dentry = d_alloc(configfs_sb->s_root, &name);
        if (dentry) {
                d_add(dentry, NULL);

                err = configfs_attach_group(sd->s_element, &group->cg_item,
                                            dentry);
                if (err) {
                        d_delete(dentry);
                        dput(dentry);


d_delete kills the kernel. I don't know what the actual bug is here, but
d_delete looks broken anyway:
        spin_lock(&dentry->d_lock);
        inode = dentry->d_inode;
        isdir = S_ISDIR(inode->i_mode);  <======== dereference
        if (dentry->d_count == 1) {
                if (inode && !spin_trylock(&inode->i_lock)) {
                    ^^^^^  <============= test

It seems like a superfluous test, not a potential null dereference to
me, right?

The oops:
BUG: unable to handle kernel NULL pointer dereference at 00000000000000ba
IP: [<ffffffff81166f64>] d_delete+0x34/0x100
PGD 26108067 PUD 26111067 PMD 0
Oops: 0000 [#1] PREEMPT SMP
last sysfs file: /sys/devices/virtual/tty/digi_ctl255/uevent
CPU 0
Modules linked in: ...
Pid: 6876, comm: modprobe Not tainted 2.6.37-22-desktop #1 /Bochs
RIP: 0010:[<ffffffff81166f64>]  [<ffffffff81166f64>] d_delete+0x34/0x100
RSP: 0018:ffff880026123ea8  EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff880028beb380 RCX: ffffffffa45c7bc0
RDX: 0000000000000001 RSI: ffff880028beb42d RDI: ffff880028beb388
RBP: ffff880028beb388 R08: 00000000d70f3acb R09: 000000009e367ab8
R10: 0000000000000000 R11: 000000009d377ab8 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000406750
FS:  00007f574dd28700(0000) GS:ffff88003e400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000000000000ba CR3: 0000000026002000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process modprobe (pid: 6876, threadinfo ffff880026122000, task
ffff880026dc4440)
Stack:
 ffffffffa45c0c80 ffff880028beb380 ffffffffa29204e0 ffffffffa291ca00
 0000000000000001 ffffffffffffffef 0000000c9f97db74 ffffffffa45c0c88
 ffffffffa45c0c80 ffffffffa45c0948 ffffffffa45d4000 ffffffffa45d4041
Call Trace:
 [<ffffffffa291ca00>] configfs_register_subsystem+0x110/0x1d0 [configfs]
 [<ffffffffa45d4041>] configfs_example_init+0x41/0x96
[configfs_example_macros]
 [<ffffffff810002da>] do_one_initcall+0x3a/0x170
 [<ffffffff810963ba>] sys_init_module+0xba/0x210
 [<ffffffff81002f8b>] system_call_fastpath+0x16/0x1b
 [<00007f574d672d2a>] 0x7f574d672d2a
Code: 89 fb 48 89 6c 24 08 48 8d 6b 08 48 c7 c7 80 22 a0 81 4c 89 64 24
10 e8 cb b6 3b 00 48 89 ef 45 31 e4 e8 c0 b6 3b 00 48 8b 43 10 <0f> b7
80 ba 00 00 00 25 00 f0 00 00 3d 00 40 00 00 8b 03 41 0f
RIP  [<ffffffff81166f64>] d_delete+0x34/0x100
 RSP <ffff880026123ea8>
CR2: 00000000000000ba

regards,
-- 
js
suse labs

Re: OOPS in configfs when doing d_delete

[Joel Becker]

On Mon, Feb 21, 2011 at 11:20:18AM +0100, Jiri Slaby wrote:
> when configfs_attach_group fails in configfs_register_subsystem:
>         dentry = d_alloc(configfs_sb->s_root, &name);
>         if (dentry) {
>                 d_add(dentry, NULL);
> 
>                 err = configfs_attach_group(sd->s_element, &group->cg_item,
>                                             dentry);
>                 if (err) {
>                         d_delete(dentry);
>                         dput(dentry);
> 
> 
> d_delete kills the kernel. I don't know what the actual bug is here, but
> d_delete looks broken anyway:
>         spin_lock(&dentry->d_lock);
>         inode = dentry->d_inode;
>         isdir = S_ISDIR(inode->i_mode);  <======== dereference
>         if (dentry->d_count == 1) {
>                 if (inode && !spin_trylock(&inode->i_lock)) {
>                     ^^^^^  <============= test
> 
> It seems like a superfluous test, not a potential null dereference to
> me, right?

	I think you're right about the superfluous test, but I need more
investigation to see what's going on.  Thanks for the report.
	What was causing attach_group() to fail?  Do you know?

Joel

-- 

Life's Little Instruction Book #444

	"Never underestimate the power of a kind word or deed."

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: OOPS in configfs when doing d_delete

[Jiri Slaby]

On 02/21/2011 11:44 AM, Joel Becker wrote:
> On Mon, Feb 21, 2011 at 11:20:18AM +0100, Jiri Slaby wrote:
>> when configfs_attach_group fails in configfs_register_subsystem:
>>         dentry = d_alloc(configfs_sb->s_root, &name);
>>         if (dentry) {
>>                 d_add(dentry, NULL);
>>
>>                 err = configfs_attach_group(sd->s_element, &group->cg_item,
>>                                             dentry);
>>                 if (err) {
>>                         d_delete(dentry);
>>                         dput(dentry);
>>
>>
>> d_delete kills the kernel. I don't know what the actual bug is here, but
>> d_delete looks broken anyway:
>>         spin_lock(&dentry->d_lock);
>>         inode = dentry->d_inode;
>>         isdir = S_ISDIR(inode->i_mode);  <======== dereference
>>         if (dentry->d_count == 1) {
>>                 if (inode && !spin_trylock(&inode->i_lock)) {
>>                     ^^^^^  <============= test
>>
>> It seems like a superfluous test, not a potential null dereference to
>> me, right?
> 
> 	I think you're right about the superfluous test, but I need more
> investigation to see what's going on.  Thanks for the report.
> 	What was causing attach_group() to fail?  Do you know?

Dunno, I just modprobe'd the configfs example from Doc dir
(configfs_example_macros).

regards,
-- 
js
suse labs

Re: OOPS in configfs when doing d_delete

[Joel Becker]

On Mon, Feb 21, 2011 at 11:47:09AM +0100, Jiri Slaby wrote:
> > 	I think you're right about the superfluous test, but I need more
> > investigation to see what's going on.  Thanks for the report.
> > 	What was causing attach_group() to fail?  Do you know?
> 
> Dunno, I just modprobe'd the configfs example from Doc dir
> (configfs_example_macros).

	I'm going to revisit the failed example (which shouldn't fail, I
would think).  Can you try the following patch to safely handle the
failure rather than crashing the kernel?

Joel

>From 68bbb327c48fdcdc48b71435d19b9e899745adf0 Mon Sep 17 00:00:00 2001
From: Joel Becker <jlbec@evilplan.org>
Date: Tue, 22 Feb 2011 01:09:49 -0800
Subject: [PATCH] configfs: Don't try to d_delete() negative dentries.

When configfs is faking mkdir() on its subsystem or default group
objects, it starts by adding a negative dentry.  It then tries to
instantiate the group.  If that should fail, it must clean up after
itself.

I was using d_delete() here, but configfs_attach_group() promises to
return an empty dentry on error.  d_delete() explodes with the entry
dentry.  Let's try d_drop() instead.  The unhashing is what we want for
our dentry.

Signed-off-by: Joel Becker <jlbec@evilplan.org>
---
 fs/configfs/dir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 90ff3cb..2af26b8 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -689,7 +689,8 @@ static int create_default_group(struct config_group *parent_group,
 			sd = child->d_fsdata;
 			sd->s_type |= CONFIGFS_USET_DEFAULT;
 		} else {
-			d_delete(child);
+			BUG_ON(child->d_inode);
+			d_drop(child);
 			dput(child);
 		}
 	}
@@ -1683,7 +1684,8 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 		err = configfs_attach_group(sd->s_element, &group->cg_item,
 					    dentry);
 		if (err) {
-			d_delete(dentry);
+			BUG_ON(dentry->d_inode);
+			d_drop(dentry);
 			dput(dentry);
 		} else {
 			spin_lock(&configfs_dirent_lock);
-- 
1.7.3.1


-- 

"But then she looks me in the eye
 And says, 'We're going to last forever,'
 And man you know I can't begin to doubt it.
 Cause it just feels so good and so free and so right,
 I know we ain't never going to change our minds about it, Hey!
 Here comes my girl."

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: OOPS in configfs when doing d_delete

[Jiri Slaby]

On 02/22/2011 10:14 AM, Joel Becker wrote:
> On Mon, Feb 21, 2011 at 11:47:09AM +0100, Jiri Slaby wrote:
>>> 	I think you're right about the superfluous test, but I need more
>>> investigation to see what's going on.  Thanks for the report.
>>> 	What was causing attach_group() to fail?  Do you know?
>>
>> Dunno, I just modprobe'd the configfs example from Doc dir
>> (configfs_example_macros).
> 
> 	I'm going to revisit the failed example (which shouldn't fail, I
> would think).

Got it. One needs to load both of the examples. The second loaded kills
the box.

>  Can you try the following patch to safely handle the
> failure rather than crashing the kernel?

Yes, it helps. But I need also (cut & paste):
--- a/Documentation/filesystems/configfs/configfs_example_explicit.c
+++ b/Documentation/filesystems/configfs/configfs_example_explicit.c
@@ -464,7 +464,7 @@ static int __init configfs_example_init(void)
        return 0;

 out_unregister:
-       for (; i >= 0; i--) {
+       for (i--; i >= 0; i--) {
                configfs_unregister_subsystem(example_subsys[i]);
        }

--- a/Documentation/filesystems/configfs/configfs_example_macros.c
+++ b/Documentation/filesystems/configfs/configfs_example_macros.c
@@ -427,7 +427,7 @@ static int __init configfs_example_init(void)
        return 0;

 out_unregister:
-       for (; i >= 0; i--) {
+       for (i--; i >= 0; i--) {
                configfs_unregister_subsystem(example_subsys[i]);
        }

> From 68bbb327c48fdcdc48b71435d19b9e899745adf0 Mon Sep 17 00:00:00 2001
> From: Joel Becker <jlbec@evilplan.org>
> Date: Tue, 22 Feb 2011 01:09:49 -0800
> Subject: [PATCH] configfs: Don't try to d_delete() negative dentries.
> 
> When configfs is faking mkdir() on its subsystem or default group
> objects, it starts by adding a negative dentry.  It then tries to
> instantiate the group.  If that should fail, it must clean up after
> itself.
> 
> I was using d_delete() here, but configfs_attach_group() promises to
> return an empty dentry on error.  d_delete() explodes with the entry
> dentry.  Let's try d_drop() instead.  The unhashing is what we want for
> our dentry.
> 
> Signed-off-by: Joel Becker <jlbec@evilplan.org>
> ---
>  fs/configfs/dir.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 90ff3cb..2af26b8 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -689,7 +689,8 @@ static int create_default_group(struct config_group *parent_group,
>  			sd = child->d_fsdata;
>  			sd->s_type |= CONFIGFS_USET_DEFAULT;
>  		} else {
> -			d_delete(child);
> +			BUG_ON(child->d_inode);
> +			d_drop(child);
>  			dput(child);
>  		}
>  	}
> @@ -1683,7 +1684,8 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
>  		err = configfs_attach_group(sd->s_element, &group->cg_item,
>  					    dentry);
>  		if (err) {
> -			d_delete(dentry);
> +			BUG_ON(dentry->d_inode);
> +			d_drop(dentry);
>  			dput(dentry);
>  		} else {
>  			spin_lock(&configfs_dirent_lock);

thanks,
-- 
js

Re: OOPS in configfs when doing d_delete

[Jiri Slaby]

On 02/22/2011 10:14 AM, Joel Becker wrote:
> On Mon, Feb 21, 2011 at 11:47:09AM +0100, Jiri Slaby wrote:
>>> 	I think you're right about the superfluous test, but I need more
>>> investigation to see what's going on.  Thanks for the report.
>>> 	What was causing attach_group() to fail?  Do you know?
>>
>> Dunno, I just modprobe'd the configfs example from Doc dir
>> (configfs_example_macros).
> 
> 	I'm going to revisit the failed example (which shouldn't fail, I
> would think).  Can you try the following patch to safely handle the
> failure rather than crashing the kernel?
> 
> Joel

Hi, what's the status of this? (It's verified to work some time ago.)

> From 68bbb327c48fdcdc48b71435d19b9e899745adf0 Mon Sep 17 00:00:00 2001
> From: Joel Becker <jlbec@evilplan.org>
> Date: Tue, 22 Feb 2011 01:09:49 -0800
> Subject: [PATCH] configfs: Don't try to d_delete() negative dentries.
> 
> When configfs is faking mkdir() on its subsystem or default group
> objects, it starts by adding a negative dentry.  It then tries to
> instantiate the group.  If that should fail, it must clean up after
> itself.
> 
> I was using d_delete() here, but configfs_attach_group() promises to
> return an empty dentry on error.  d_delete() explodes with the entry
> dentry.  Let's try d_drop() instead.  The unhashing is what we want for
> our dentry.
> 
> Signed-off-by: Joel Becker <jlbec@evilplan.org>
> ---
>  fs/configfs/dir.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 90ff3cb..2af26b8 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -689,7 +689,8 @@ static int create_default_group(struct config_group *parent_group,
>  			sd = child->d_fsdata;
>  			sd->s_type |= CONFIGFS_USET_DEFAULT;
>  		} else {
> -			d_delete(child);
> +			BUG_ON(child->d_inode);
> +			d_drop(child);
>  			dput(child);
>  		}
>  	}
> @@ -1683,7 +1684,8 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
>  		err = configfs_attach_group(sd->s_element, &group->cg_item,
>  					    dentry);
>  		if (err) {
> -			d_delete(dentry);
> +			BUG_ON(dentry->d_inode);
> +			d_drop(dentry);
>  			dput(dentry);
>  		} else {
>  			spin_lock(&configfs_dirent_lock);


-- 
js

Re: OOPS in configfs when doing d_delete

[Joel Becker]

On Thu, May 12, 2011 at 11:34:29AM +0200, Jiri Slaby wrote:
> On 02/22/2011 10:14 AM, Joel Becker wrote:
> > On Mon, Feb 21, 2011 at 11:47:09AM +0100, Jiri Slaby wrote:
> >>> 	I think you're right about the superfluous test, but I need more
> >>> investigation to see what's going on.  Thanks for the report.
> >>> 	What was causing attach_group() to fail?  Do you know?
> >>
> >> Dunno, I just modprobe'd the configfs example from Doc dir
> >> (configfs_example_macros).
> > 
> > 	I'm going to revisit the failed example (which shouldn't fail, I
> > would think).  Can you try the following patch to safely handle the
> > failure rather than crashing the kernel?
> > 
> > Joel
> 
> Hi, what's the status of this? (It's verified to work some time ago.)

	The runtime fix is queued up for mainline.  I'll be looking at
the example code fix for the next merge window.

Joel

-- 

"And yet I find,
 And yet I find repeating in my head.
 If I can't be my own, 
 I'd feel better dead."

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: OOPS in configfs when doing d_delete

[Jiri Slaby]

On 05/18/2011 09:58 PM, Joel Becker wrote:
> On Thu, May 12, 2011 at 11:34:29AM +0200, Jiri Slaby wrote:
>> On 02/22/2011 10:14 AM, Joel Becker wrote:
>>> On Mon, Feb 21, 2011 at 11:47:09AM +0100, Jiri Slaby wrote:
>>>>> 	I think you're right about the superfluous test, but I need more
>>>>> investigation to see what's going on.  Thanks for the report.
>>>>> 	What was causing attach_group() to fail?  Do you know?
>>>>
>>>> Dunno, I just modprobe'd the configfs example from Doc dir
>>>> (configfs_example_macros).
>>>
>>> 	I'm going to revisit the failed example (which shouldn't fail, I
>>> would think).  Can you try the following patch to safely handle the
>>> failure rather than crashing the kernel?
>>>
>>> Joel
>>
>> Hi, what's the status of this? (It's verified to work some time ago.)
> 
> 	The runtime fix is queued up for mainline.  I'll be looking at
> the example code fix for the next merge window.

The fixes for the examples were merged today after being some time in
the -mm tree.

thanks,
-- 
js

Re: OOPS in configfs when doing d_delete

[Joel Becker]

On Fri, May 27, 2011 at 11:12:22PM +0200, Jiri Slaby wrote:
> On 05/18/2011 09:58 PM, Joel Becker wrote:
> > On Thu, May 12, 2011 at 11:34:29AM +0200, Jiri Slaby wrote:
> >> On 02/22/2011 10:14 AM, Joel Becker wrote:
> >>> On Mon, Feb 21, 2011 at 11:47:09AM +0100, Jiri Slaby wrote:
> >>>>> 	I think you're right about the superfluous test, but I need more
> >>>>> investigation to see what's going on.  Thanks for the report.
> >>>>> 	What was causing attach_group() to fail?  Do you know?
> >>>>
> >>>> Dunno, I just modprobe'd the configfs example from Doc dir
> >>>> (configfs_example_macros).
> >>>
> >>> 	I'm going to revisit the failed example (which shouldn't fail, I
> >>> would think).  Can you try the following patch to safely handle the
> >>> failure rather than crashing the kernel?
> >>>
> >>> Joel
> >>
> >> Hi, what's the status of this? (It's verified to work some time ago.)
> > 
> > 	The runtime fix is queued up for mainline.  I'll be looking at
> > the example code fix for the next merge window.
> 
> The fixes for the examples were merged today after being some time in
> the -mm tree.

	I saw.  Thanks.

Joel

-- 

 One look at the From:
 understanding has blossomed
 .procmailrc grows
        - Alexander Viro

			http://www.jlbec.org/
			jlbec@evilplan.org