Deadlock between tasklist_lock and dcache_lock

Deadlock between tasklist_lock and dcache_lock

[Manfred Spraul]

__unhash_process acquires the dcache_lock while holding the tasklist_lock 
for writing. AFAICS this can deadlock:

CPU1:
	[ in release_task() ]
	write_lock_irq(&tasklist_lock);
	__unhash_process()
	
CPU2:
	spin_lock(&dcache_lock);
	<interrupt>
	read_lock(&tasklist_lock);, // e.g. for signal delivery
	** spins, lock held by cpu 1 for writing.

CPU1:
	[ within __unhash_process() ]
	spin_lock(&dcache_lock);
	** spins, lock held by CPU2

Probably the callers of __unhash_process must acquire the dcache_lock 
before write_lock_irq(&tasklist_lock).

Any other ideas how to fix the deadlock?
--
	Manfred

Re: Deadlock between tasklist_lock and dcache_lock

[Maneesh Soni]

On Sun, Mar 23, 2003 at 05:30:39PM +0000, Manfred Spraul wrote:
> __unhash_process acquires the dcache_lock while holding the tasklist_lock 
> for writing. AFAICS this can deadlock:
> 
> CPU1:
> 	[ in release_task() ]
> 	write_lock_irq(&tasklist_lock);
> 	__unhash_process()
> 	
> CPU2:
> 	spin_lock(&dcache_lock);
> 	<interrupt>
> 	read_lock(&tasklist_lock);, // e.g. for signal delivery
> 	** spins, lock held by cpu 1 for writing.
> 
> CPU1:
> 	[ within __unhash_process() ]
> 	spin_lock(&dcache_lock);
> 	** spins, lock held by CPU2
> 
> Probably the callers of __unhash_process must acquire the dcache_lock 
> before write_lock_irq(&tasklist_lock).
> 
> Any other ideas how to fix the deadlock?
> --
> 	Manfred

Hello Manfred, Andrew

How is the patch below?.. just some re-arrangement of code. I don't think
we need task_list lock while cleaning up the proc_dentry.

Same problem is there in fs/exec.c:de_thread(). So this is a common solution.

The patch does following things:
0. brings cleaning up of proc_dentry outside tasklist_lock protection.
1. gets rid of code duplication in __unhash_process() and clean_proc_dentry()
2. combines clean_proc_dentry() and put_proc_dentry()
3. makes clean_proc_dentry() extern

fs/exec.c               |   31 +++++++++++++------------------
include/linux/proc_fs.h |    1 +
kernel/exit.c           |   28 ++++++++--------------------
3 files changed, 22 insertions(+), 38 deletions(-)

diff -urN linux-2.5.65-base/fs/exec.c linux-2.5.65-unhash_process/fs/exec.c
--- linux-2.5.65-base/fs/exec.c	2003-03-18 03:14:03.000000000 +0530
+++ linux-2.5.65-unhash_process/fs/exec.c	2003-03-24 16:49:30.000000000 +0530
@@ -529,27 +529,20 @@
 	return 0;
 }
 
-static struct dentry *clean_proc_dentry(struct task_struct *p)
+void clean_proc_dentry(struct dentry * proc_dentry)
 {
-	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);
+			spin_unlock(&dcache_lock);
+			shrink_dcache_parent(proc_dentry);
+			dput(proc_dentry);
+		}
+		else
+			spin_unlock(&dcache_lock);
 	}
 }
 
@@ -661,8 +654,8 @@
 			yield();
 
 		write_lock_irq(&tasklist_lock);
-		proc_dentry1 = clean_proc_dentry(current);
-		proc_dentry2 = clean_proc_dentry(leader);
+		proc_dentry1 = current->proc_dentry;
+		proc_dentry2 = leader->proc_dentry;
 
 		if (leader->tgid != current->tgid)
 			BUG();
@@ -703,8 +696,10 @@
 
 		write_unlock_irq(&tasklist_lock);
 
-		put_proc_dentry(proc_dentry1);
-		put_proc_dentry(proc_dentry2);
+		if (proc_dentry1)
+			clean_proc_dentry(proc_dentry1);
+		if (proc_dentry2)
+			clean_proc_dentry(proc_dentry2);
 
 		if (state != TASK_ZOMBIE)
 			BUG();
diff -urN linux-2.5.65-base/include/linux/proc_fs.h linux-2.5.65-unhash_process/include/linux/proc_fs.h
--- linux-2.5.65-base/include/linux/proc_fs.h	2003-03-18 03:13:37.000000000 +0530
+++ linux-2.5.65-unhash_process/include/linux/proc_fs.h	2003-03-24 16:47:10.000000000 +0530
@@ -224,4 +224,5 @@
 	return PROC_I(inode)->pde;
 }
 
+extern void clean_proc_dentry(struct dentry *proc_dentry);
 #endif /* _LINUX_PROC_FS_H */
diff -urN linux-2.5.65-base/kernel/exit.c linux-2.5.65-unhash_process/kernel/exit.c
--- linux-2.5.65-base/kernel/exit.c	2003-03-18 03:14:43.000000000 +0530
+++ linux-2.5.65-unhash_process/kernel/exit.c	2003-03-24 16:46:40.000000000 +0530
@@ -21,6 +21,7 @@
 #include <linux/ptrace.h>
 #include <linux/profile.h>
 #include <linux/mount.h>
+#include <linux/proc_fs.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
@@ -33,7 +34,6 @@
 
 static struct dentry * __unhash_process(struct task_struct *p)
 {
-	struct dentry *proc_dentry;
 
 	nr_threads--;
 	detach_pid(p, PIDTYPE_PID);
@@ -46,17 +46,8 @@
 	}
 
 	REMOVE_LINKS(p);
-	proc_dentry = p->proc_dentry;
-	if (unlikely(proc_dentry != NULL)) {
-		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;
+
+	return p->proc_dentry;
 }
 
 void release_task(struct task_struct * p)
@@ -93,10 +84,9 @@
 	sched_exit(p);
 	write_unlock_irq(&tasklist_lock);
 
-	if (unlikely(proc_dentry != NULL)) {
-		shrink_dcache_parent(proc_dentry);
-		dput(proc_dentry);
-	}
+	if (unlikely(proc_dentry != NULL))
+		clean_proc_dentry(proc_dentry);
+
 	release_thread(p);
 	put_task_struct(p);
 }
@@ -111,10 +101,8 @@
 	proc_dentry = __unhash_process(p);
 	write_unlock_irq(&tasklist_lock);
 
-	if (unlikely(proc_dentry != NULL)) {
-		shrink_dcache_parent(proc_dentry);
-		dput(proc_dentry);
-	}
+	if (unlikely(proc_dentry != NULL)) 
+		clean_proc_dentry(proc_dentry);
 }
 
 /*

-- 
Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: maneesh@in.ibm.com
http://lse.sourceforge.net/