diff options
author | Alexey Dobriyan <adobriyan@sw.ru> | 2007-12-05 02:45:28 -0500 |
---|---|---|
committer | Linus Torvalds <torvalds@woody.linux-foundation.org> | 2007-12-05 12:21:20 -0500 |
commit | 5a622f2d0f86b316b07b55a4866ecb5518dd1cf7 (patch) | |
tree | f89bb875b5b440dced320336db6cac24b9e4d8b7 /fs | |
parent | d4beaf4ab5f89496f2bcf67db62ad95d99bfeff6 (diff) |
proc: fix proc_dir_entry refcounting
Creating PDEs with refcount 0 and "deleted" flag has problems (see below).
Switch to usual scheme:
* PDE is created with refcount 1
* every de_get does +1
* every de_put() and remove_proc_entry() do -1
* once refcount reaches 0, PDE is freed.
This elegantly fixes at least two following races (both observed) without
introducing new locks, without abusing old locks, without spreading
lock_kernel():
1) PDE leak
remove_proc_entry de_put
----------------- ------
[refcnt = 1]
if (atomic_read(&de->count) == 0)
if (atomic_dec_and_test(&de->count))
if (de->deleted)
/* also not taken! */
free_proc_entry(de);
else
de->deleted = 1;
[refcount=0, deleted=1]
2) use after free
remove_proc_entry de_put
----------------- ------
[refcnt = 1]
if (atomic_dec_and_test(&de->count))
if (atomic_read(&de->count) == 0)
free_proc_entry(de);
/* boom! */
if (de->deleted)
free_proc_entry(de);
BUG: unable to handle kernel paging request at virtual address 6b6b6b6b
printing eip: c10acdda *pdpt = 00000000338f8001 *pde = 0000000000000000
Oops: 0000 [#1] PREEMPT SMP
Modules linked in: af_packet ipv6 cpufreq_ondemand loop serio_raw psmouse k8temp hwmon sr_mod cdrom
Pid: 23161, comm: cat Not tainted (2.6.24-rc2-8c0863403f109a43d7000b4646da4818220d501f #4)
EIP: 0060:[<c10acdda>] EFLAGS: 00210097 CPU: 1
EIP is at strnlen+0x6/0x18
EAX: 6b6b6b6b EBX: 6b6b6b6b ECX: 6b6b6b6b EDX: fffffffe
ESI: c128fa3b EDI: f380bf34 EBP: ffffffff ESP: f380be44
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process cat (pid: 23161, ti=f380b000 task=f38f2570 task.ti=f380b000)
Stack: c10ac4f0 00000278 c12ce000 f43cd2a8 00000163 00000000 7da86067 00000400
c128fa20 00896b18 f38325a8 c128fe20 ffffffff 00000000 c11f291e 00000400
f75be300 c128fa20 f769c9a0 c10ac779 f380bf34 f7bfee70 c1018e6b f380bf34
Call Trace:
[<c10ac4f0>] vsnprintf+0x2ad/0x49b
[<c10ac779>] vscnprintf+0x14/0x1f
[<c1018e6b>] vprintk+0xc5/0x2f9
[<c10379f1>] handle_fasteoi_irq+0x0/0xab
[<c1004f44>] do_IRQ+0x9f/0xb7
[<c117db3b>] preempt_schedule_irq+0x3f/0x5b
[<c100264e>] need_resched+0x1f/0x21
[<c10190ba>] printk+0x1b/0x1f
[<c107c8ad>] de_put+0x3d/0x50
[<c107c8f8>] proc_delete_inode+0x38/0x41
[<c107c8c0>] proc_delete_inode+0x0/0x41
[<c1066298>] generic_delete_inode+0x5e/0xc6
[<c1065aa9>] iput+0x60/0x62
[<c1063c8e>] d_kill+0x2d/0x46
[<c1063fa9>] dput+0xdc/0xe4
[<c10571a1>] __fput+0xb0/0xcd
[<c1054e49>] filp_close+0x48/0x4f
[<c1055ee9>] sys_close+0x67/0xa5
[<c10026b6>] sysenter_past_esp+0x5f/0x85
=======================
Code: c9 74 0c f2 ae 74 05 bf 01 00 00 00 4f 89 fa 5f 89 d0 c3 85 c9 57 89 c7 89 d0 74 05 f2 ae 75 01 4f 89 f8 5f c3 89 c1 89 c8 eb 06 <80> 38 00 74 07 40 4a 83 fa ff 75 f4 29 c8 c3 90 90 90 57 83 c9
EIP: [<c10acdda>] strnlen+0x6/0x18 SS:ESP 0068:f380be44
Also, remove broken usage of ->deleted from reiserfs: if sget() succeeds,
module is already pinned and remove_proc_entry() can't happen => nobody
can mark PDE deleted.
Dummy proc root in netns code is not marked with refcount 1. AFAICS, we
never get it, it's just for proper /proc/net removal. I double checked
CLONE_NETNS continues to work.
Patch survives many hours of modprobe/rmmod/cat loops without new bugs
which can be attributed to refcounting.
Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Diffstat (limited to 'fs')
-rw-r--r-- | fs/proc/generic.c | 9 | ||||
-rw-r--r-- | fs/proc/inode.c | 9 | ||||
-rw-r--r-- | fs/proc/root.c | 1 | ||||
-rw-r--r-- | fs/reiserfs/procfs.c | 6 |
4 files changed, 5 insertions, 20 deletions
diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 5fccfe222a63..8d49838e5554 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c | |||
@@ -595,6 +595,7 @@ static struct proc_dir_entry *proc_create(struct proc_dir_entry **parent, | |||
595 | ent->namelen = len; | 595 | ent->namelen = len; |
596 | ent->mode = mode; | 596 | ent->mode = mode; |
597 | ent->nlink = nlink; | 597 | ent->nlink = nlink; |
598 | atomic_set(&ent->count, 1); | ||
598 | ent->pde_users = 0; | 599 | ent->pde_users = 0; |
599 | spin_lock_init(&ent->pde_unload_lock); | 600 | spin_lock_init(&ent->pde_unload_lock); |
600 | ent->pde_unload_completion = NULL; | 601 | ent->pde_unload_completion = NULL; |
@@ -692,7 +693,6 @@ void free_proc_entry(struct proc_dir_entry *de) | |||
692 | 693 | ||
693 | /* | 694 | /* |
694 | * Remove a /proc entry and free it if it's not currently in use. | 695 | * Remove a /proc entry and free it if it's not currently in use. |
695 | * If it is in use, we set the 'deleted' flag. | ||
696 | */ | 696 | */ |
697 | void remove_proc_entry(const char *name, struct proc_dir_entry *parent) | 697 | void remove_proc_entry(const char *name, struct proc_dir_entry *parent) |
698 | { | 698 | { |
@@ -741,13 +741,8 @@ continue_removing: | |||
741 | parent->nlink--; | 741 | parent->nlink--; |
742 | de->nlink = 0; | 742 | de->nlink = 0; |
743 | WARN_ON(de->subdir); | 743 | WARN_ON(de->subdir); |
744 | if (!atomic_read(&de->count)) | 744 | if (atomic_dec_and_test(&de->count)) |
745 | free_proc_entry(de); | 745 | free_proc_entry(de); |
746 | else { | ||
747 | de->deleted = 1; | ||
748 | printk("remove_proc_entry: %s/%s busy, count=%d\n", | ||
749 | parent->name, de->name, atomic_read(&de->count)); | ||
750 | } | ||
751 | break; | 746 | break; |
752 | } | 747 | } |
753 | spin_unlock(&proc_subdir_lock); | 748 | spin_unlock(&proc_subdir_lock); |
diff --git a/fs/proc/inode.c b/fs/proc/inode.c index abe6a3f04368..1a551d92e1d8 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c | |||
@@ -43,13 +43,8 @@ void de_put(struct proc_dir_entry *de) | |||
43 | return; | 43 | return; |
44 | } | 44 | } |
45 | 45 | ||
46 | if (atomic_dec_and_test(&de->count)) { | 46 | if (atomic_dec_and_test(&de->count)) |
47 | if (de->deleted) { | 47 | free_proc_entry(de); |
48 | printk("de_put: deferred delete of %s\n", | ||
49 | de->name); | ||
50 | free_proc_entry(de); | ||
51 | } | ||
52 | } | ||
53 | unlock_kernel(); | 48 | unlock_kernel(); |
54 | } | 49 | } |
55 | } | 50 | } |
diff --git a/fs/proc/root.c b/fs/proc/root.c index ec9cb3b6c93b..81f99e691f99 100644 --- a/fs/proc/root.c +++ b/fs/proc/root.c | |||
@@ -207,6 +207,7 @@ struct proc_dir_entry proc_root = { | |||
207 | .name = "/proc", | 207 | .name = "/proc", |
208 | .mode = S_IFDIR | S_IRUGO | S_IXUGO, | 208 | .mode = S_IFDIR | S_IRUGO | S_IXUGO, |
209 | .nlink = 2, | 209 | .nlink = 2, |
210 | .count = ATOMIC_INIT(1), | ||
210 | .proc_iops = &proc_root_inode_operations, | 211 | .proc_iops = &proc_root_inode_operations, |
211 | .proc_fops = &proc_root_operations, | 212 | .proc_fops = &proc_root_operations, |
212 | .parent = &proc_root, | 213 | .parent = &proc_root, |
diff --git a/fs/reiserfs/procfs.c b/fs/reiserfs/procfs.c index 9aa7a06e093f..001144621672 100644 --- a/fs/reiserfs/procfs.c +++ b/fs/reiserfs/procfs.c | |||
@@ -420,12 +420,6 @@ static void *r_start(struct seq_file *m, loff_t * pos) | |||
420 | return NULL; | 420 | return NULL; |
421 | 421 | ||
422 | up_write(&s->s_umount); | 422 | up_write(&s->s_umount); |
423 | |||
424 | if (de->deleted) { | ||
425 | deactivate_super(s); | ||
426 | return NULL; | ||
427 | } | ||
428 | |||
429 | return s; | 423 | return s; |
430 | } | 424 | } |
431 | 425 | ||