OOPS in configfs when doing d_delete

DISCUSSION Jiri Slaby 3 participants 9 messages Feb 21, 2011 – May 27, 2011

Subsystems (3)

Documentation VFS configfs

lore

Share & tools

lore

Jiri Slaby

author

2011-02-21 10:20 UTC #0 lore raw

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,

Signature · 2 lines

js
suse labs

Joel Becker

2011-02-21 10:44 UTC #1 lore raw

Quoted 21 lines · Jiri Slaby

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

Quoted 2 lines

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

Signature · 6 lines

Life's Little Instruction Book #444

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

			http://www.jlbec.org/
			[email protected]

Jiri Slaby

author

2011-02-21 10:55 UTC #2 lore raw

Quoted 22 lines · Joel Becker

Quoted 21 lines · Jiri Slaby

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

Quoted 6 lines

Quoted 2 lines
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,

Signature · 2 lines

js
suse labs

Joel Becker

2011-02-22 09:14 UTC #3 lore raw

Quoted 6 lines · Jiri Slaby

Quoted 3 lines
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

Quoted 1 line

From 68bbb327c48fdcdc48b71435d19b9e899745adf0 Mon Sep 17 00:00:00 2001

From: Joel Becker <[email protected]>
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 <[email protected]>

---
 fs/configfs/dir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

fs/configfs/dir.c +4 -2

index 90ff3cb..2af26b8 100644
@@ -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);

Signature · 14 lines

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/
			[email protected]

Jiri Slaby

2011-03-01 22:42 UTC #4 lore raw

Quoted 10 lines · Joel Becker

Quoted 6 lines · Jiri Slaby
Quoted 3 lines
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.

Quoted 2 lines

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):

Documentation/filesystems/configfs/configfs_example_explicit.c +1 -1

@@ -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]);
        }

Documentation/filesystems/configfs/configfs_example_macros.c +1 -1

@@ -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]);
        }

Quoted 35 lines

From 68bbb327c48fdcdc48b71435d19b9e899745adf0 Mon Sep 17 00:00:00 2001
From: Joel Becker <[email protected]>
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 <[email protected]>
---
 fs/configfs/dir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
fs/configfs/dir.c +2 -1
index 90ff3cb..2af26b8 100644
@@ -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);
 		}
 	}

fs/configfs/dir.c int configfs_register_subsystem(struct configfs_subsystem *subsys) · line 1691

Quoted 9 lines

fs/configfs/dir.c +2 -1

@@ -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,

Signature · 1 line

js

Jiri Slaby

2011-05-12 09:34 UTC #5 lore raw

In reply to Joel Becker

Quoted 5 lines · Joel Becker

Quoted 4 lines · Jiri Slaby
Quoted 3 lines
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?

Quoted 8 lines

Quoted 2 lines
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.)

Quoted 44 lines

From 68bbb327c48fdcdc48b71435d19b9e899745adf0 Mon Sep 17 00:00:00 2001
From: Joel Becker <[email protected]>
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 <[email protected]>
---
 fs/configfs/dir.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
fs/configfs/dir.c +4 -2
index 90ff3cb..2af26b8 100644
@@ -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);

Signature · 1 line

js

Joel Becker

2011-05-18 19:58 UTC #6 lore raw

Quoted 9 lines · Jiri Slaby

Quoted 8 lines · Joel Becker
Quoted 6 lines · Jiri Slaby
Quoted 3 lines
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).

Quoted 7 lines

Quoted 5 lines
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

Signature · 7 lines

"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/
			[email protected]

Jiri Slaby

2011-05-27 21:12 UTC #7 lore raw

Quoted 14 lines · Joel Becker

Quoted 13 lines · Jiri Slaby
Quoted 12 lines · Joel Becker
Quoted 6 lines · Jiri Slaby
Quoted 3 lines
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?

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
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,

Signature · 1 line

js

Joel Becker

2011-05-27 21:13 UTC #8 lore raw

Quoted 17 lines · Jiri Slaby

Quoted 16 lines · Joel Becker
Quoted 15 lines · Jiri Slaby
Quoted 13 lines · Joel Becker
Quoted 6 lines · Jiri Slaby
Quoted 3 lines
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

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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

Signature · 7 lines

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

			http://www.jlbec.org/
			[email protected]