[PATCH] better locking in fs/proc/generic.c (bug 1552)

[PATCH] better locking in fs/proc/generic.c (bug 1552)

[Nathan Lynch]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Hi-

I have been experiencing various oopses in the procfs code when rapidly 
adding and removing /proc entries.  It seems that the main problem is 
lack of proper locking around code that traverses or modifies the 
proc_dir_entry tree.  There is also the matter of proc_kill_inodes() 
clearing the file pointer's f_op, which caused oopses in vfs_read() in 
my testing.

I have followed the example of proc_lookup() and used 
lock_kernel/unlock_kernel around critical sections.  I'm not much of a 
filesystem person, so I would appreciate a review of the patch from 
folks more knowledgeable.  I have been testing the code on a couple of 
dual processor machines (i386 and ppc64) without incident.

Please see http://bugme.osdl.org/show_bug.cgi?id=1552 for oops traces 
and a testcase.  Andrew posted a one-liner patch there which was 
included in mm4 which is also needed.

Patch is against 2.6.0-test9-mm4.

Thanks,
Nathan

[-- Attachment #2: proc_generic_locking.patch --]
[-- Type: text/plain, Size: 1711 bytes --]

diff -Naurp linux-2.6.0-test9-mm4.orig/fs/proc/generic.c linux-2.6.0-test9-mm4.new/fs/proc/generic.c
--- linux-2.6.0-test9-mm4.orig/fs/proc/generic.c	2003-10-25 13:44:56.000000000 -0500
+++ linux-2.6.0-test9-mm4.new/fs/proc/generic.c	2003-11-21 14:47:00.000000000 -0600
@@ -260,6 +260,7 @@ static int xlate_proc_name(const char *n
 	int			len;
 
 	de = &proc_root;
+	lock_kernel();
 	while (1) {
 		next = strchr(cp, '/');
 		if (!next)
@@ -270,10 +271,13 @@ static int xlate_proc_name(const char *n
 			if (proc_match(len, cp, de))
 				break;
 		}
-		if (!de)
+		if (!de) {
+			unlock_kernel();
 			return -ENOENT;
+		}
 		cp += len + 1;
 	}
+	unlock_kernel();
 	*residual = cp;
 	*ret = de;
 	return 0;
@@ -462,6 +466,7 @@ static int proc_register(struct proc_dir
 	if (i < 0)
 		return -EAGAIN;
 	dp->low_ino = i;
+	lock_kernel(); /* needed to protect against concurrent registration */
 	dp->next = dir->subdir;
 	dp->parent = dir;
 	dir->subdir = dp;
@@ -480,6 +485,7 @@ static int proc_register(struct proc_dir
 		if (dp->proc_iops == NULL)
 			dp->proc_iops = &proc_file_inode_operations;
 	}
+	unlock_kernel();
 	return 0;
 }
 
@@ -507,7 +513,6 @@ static void proc_kill_inodes(struct proc
 		if (PDE(inode) != de)
 			continue;
 		fops = filp->f_op;
-		filp->f_op = NULL;
 		fops_put(fops);
 	}
 	file_list_unlock();
@@ -643,6 +648,7 @@ void remove_proc_entry(const char *name,
 	if (!parent && xlate_proc_name(name, &parent, &fn) != 0)
 		goto out;
 	len = strlen(fn);
+	lock_kernel();
 	for (p = &parent->subdir; *p; p=&(*p)->next ) {
 		if (!proc_match(len, fn, *p))
 			continue;
@@ -664,6 +670,7 @@ void remove_proc_entry(const char *name,
 		}
 		break;
 	}
+	unlock_kernel();
 out:
 	return;
 }

Re: [PATCH] better locking in fs/proc/generic.c (bug 1552)

[Andrew Morton]

Nathan Lynch <nathanl@austin.ibm.com> wrote:
>
> Hi-
> 
> I have been experiencing various oopses in the procfs code when rapidly 
> adding and removing /proc entries.

I doubt if this particularly surprises anyone.

>  It seems that the main problem is 
> lack of proper locking around code that traverses or modifies the 
> proc_dir_entry tree.  There is also the matter of proc_kill_inodes() 
> clearing the file pointer's f_op, which caused oopses in vfs_read() in 
> my testing.
> 
> I have followed the example of proc_lookup() and used 
> lock_kernel/unlock_kernel around critical sections.  I'm not much of a 
> filesystem person, so I would appreciate a review of the patch from 
> folks more knowledgeable.  I have been testing the code on a couple of 
> dual processor machines (i386 and ppc64) without incident.

The correct lock to use is i_sem on the directory which is subject to
lookup.

> Please see http://bugme.osdl.org/show_bug.cgi?id=1552 for oops traces 
> and a testcase.  Andrew posted a one-liner patch there which was 
> included in mm4 which is also needed.

gargh, I was waiting to hear back on that patch, and there it is.  Durned
bugzilla.  Thanks.