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

From: Nathan Lynch
Date: Fri Nov 21 2003 - 17:36:50 EST


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
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;
}