[PATCH] Remove BKL from sysctl(2)

[PATCH] Remove BKL from sysctl(2)

[Alexey Dobriyan]

/proc/sys part of sysctl code runs without BKL held, so BKL during
sysctl(2) is useless. Remove misleading comment and "protection" around
coredumping code -- kernel.core_pattern can be written without BKL.

do_sysctl() and lookup in /proc/sys use identical iterators, so any locking
bug BKL supposedly fixed in sysctl(2) code we should have in /proc/sys
code anyway.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 arch/ia64/ia32/sys_ia32.c         |    2 --
 arch/mips/kernel/linux32.c        |    3 ---
 arch/parisc/kernel/sys_parisc32.c |    3 ---
 arch/powerpc/kernel/sys_ppc32.c   |    3 ---
 arch/s390/kernel/compat_linux.c   |    3 ---
 arch/sparc64/kernel/sys_sparc32.c |    3 ---
 arch/x86/ia32/sys_ia32.c          |    3 ---
 fs/exec.c                         |    7 -------
 kernel/sysctl.c                   |    3 ---
 9 files changed, 30 deletions(-)

--- a/arch/ia64/ia32/sys_ia32.c
+++ b/arch/ia64/ia32/sys_ia32.c
@@ -1991,10 +1991,8 @@ sys32_sysctl (struct sysctl32 __user *args)
 		return -EFAULT;
 
 	set_fs(KERNEL_DS);
-	lock_kernel();
 	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *) &oldlen,
 			newvalp, (size_t) a32.newlen);
-	unlock_kernel();
 	set_fs(old_fs);
 
 	if (oldvalp && put_user (oldlen, (int __user *) compat_ptr(a32.oldlenp)))
--- a/arch/mips/kernel/linux32.c
+++ b/arch/mips/kernel/linux32.c
@@ -9,7 +9,6 @@
 #include <linux/mm.h>
 #include <linux/errno.h>
 #include <linux/file.h>
-#include <linux/smp_lock.h>
 #include <linux/highuid.h>
 #include <linux/dirent.h>
 #include <linux/resource.h>
@@ -440,10 +439,8 @@ asmlinkage long sys32_sysctl(struct sysctl_args32 __user *args)
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)A(tmp.name), tmp.nlen, (void __user *)A(tmp.oldval),
 			  oldlenp, (void __user *)A(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
--- a/arch/parisc/kernel/sys_parisc32.c
+++ b/arch/parisc/kernel/sys_parisc32.c
@@ -21,7 +21,6 @@
 #include <linux/utsname.h>
 #include <linux/time.h>
 #include <linux/smp.h>
-#include <linux/smp_lock.h>
 #include <linux/sem.h>
 #include <linux/msg.h>
 #include <linux/shm.h>
@@ -144,11 +143,9 @@ asmlinkage long sys32_sysctl(struct __sysctl_args32 __user *args)
 		oldlenp = (size_t *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)(u64)tmp.name, tmp.nlen,
 			  (void __user *)(u64)tmp.oldval, oldlenp,
 			  (void __user *)(u64)tmp.newval, tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t *)addr)) {
--- a/arch/powerpc/kernel/sys_ppc32.c
+++ b/arch/powerpc/kernel/sys_ppc32.c
@@ -24,7 +24,6 @@
 #include <linux/times.h>
 #include <linux/utsname.h>
 #include <linux/smp.h>
-#include <linux/smp_lock.h>
 #include <linux/sem.h>
 #include <linux/msg.h>
 #include <linux/shm.h>
@@ -714,11 +713,9 @@ asmlinkage long compat_sys_sysctl(struct __sysctl_args32 __user *args)
 			return -EFAULT;
 	}
 
-	lock_kernel();
 	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
 			  compat_ptr(tmp.oldval), oldlenp,
 			  compat_ptr(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, oldlenp) ||
--- a/arch/s390/kernel/compat_linux.c
+++ b/arch/s390/kernel/compat_linux.c
@@ -26,7 +26,6 @@
 #include <linux/times.h>
 #include <linux/utsname.h>
 #include <linux/smp.h>
-#include <linux/smp_lock.h>
 #include <linux/sem.h>
 #include <linux/msg.h>
 #include <linux/shm.h>
@@ -715,10 +714,8 @@ asmlinkage long sys32_sysctl(struct __sysctl_args32 __user *args)
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen, compat_ptr(tmp.oldval),
 			  oldlenp, compat_ptr(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
--- a/arch/sparc64/kernel/sys_sparc32.c
+++ b/arch/sparc64/kernel/sys_sparc32.c
@@ -18,7 +18,6 @@
 #include <linux/times.h>
 #include <linux/utsname.h>
 #include <linux/smp.h>
-#include <linux/smp_lock.h>
 #include <linux/sem.h>
 #include <linux/msg.h>
 #include <linux/shm.h>
@@ -989,14 +988,12 @@ asmlinkage long sys32_sysctl(struct __sysctl_args32 __user *args)
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)(unsigned long) tmp.name,
 			  tmp.nlen,
 			  (void __user *)(unsigned long) tmp.oldval,
 			  oldlenp,
 			  (void __user *)(unsigned long) tmp.newval,
 			  tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
--- a/arch/x86/ia32/sys_ia32.c
+++ b/arch/x86/ia32/sys_ia32.c
@@ -30,7 +30,6 @@
 #include <linux/times.h>
 #include <linux/utsname.h>
 #include <linux/smp.h>
-#include <linux/smp_lock.h>
 #include <linux/sem.h>
 #include <linux/msg.h>
 #include <linux/mm.h>
@@ -630,10 +629,8 @@ sys32_sysctl(struct sysctl_ia32 __user *args32)
 		return -EFAULT;
 
 	set_fs(KERNEL_DS);
-	lock_kernel();
 	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *)&oldlen,
 			newvalp, (size_t) a32.newlen);
-	unlock_kernel();
 	set_fs(old_fs);
 
 	if (oldvalp && put_user (oldlen, (int __user *)compat_ptr(a32.oldlenp)))
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -28,7 +28,6 @@
 #include <linux/a.out.h>
 #include <linux/stat.h>
 #include <linux/fcntl.h>
-#include <linux/smp_lock.h>
 #include <linux/string.h>
 #include <linux/init.h>
 #include <linux/pagemap.h>
@@ -1720,13 +1719,7 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	 */
 	clear_thread_flag(TIF_SIGPENDING);
 
-	/*
-	 * lock_kernel() because format_corename() is controlled by sysctl, which
-	 * uses lock_kernel()
-	 */
- 	lock_kernel();
 	ispipe = format_corename(corename, core_pattern, signr);
-	unlock_kernel();
 	/*
 	 * Don't bother to check the RLIMIT_CORE value if core_pattern points
 	 * to a pipe.  Since we're not writing directly to the filesystem
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -27,7 +27,6 @@
 #include <linux/security.h>
 #include <linux/ctype.h>
 #include <linux/utsname.h>
-#include <linux/smp_lock.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -1366,10 +1365,8 @@ asmlinkage long sys_sysctl(struct __sysctl_args __user *args)
 	if (error)
 		goto out;
 
-	lock_kernel();
 	error = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, tmp.oldlenp,
 			  tmp.newval, tmp.newlen);
-	unlock_kernel();
 out:
 	return error;
 }

Re: [PATCH] Remove BKL from sysctl(2)

[Ingo Molnar]

* Alexey Dobriyan <adobriyan@gmail.com> wrote:

> /proc/sys part of sysctl code runs without BKL held, so BKL during 
> sysctl(2) is useless. Remove misleading comment and "protection" 
> around coredumping code -- kernel.core_pattern can be written without 
> BKL.
> 
> do_sysctl() and lookup in /proc/sys use identical iterators, so any 
> locking bug BKL supposedly fixed in sysctl(2) code we should have in 
> /proc/sys code anyway.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

cool! You just shortened the grand 10-year-plan of BKL removal by at 
least one year :-)

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH] Remove BKL from sysctl(2)

[Alan Cox]

On Tue, 22 Jan 2008 23:27:27 +0300
Alexey Dobriyan <adobriyan@gmail.com> wrote:

> /proc/sys part of sysctl code runs without BKL held, so BKL during
> sysctl(2) is useless. Remove misleading comment and "protection" around
> coredumping code -- kernel.core_pattern can be written without BKL.
> 
> do_sysctl() and lookup in /proc/sys use identical iterators, so any locking
> bug BKL supposedly fixed in sysctl(2) code we should have in /proc/sys
> code anyway.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

NAK

The core dump locking is now totally unprotected rather than slightly
dubious. This patch needs to go in together with a parallel patch to
actually lock properly. You've made a bug worse not fixed it.

There are cases that updating the corepath name and dumping a core at the
same moment can result in the wrong thing being exec()'d or a file being
opened which is a mix of the old and new name and could go anywhere.

I see two variants on your patch that work

#1	Replace the lock_kernel with a sysctl_update mutex and fix both
paths
#2	Add locking specifically to the corename path



Alan

Re: [PATCH] Remove BKL from sysctl(2)

[Andi Kleen]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> 
> There are cases that updating the corepath name and dumping a core at the
> same moment can result in the wrong thing being exec()'d or a file being
> opened which is a mix of the old and new name and could go anywhere.
> 
> I see two variants on your patch that work
> 
> #1	Replace the lock_kernel with a sysctl_update mutex and fix both
> paths
> #2	Add locking specifically to the corename path

Doesn't that apply to pretty much all proc_dostring users, not just
corename? 

Some of them might be already broken if their readers don't take BKL.
Also possibly some of the proc_dointvec for multiple numbers.

-Andi

Re: [PATCH] Remove BKL from sysctl(2)

[Alan Cox]

On 23 Jan 2008 12:09:05 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> > 
> > There are cases that updating the corepath name and dumping a core at the
> > same moment can result in the wrong thing being exec()'d or a file being
> > opened which is a mix of the old and new name and could go anywhere.
> > 
> > I see two variants on your patch that work
> > 
> > #1	Replace the lock_kernel with a sysctl_update mutex and fix both
> > paths
> > #2	Add locking specifically to the corename path
> 
> Doesn't that apply to pretty much all proc_dostring users, not just
> corename? 
> 
> Some of them might be already broken if their readers don't take BKL.
> Also possibly some of the proc_dointvec for multiple numbers.

Yes - that's why I am wondering if we want a general 'sysctl' mutex or
just to fix the specific case. The corename case is nasty as it is timing
dependant against user activity rather than the less interesting "root
can be silly" type of problem.

If we have a general lock we can then fix the string cases, or even add a
sysctl_string_get() type function.

Re: [PATCH] Remove BKL from sysctl(2)

[Andi Kleen]

> Yes - that's why I am wondering if we want a general 'sysctl' mutex or
> just to fix the specific case. The corename case is nasty as it is timing
> dependant against user activity rather than the less interesting "root
> can be silly" type of problem.

I think for the strings it would be better to just do a kind of copy-on-write.
As in don't use the array directly, but a pointer instead and then sysctl 
allocates a new string, switches the pointer around
and then does a RCU delayed free on the old string if it wasn't in .data.

This would also have the advantage that the small upper limits these 
strings currently have are gone and actually save a little memory in 
the common case of them not being changed.

Only case that wouldn't fix would be someone keeping the string accessed
over a sleep point. And for preemptible kernels it would need rcu_read_lock().
So no free lunch without code auditing. But it would be probably the best
way to go forward longer term.

For multi number arrays the code ideally needs to be robust against temporary
inconsistencies.

BTW while interesting in theory in practice i suspect the actual likelyhood
of a user actually hitting such a race is pretty small because sysctls tend
to be set up a boot only where not much is going on.

-Andi

[PATCH 2/2] Race-free kernel.core_pattern

[Alexey Dobriyan]

On Wed, Jan 23, 2008 at 10:51:40AM +0000, Alan Cox wrote:
> On Tue, 22 Jan 2008 23:27:27 +0300
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > /proc/sys part of sysctl code runs without BKL held, so BKL during
> > sysctl(2) is useless. Remove misleading comment and "protection" around
> > coredumping code -- kernel.core_pattern can be written without BKL.
> > 
> > do_sysctl() and lookup in /proc/sys use identical iterators, so any locking
> > bug BKL supposedly fixed in sysctl(2) code we should have in /proc/sys
> > code anyway.
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> 
> NAK
> 
> The core dump locking is now totally unprotected rather than slightly
> dubious. This patch needs to go in together with a parallel patch to
> actually lock properly. You've made a bug worse not fixed it.

Techically, yes.

> There are cases that updating the corepath name and dumping a core at the
> same moment can result in the wrong thing being exec()'d or a file being
> opened which is a mix of the old and new name and could go anywhere.

Yup, that's the bug.

Below is companion patch to be applied after BKL removal which fixes it:



[PATCH 2/2] Race-free kernel.core_pattern

Alan correctly notices that playing with kernel.core_pattern while coredumping
is in flight can lead to unpredictable name of coredump files or wrong piped to
wrong executable. Fix that by taking mutex around formatting corename and
changing core_pattern via both /proc/sys/kernel/core_pattern and
{CTL_KERN, KERN_CORE_PATTERN}.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/exec.c                |   32 ++++++++++++++++++++++++++++++++
 include/linux/coredump.h |   31 +++++++++++++++++++++++++++++++
 kernel/sysctl.c          |    7 +++----
 3 files changed, 66 insertions(+), 4 deletions(-)

--- a/fs/exec.c
+++ b/fs/exec.c
@@ -41,6 +41,8 @@
 #include <linux/pid_namespace.h>
 #include <linux/module.h>
 #include <linux/namei.h>
+#include <linux/coredump.h>
+#include <linux/sysctl.h>
 #include <linux/proc_fs.h>
 #include <linux/ptrace.h>
 #include <linux/mount.h>
@@ -61,6 +63,7 @@
 
 int core_uses_pid;
 char core_pattern[CORENAME_MAX_SIZE] = "core";
+DEFINE_MUTEX(core_pattern_mutex);
 int suid_dumpable = 0;
 
 /* The maximal length of core_pattern is also specified in sysctl.c */
@@ -1585,6 +1588,33 @@ done:
 	return mm->core_waiters;
 }
 
+#ifdef CONFIG_PROC_SYSCTL
+int proc_core_pattern(struct ctl_table *table, int write, struct file *file,
+		      void __user *buf, size_t *lenp, loff_t *ppos)
+{
+	int rv;
+
+	mutex_lock(&core_pattern_mutex);
+	rv = proc_dostring(table, write, file, buf, lenp, ppos);
+	mutex_unlock(&core_pattern_mutex);
+	return rv;
+}
+#endif
+
+#ifdef CONFIG_SYSCTL_SYSCALL
+int sysctl_core_pattern(struct ctl_table *table, int __user *name, int nlen,
+			void __user *oldval, size_t __user *oldlenp,
+			void __user *newval, size_t newlen)
+{
+	int rv;
+
+	mutex_lock(&core_pattern_mutex);
+	rv = sysctl_string(table, name, nlen, oldval, oldlenp, newval, newlen);
+	mutex_unlock(&core_pattern_mutex);
+	return rv;
+}
+#endif
+
 static int coredump_wait(int exit_code)
 {
 	struct task_struct *tsk = current;
@@ -1719,7 +1749,9 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	 */
 	clear_thread_flag(TIF_SIGPENDING);
 
+	mutex_lock(&core_pattern_mutex);
 	ispipe = format_corename(corename, core_pattern, signr);
+	mutex_unlock(&core_pattern_mutex);
 	/*
 	 * Don't bother to check the RLIMIT_CORE value if core_pattern points
 	 * to a pipe.  Since we're not writing directly to the filesystem
new file mode 100644
--- /dev/null
+++ b/include/linux/coredump.h
@@ -0,0 +1,31 @@
+#ifndef __INCLUDE_LINUX_COREDUMP_H
+#define __INCLUDE_LINUX_COREDUMP_H
+
+#include <linux/compiler.h>
+#include <linux/mutex.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+struct ctl_table;
+struct file;
+
+extern char core_pattern[];
+extern int core_uses_pid;
+extern struct mutex core_pattern_mutex;
+
+#ifdef CONFIG_PROC_SYSCTL
+int proc_core_pattern(struct ctl_table *ctl, int write, struct file *file,
+		      void __user *buffer, size_t *lenp, loff_t *ppos);
+#else
+#define proc_core_pattern NULL
+#endif
+
+#ifdef CONFIG_SYSCTL_SYSCALL
+int sysctl_core_pattern(struct ctl_table *table, int __user *name, int nlen,
+			void __user *oldval, size_t __user *oldlenp,
+			void __user *newval, size_t newlen);
+#else
+#define sysctl_core_pattern NULL
+#endif
+
+#endif
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -26,6 +26,7 @@
 #include <linux/proc_fs.h>
 #include <linux/security.h>
 #include <linux/ctype.h>
+#include <linux/coredump.h>
 #include <linux/utsname.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -66,9 +67,7 @@ extern int sysctl_overcommit_ratio;
 extern int sysctl_panic_on_oom;
 extern int sysctl_oom_kill_allocating_task;
 extern int max_threads;
-extern int core_uses_pid;
 extern int suid_dumpable;
-extern char core_pattern[];
 extern int pid_max;
 extern int min_free_kbytes;
 extern int printk_ratelimit_jiffies;
@@ -369,8 +368,8 @@ static struct ctl_table kern_table[] = {
 		.data		= core_pattern,
 		.maxlen		= CORENAME_MAX_SIZE,
 		.mode		= 0644,
-		.proc_handler	= &proc_dostring,
-		.strategy	= &sysctl_string,
+		.proc_handler	= proc_core_pattern,
+		.strategy	= sysctl_core_pattern,
 	},
 #ifdef CONFIG_PROC_SYSCTL
 	{

Re: [PATCH 2/2] Race-free kernel.core_pattern

[Andi Kleen]

> Yup, that's the bug.
> 
> Below is companion patch to be applied after BKL removal which fixes it:

Still there are other do_string (and possibly multiple number integer) users 
which are likely buggy in the same way. It's probably better to find some more 
generic mechanism instead of adding a new lock for each of them.

-Andi