From: Ingo Molnar <mingo@elte.hu>
To: Andrew Morton <akpm@osdl.org>
Cc: Steven Rostedt <rostedt@goodmis.org>, linux-kernel@vger.kernel.org
Subject: [patch, validator] fix proc_subdir_lock related deadlock
Date: Wed, 25 Jan 2006 18:03:31 +0100 [thread overview]
Message-ID: <20060125170331.GA29339@elte.hu> (raw)
proc_subdir_lock can also be used from softirq (tasklet) context, which
may lead to deadlocks.
This bug was found via the lock validator:
============================
[ BUG: illegal lock usage! ]
----------------------------
illegal {enabled-softirqs} -> {used-in-softirq} usage.
ifup/2283 [HC0[0]:SC1[2]:HE1:SE0] takes {proc_subdir_lock [u:25]}, at:
[<c0196363>] remove_proc_entry+0x33/0x1f0
{enabled-softirqs} state was registered at:
[<c04d7c0d>] _spin_unlock_bh+0xd/0x10
hardirqs last enabled at: [<c04d7bb5>] _spin_unlock_irqrestore+0x25/0x30
softirqs last enabled at: [<c0127624>] free_uid+0x24/0x80
other info that might help in debugging this:
------------------------------
| showing all locks held by: | (ifup/2283 [c31a6790, 125]):
------------------------------
[<c010432d>] show_trace+0xd/0x10
[<c0104347>] dump_stack+0x17/0x20
[<c0137181>] print_usage_bug+0x1e1/0x200
[<c0137739>] mark_lock+0x259/0x290
[<c0137bd5>] debug_lock_chain_spin+0x465/0x10f0
[<c0264a6d>] _raw_spin_lock+0x2d/0x90
[<c04d7a18>] _spin_lock+0x8/0x10
[<c0196363>] remove_proc_entry+0x33/0x1f0
[<c0141d79>] unregister_handler_proc+0x19/0x20
[<c014153b>] free_irq+0x7b/0xe0
[<c02f15e2>] floppy_release_irq_and_dma+0x1b2/0x210
[<c02efad7>] set_dor+0xc7/0x1b0
[<c02f2b51>] motor_off_callback+0x21/0x30
[<c01272b5>] run_timer_softirq+0xf5/0x1f0
[<c0122c17>] __do_softirq+0x87/0x120
[<c0105519>] do_softirq+0x69/0xb0
=======================
the way i fixed this bug was to make all uses of the proc_subdir_lock
softirq-safe. Alternatively, we may want to forbid the use of this lock
(and remove_proc_entry()) from softirq contexts - but a quick glance
showed that quite some drivers are affected, and it would need a full
review to make sure the block is never taken from a softirq context. So
this seemed the safest fix.
the patched 2.6.16-rc1-mm1 kernel now passes validation.
Signed-off-by: Ingo Molnar <mingo@elte.hu>
----
fs/proc/generic.c | 34 +++++++++++++++++++---------------
fs/proc/proc_devtree.c | 4 ++--
2 files changed, 21 insertions(+), 17 deletions(-)
Index: linux/fs/proc/generic.c
===================================================================
--- linux.orig/fs/proc/generic.c
+++ linux/fs/proc/generic.c
@@ -30,6 +30,10 @@ static ssize_t proc_file_write(struct fi
size_t count, loff_t *ppos);
static loff_t proc_file_lseek(struct file *, loff_t, int);
+/*
+ * Is mostly used from process, but can occasionally be used from
+ * softirq context too - hence all locking must be softirq-safe:
+ */
DEFINE_SPINLOCK(proc_subdir_lock);
int proc_match(int len, const char *name, struct proc_dir_entry *de)
@@ -282,7 +286,7 @@ static int xlate_proc_name(const char *n
int len;
int rtn = 0;
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
de = &proc_root;
while (1) {
next = strchr(cp, '/');
@@ -303,7 +307,7 @@ static int xlate_proc_name(const char *n
*residual = cp;
*ret = de;
out:
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
return rtn;
}
@@ -389,7 +393,7 @@ struct dentry *proc_lookup(struct inode
int error = -ENOENT;
lock_kernel();
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
de = PDE(dir);
if (de) {
for (de = de->subdir; de ; de = de->next) {
@@ -398,15 +402,15 @@ struct dentry *proc_lookup(struct inode
if (!memcmp(dentry->d_name.name, de->name, de->namelen)) {
unsigned int ino = de->low_ino;
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
error = -EINVAL;
inode = proc_get_inode(dir->i_sb, ino, de);
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
break;
}
}
}
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
unlock_kernel();
if (inode) {
@@ -460,13 +464,13 @@ int proc_readdir(struct file * filp,
filp->f_pos++;
/* fall through */
default:
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
de = de->subdir;
i -= 2;
for (;;) {
if (!de) {
ret = 1;
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
goto out;
}
if (!i)
@@ -477,15 +481,15 @@ int proc_readdir(struct file * filp,
do {
/* filldir passes info to user space */
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
if (filldir(dirent, de->name, de->namelen, filp->f_pos,
de->low_ino, de->mode >> 12) < 0)
goto out;
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
filp->f_pos++;
de = de->next;
} while (de);
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
}
ret = 1;
out: unlock_kernel();
@@ -520,11 +524,11 @@ static int proc_register(struct proc_dir
return -EAGAIN;
dp->low_ino = i;
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
dp->next = dir->subdir;
dp->parent = dir;
dir->subdir = dp;
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
if (S_ISDIR(dp->mode)) {
if (dp->proc_iops == NULL) {
@@ -718,7 +722,7 @@ void remove_proc_entry(const char *name,
goto out;
len = strlen(fn);
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
for (p = &parent->subdir; *p; p=&(*p)->next ) {
if (!proc_match(len, fn, *p))
continue;
@@ -739,7 +743,7 @@ void remove_proc_entry(const char *name,
}
break;
}
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
out:
return;
}
Index: linux/fs/proc/proc_devtree.c
===================================================================
--- linux.orig/fs/proc/proc_devtree.c
+++ linux/fs/proc/proc_devtree.c
@@ -136,11 +136,11 @@ void proc_device_tree_add_node(struct de
* properties are quite unimportant for us though, thus we
* simply "skip" them here, but we do have to check.
*/
- spin_lock(&proc_subdir_lock);
+ spin_lock_bh(&proc_subdir_lock);
for (ent = de->subdir; ent != NULL; ent = ent->next)
if (!strcmp(ent->name, pp->name))
break;
- spin_unlock(&proc_subdir_lock);
+ spin_unlock_bh(&proc_subdir_lock);
if (ent != NULL) {
printk(KERN_WARNING "device-tree: property \"%s\" name"
" conflicts with node in %s\n", pp->name,
next reply other threads:[~2006-01-25 17:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-01-25 17:03 Ingo Molnar [this message]
2006-01-25 17:14 ` [patch, validator] fix proc_subdir_lock related deadlock Steven Rostedt
2006-01-25 18:08 ` Ingo Molnar
2006-01-25 18:14 ` [patch, validator] fix files_lock " Ingo Molnar
2006-01-25 18:23 ` [patch, validator] fix proc_subdir_lock " Andrew Morton
2006-01-25 20:21 ` Ingo Molnar
2006-01-26 0:02 ` [patch, lock validator] fix proc_inum_lock " Ingo Molnar
2006-01-26 0:11 ` Ingo Molnar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20060125170331.GA29339@elte.hu \
--to=mingo@elte.hu \
--cc=akpm@osdl.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.