tasklist_lock/dcache_lock race bugfix

RFC PATCH In review Manfred Spraul 2 participants 3 messages Apr 6, 2003 – Apr 13, 2003

Subsystems (4)

VFS Scheduler exec and binfmt Proc filesystem

lore

Share & tools

lore

Manfred Spraul

author

2003-04-06 19:47 UTC #0 lore raw

RFC PATCH 1/1 tasklist_lock/dcache_lock race bugfix

__unhash_process acquires the dcache_lock while holding the tasklist_lock for writing. This can deadlock. While trying to fix that race, I found further races with proc_dentry. The attached patch (hopefully) fixes all these races.

Changes:
- fs/proc/base.c assumed that p->pid is reset to 0 during exit. This is not the case anymore. I now look at the count of the pid structure for PIDTYPE_PID.
- pid_delete_dentry and pid_revalidate removed. The implementation didn't work, and noone complained.
- only d_add the new proc entry if the task is still valid: d_add+d_drop is a race, if a lookup happens between both calls.
- add a new spinlock that protects proc_dentry.

What do you think? I don't like the new spinlock, but I couldn't find another solution.

--
Manfred

// $Header$
// Kernel Version:

//  VERSION = 2
//  PATCHLEVEL = 5
//  SUBLEVEL = 66
//  EXTRAVERSION =

include/linux/sched.h +2 -0

@@ -428,6 +428,8 @@
    	u32 self_exec_id;
 /* Protection of (de-)allocation: mm, files, fs, tty */
 	spinlock_t alloc_lock;
+/* Protection of proc_dentry: nesting proc_lock, dcache_lock, write_lock_irq(&tasklist_lock); */
+	spinlock_t proc_lock;
 /* context-switch lock */
 	spinlock_t switch_lock;

fs/proc/base.c +70 ~~-29~~

141 lines not on this page · raw message

kernel/exit.c +13 ~~-28~~

89 lines not on this page · raw message

include/linux/init_task.h +1 -0

@@ -101,6 +101,7 @@
 	.blocked	= {{0}},					\
 	 .posix_timers	 = LIST_HEAD_INIT(tsk.posix_timers),		   \
 	.alloc_lock	= SPIN_LOCK_UNLOCKED,				\
+	.proc_lock	= SPIN_LOCK_UNLOCKED,				\
 	.switch_lock	= SPIN_LOCK_UNLOCKED,				\
 	.journal_info	= NULL,						\
 }

fs/exec.c +8 ~~-29~~

@@ -529,30 +529,6 @@
 	return 0;
 }
 
-static struct dentry *clean_proc_dentry(struct task_struct *p)
-{
-	struct dentry *proc_dentry = p->proc_dentry;
-
-	if (proc_dentry) {
-		spin_lock(&dcache_lock);
-		if (!d_unhashed(proc_dentry)) {
-			dget_locked(proc_dentry);
-			__d_drop(proc_dentry);
-		} else
-			proc_dentry = NULL;
-		spin_unlock(&dcache_lock);
-	}
-	return proc_dentry;
-}
-
-static inline void put_proc_dentry(struct dentry *dentry)
-{
-	if (dentry) {
-		shrink_dcache_parent(dentry);
-		dput(dentry);
-	}
-}
-
 /*
  * This function makes sure the current process has its own signal table,
  * so that flush_signal_handlers can later reset the handlers without

@@ -660,9 +636,11 @@
 		while (leader->state != TASK_ZOMBIE)
 			yield();
 
+		task_lock(leader);
+		task_lock(current);
+		proc_dentry1 = proc_pid_unhash(current);
+		proc_dentry2 = proc_pid_unhash(leader);
 		write_lock_irq(&tasklist_lock);
-		proc_dentry1 = clean_proc_dentry(current);
-		proc_dentry2 = clean_proc_dentry(leader);
 
 		if (leader->tgid != current->tgid)
 			BUG();

@@ -702,9 +680,10 @@
 		state = leader->state;
 
 		write_unlock_irq(&tasklist_lock);
-
-		put_proc_dentry(proc_dentry1);
-		put_proc_dentry(proc_dentry2);
+		task_unlock(current);
+		task_unlock(leader);
+		proc_pid_flush(proc_dentry1);
+		proc_pid_flush(proc_dentry2);
 
 		if (state != TASK_ZOMBIE)
 			BUG();

Andrew Morton

2003-04-07 09:26 UTC #1 lore raw

Quoted 6 lines · Manfred Spraul

__unhash_process acquires the dcache_lock while holding the tasklist_lock for writing. This can deadlock. While trying to fix that race, I found further races with proc_dentry.
The attached patch (hopefully) fixes all these races.

Quoted 9 lines

Changes:
- fs/proc/base.c assumed that p->pid is reset to 0 during exit. This is not the case anymore. I now look at the count of the pid structure for PIDTYPE_PID.
- pid_delete_dentry and pid_revalidate removed. The implementation didn't work, and noone complained.
- only d_add the new proc entry if the task is still valid: d_add+d_drop is a race, if a lookup happens between both calls.
- add a new spinlock that protects proc_dentry.

Did you send the correct patch?

kernel/exit.c: In function `release_task':
kernel/exit.c:59: warning: implicit declaration of function proc_pid_unhash' kernel/exit.c:59: warning: assignment makes pointer from integer without a cast kernel/exit.c:86: warning: implicit declaration of function proc_pid_flush' kernel/exit.c: In function `unhash_process':
kernel/exit.c:95: warning: proc_dentry' might be used uninitialized in this function fs/exec.c: In function de_thread':
fs/exec.c:641: warning: implicit declaration of function `proc_pid_unhash' fs/exec.c:641: warning: assignment makes pointer from integer without a cast fs/exec.c:642: warning: assignment makes pointer from integer without a cast fs/exec.c:685: warning: implicit declaration of function `proc_pid_flush'

and an oops at boot:

Program received signal SIGEMT, Emulation trap.
select_parent (parent=0x1) at fs/dcache.c:540
540 next = tmp->next;
(gdb) p tmp
No symbol "tmp" in current context.
(gdb) p next
$1 = (struct list_head *) 0x87f000fe
(gdb) bt

#0  select_parent (parent=0x1) at fs/dcache.c:540
#1  0xc01624e0 in shrink_dcache_parent (parent=0x1) at fs/dcache.c:590
#2  0xc017a8bd in proc_pid_flush (proc_dentry=0x1) at fs/proc/base.c:1129
#3  0xc0120a53 in unhash_process (p=0xc8db6660) at kernel/exit.c:102
#4  0xc03d188f in do_boot_cpu (apicid=0) at arch/i386/kernel/smpboot.c:805
#5  0xc03d1b53 in smp_boot_cpus (max_cpus=4) at arch/i386/kernel/smpboot.c:1040
#6  0xc03d1d58 in smp_prepare_cpus (max_cpus=4) at arch/i386/kernel/smpboot.c:1125
#7  0xc0105098 in init (unused=0x0) at init/main.c:563

Manfred Spraul

author

2003-04-13 20:10 UTC #2 lore raw

Quoted 17 lines · Andrew Morton

Quoted 13 lines · Manfred Spraul
__unhash_process acquires the dcache_lock while holding the tasklist_lock for writing. This can deadlock. While trying to fix that race, I found further races with proc_dentry. The attached patch (hopefully) fixes all these races.
Changes:
- fs/proc/base.c assumed that p->pid is reset to 0 during exit. This is not the case anymore. I now look at the count of the pid structure for PIDTYPE_PID.
- pid_delete_dentry and pid_revalidate removed. The implementation didn't work, and noone complained.
- only d_add the new proc entry if the task is still valid: d_add+d_drop is a race, if a lookup happens between both calls.
- add a new spinlock that protects proc_dentry.

Quoted 3 lines

Did you send the correct patch?

Yes, but my diff script was incomplete :-(

Quoted 3 lines

kernel/exit.c: In function `release_task':
kernel/exit.c:59: warning: implicit declaration of function `proc_pid_unhash'

Missing updates to <linux/proc_fs.h>

Quoted 5 lines

kernel/exit.c:59: warning: assignment makes pointer from integer without a cast kernel/exit.c:86: warning: implicit declaration of function proc_pid_flush' kernel/exit.c: In function unhash_process':
kernel/exit.c:95: warning: `proc_dentry' might be used uninitialized in this function

And this one is a fatal error: proc_dentry remained uninitialized, and that crashes during smp boot.

New patch attached.
Changelog unchanged.