+ sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

+ sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[akpm]

The patch titled
     sysctl: use RCU protected sysctl for ocfs group add helper
has been added to the -mm tree.  Its filename is
     sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: sysctl: use RCU protected sysctl for ocfs group add helper
From: Andi Kleen <andi@firstfloor.org>

This avoids races with unlocked sysctl()

Also saves ~220 bytes in the data segment.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Cc: "Paul E. McKenney" <paulmck@us.ibm.com>
Cc: Russell King <rmk+lkml@arm.linux.org.uk>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Joel Becker <Joel.Becker@oracle.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ocfs2/stackglue.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff -puN fs/ocfs2/stackglue.c~sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper fs/ocfs2/stackglue.c
--- a/fs/ocfs2/stackglue.c~sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper
+++ a/fs/ocfs2/stackglue.c
@@ -27,6 +27,7 @@
 #include <linux/kobject.h>
 #include <linux/sysfs.h>
 #include <linux/sysctl.h>
+#include <linux/rcustring.h>
 
 #include "ocfs2_fs.h"
 
@@ -40,7 +41,7 @@ static struct ocfs2_locking_protocol *lp
 static DEFINE_SPINLOCK(ocfs2_stack_lock);
 static LIST_HEAD(ocfs2_stack_list);
 static char cluster_stack_name[OCFS2_STACK_LABEL_LEN + 1];
-static char ocfs2_hb_ctl_path[OCFS2_MAX_HB_CTL_PATH] = "/sbin/ocfs2_hb_ctl";
+static char *ocfs2_hb_ctl_path = "/sbin/ocfs2_hb_ctl";
 
 /*
  * The stack currently in use.  If not null, active_stack->sp_count > 0,
@@ -395,8 +396,15 @@ static void ocfs2_leave_group(const char
 {
 	int ret;
 	char *argv[5], *envp[3];
+	char *helper;
 
-	argv[0] = ocfs2_hb_ctl_path;
+	helper = access_rcu_string(&ocfs2_hb_ctl_path, OCFS2_MAX_HB_CTL_PATH, GFP_KERNEL);
+	if (!helper) {
+		printk(KERN_ERR "ocfs2_leave_group: no memory\n");
+		return;
+	}
+
+	argv[0] = helper;
 	argv[1] = "-K";
 	argv[2] = "-u";
 	argv[3] = (char *)group;
@@ -414,6 +422,7 @@ static void ocfs2_leave_group(const char
 		       "\"%s %s %s %s\"\n",
 		       ret, argv[0], argv[1], argv[2], argv[3]);
 	}
+	kfree(helper);
 }
 
 /*
@@ -621,10 +630,10 @@ error:
 static ctl_table ocfs2_nm_table[] = {
 	{
 		.procname	= "hb_ctl_path",
-		.data		= ocfs2_hb_ctl_path,
+		.data		= &ocfs2_hb_ctl_path,
 		.maxlen		= OCFS2_MAX_HB_CTL_PATH,
 		.mode		= 0644,
-		.proc_handler	= proc_dostring,
+		.proc_handler	= proc_rcu_string,
 	},
 	{ }
 };
_

Patches currently in -mm which might be from andi@firstfloor.org are

kernel-signalc-fix-kernel-information-leak-with-print-fatal-signals=1.patch
proc-revert-procfs-provide-stack-information-for-threads.patch
kfifo-use-void-pointers-for-user-buffers.patch
kfifo-sanitize-_user-error-handling.patch
kfifo-add-kfifo_out_peek.patch
kfifo-add-kfifo_initialized.patch
kfifo-document-everywhere-that-size-has-to-be-power-of-two.patch
hardware-latency-detector-remove-default-m.patch
kbuild-move-fno-dwarf2-cfi-asm-to-powerpc-only.patch
mm-introduce-dump_page-and-print-symbolic-flag-names.patch
coredump-unify-dump_seek-implementations-for-each-binfmt_c.patch
coredump-move-dump_write-and-dump_seek-into-a-header-file.patch
elf-coredump-replace-elf_core_extra_-macros-by-functions.patch
elf-coredump-make-offset-calculation-process-and-writing-process-explicit.patch
elf-coredump-add-extended-numbering-support.patch
tracehooks-kill-some-pt_ptraced-checks.patch
tracehooks-check-pt_ptraced-before-reporting-the-single-step.patch
ptrace_signal-check-pt_ptraced-before-reporting-a-signal.patch
export-__ptrace_detach-and-do_notify_parent_cldstop.patch
reorder-the-code-in-kernel-ptracec.patch
implement-utrace-ptrace.patch
utrace-core.patch
rcu-add-rcustring-adt-for-rcu-protected-strings.patch
add-a-kernel_address-that-works-for-data-too.patch
sysctl-add-proc_rcu_string-to-manage-sysctls-using-rcu-strings.patch
sysctl-use-rcu-strings-for-core_pattern-sysctl.patch
sysctl-add-call_usermodehelper_cleanup.patch
sysctl-convert-modprobe_path-to-proc_rcu_string.patch
sysctl-convert-poweroff_command-to-proc_rcu_string.patch
sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch
sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Joel Becker]

On Fri, Jan 08, 2010 at 03:54:20PM -0800, akpm at linux-foundation.org wrote:
> 
> The patch titled
>      sysctl: use RCU protected sysctl for ocfs group add helper
> has been added to the -mm tree.  Its filename is
>      sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch

NAK until I can figure out how to make it always succeed.  We can't have
umount "succeed" like this.

Maybe the solution is GFP_ATOMIC.  Maybe the solution is to lock on the
setting end.  But this isn't it.

Joel

[Not eliding the patch so ocfs2-devel can see it]

> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> 
> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> out what to do about this
> 
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> 
> ------------------------------------------------------
> Subject: sysctl: use RCU protected sysctl for ocfs group add helper
> From: Andi Kleen <andi@firstfloor.org>
> 
> This avoids races with unlocked sysctl()
> 
> Also saves ~220 bytes in the data segment.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> Cc: "Paul E. McKenney" <paulmck@us.ibm.com>
> Cc: Russell King <rmk+lkml@arm.linux.org.uk>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Joel Becker <Joel.Becker@oracle.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/ocfs2/stackglue.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff -puN fs/ocfs2/stackglue.c~sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper fs/ocfs2/stackglue.c
> --- a/fs/ocfs2/stackglue.c~sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper
> +++ a/fs/ocfs2/stackglue.c
> @@ -27,6 +27,7 @@
>  #include <linux/kobject.h>
>  #include <linux/sysfs.h>
>  #include <linux/sysctl.h>
> +#include <linux/rcustring.h>
>  
>  #include "ocfs2_fs.h"
>  
> @@ -40,7 +41,7 @@ static struct ocfs2_locking_protocol *lp
>  static DEFINE_SPINLOCK(ocfs2_stack_lock);
>  static LIST_HEAD(ocfs2_stack_list);
>  static char cluster_stack_name[OCFS2_STACK_LABEL_LEN + 1];
> -static char ocfs2_hb_ctl_path[OCFS2_MAX_HB_CTL_PATH] = "/sbin/ocfs2_hb_ctl";
> +static char *ocfs2_hb_ctl_path = "/sbin/ocfs2_hb_ctl";
>  
>  /*
>   * The stack currently in use.  If not null, active_stack->sp_count > 0,
> @@ -395,8 +396,15 @@ static void ocfs2_leave_group(const char
>  {
>  	int ret;
>  	char *argv[5], *envp[3];
> +	char *helper;
>  
> -	argv[0] = ocfs2_hb_ctl_path;
> +	helper = access_rcu_string(&ocfs2_hb_ctl_path, OCFS2_MAX_HB_CTL_PATH, GFP_KERNEL);
> +	if (!helper) {
> +		printk(KERN_ERR "ocfs2_leave_group: no memory\n");
> +		return;
> +	}
> +
> +	argv[0] = helper;
>  	argv[1] = "-K";
>  	argv[2] = "-u";
>  	argv[3] = (char *)group;
> @@ -414,6 +422,7 @@ static void ocfs2_leave_group(const char
>  		       "\"%s %s %s %s\"\n",
>  		       ret, argv[0], argv[1], argv[2], argv[3]);
>  	}
> +	kfree(helper);
>  }
>  
>  /*
> @@ -621,10 +630,10 @@ error:
>  static ctl_table ocfs2_nm_table[] = {
>  	{
>  		.procname	= "hb_ctl_path",
> -		.data		= ocfs2_hb_ctl_path,
> +		.data		= &ocfs2_hb_ctl_path,
>  		.maxlen		= OCFS2_MAX_HB_CTL_PATH,
>  		.mode		= 0644,
> -		.proc_handler	= proc_dostring,
> +		.proc_handler	= proc_rcu_string,
>  	},
>  	{ }
>  };
> _
> 
> Patches currently in -mm which might be from andi at firstfloor.org are
> 
> kernel-signalc-fix-kernel-information-leak-with-print-fatal-signals=1.patch
> proc-revert-procfs-provide-stack-information-for-threads.patch
> kfifo-use-void-pointers-for-user-buffers.patch
> kfifo-sanitize-_user-error-handling.patch
> kfifo-add-kfifo_out_peek.patch
> kfifo-add-kfifo_initialized.patch
> kfifo-document-everywhere-that-size-has-to-be-power-of-two.patch
> hardware-latency-detector-remove-default-m.patch
> kbuild-move-fno-dwarf2-cfi-asm-to-powerpc-only.patch
> mm-introduce-dump_page-and-print-symbolic-flag-names.patch
> coredump-unify-dump_seek-implementations-for-each-binfmt_c.patch
> coredump-move-dump_write-and-dump_seek-into-a-header-file.patch
> elf-coredump-replace-elf_core_extra_-macros-by-functions.patch
> elf-coredump-make-offset-calculation-process-and-writing-process-explicit.patch
> elf-coredump-add-extended-numbering-support.patch
> tracehooks-kill-some-pt_ptraced-checks.patch
> tracehooks-check-pt_ptraced-before-reporting-the-single-step.patch
> ptrace_signal-check-pt_ptraced-before-reporting-a-signal.patch
> export-__ptrace_detach-and-do_notify_parent_cldstop.patch
> reorder-the-code-in-kernel-ptracec.patch
> implement-utrace-ptrace.patch
> utrace-core.patch
> rcu-add-rcustring-adt-for-rcu-protected-strings.patch
> add-a-kernel_address-that-works-for-data-too.patch
> sysctl-add-proc_rcu_string-to-manage-sysctls-using-rcu-strings.patch
> sysctl-use-rcu-strings-for-core_pattern-sysctl.patch
> sysctl-add-call_usermodehelper_cleanup.patch
> sysctl-convert-modprobe_path-to-proc_rcu_string.patch
> sysctl-convert-poweroff_command-to-proc_rcu_string.patch
> sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch
> sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch
> 

-- 

"Born under a bad sign.
 I been down since I began to crawl.
 If it wasn't for bad luck,
 I wouldn't have no luck at all."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Eric W. Biederman]

Joel Becker <Joel.Becker@oracle.com> writes:

> On Fri, Jan 08, 2010 at 03:54:20PM -0800, akpm at linux-foundation.org wrote:
>> 
>> The patch titled
>>      sysctl: use RCU protected sysctl for ocfs group add helper
>> has been added to the -mm tree.  Its filename is
>>      sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch
>
> NAK until I can figure out how to make it always succeed.  We can't have
> umount "succeed" like this.
>
> Maybe the solution is GFP_ATOMIC.  Maybe the solution is to lock on the
> setting end.  But this isn't it.

My observation is that most of the issues rcu_string are memory allocation
issues.

Suppose we define rcu_string as:

struct rcu_string_head {
        char *str;
	size_t size;
        char data[];
}

#define DEFINE_RCU_STRING(NAME, SIZE, VAL)	\
struct {					\
	char *str;       			\
	size_t size;				\
        char data[SIZE + SIZE];		        \
} NAME = {					\
	.str = NAME.data,			\
        .size = SIZE,				\
        .data = VAL,				\
}

Accesses would be:
        char *str; 
	rcu_read_lock();
        str = rcu_deref(NAME.str);
        ....
	rcu_read_unlock();

Updates would be:
        char *next;
	mutex_lock(&rcu_string_mutex);
        next = rcu_string->data;
        if (next == rcu_string->str)
        	next = rcu_string->data + rcu_string->size;
        memcpy(next, somestring_from_somewhere, rcu_string->size);
        next[rcu_string->size - 1] = '\0';
        rcu_string->str = next;
        synchronize_rcu();
        mutex_unlock(&rcu_string_mutex);

Thoughts?

Eric

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Andrew Morton]

On Fri, 08 Jan 2010 16:55:07 -0800 ebiederm at xmission.com (Eric W. Biederman) wrote:

> My observation is that most of the issues rcu_string are memory allocation
> issues.

My observation is that every commit email I send out prominently includes the text

: Before you just go and hit "reply", please:
:    a) Consider who else should be cc'ed
:    b) Prefer to cc a suitable mailing list as well
:    c) Ideally: find the original patch on the mailing list and do a
:       reply-to-all to that, adding suitable additional cc's

<gives Joel a wedgie>

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Joel Becker]

On Fri, Jan 08, 2010 at 05:41:51PM -0800, Andrew Morton wrote:
> My observation is that every commit email I send out prominently includes the text
> 
> : Before you just go and hit "reply", please:
> :    a) Consider who else should be cc'ed
> :    b) Prefer to cc a suitable mailing list as well
> :    c) Ideally: find the original patch on the mailing list and do a
> :       reply-to-all to that, adding suitable additional cc's
> 
> <gives Joel a wedgie>

1) I added ocfs2-devel, as (b).
2) As to (c), I don't have the original mail in my mailbox. I think I
   must have accidentally deleted it without noticing the ocfs (sic) in
   the subject.  This sometimes happens in the torrent of lkml (I filter
   ocfs2-devel to its own mailbox, but I don't think Andi CC'd that).
   If you're just saying "please remove mm-commits from the reply", I
   can do that next time this happens.
3) Ow!  ;-)

Joel

-- 

"Sometimes one pays most for the things one gets for nothing."
        - Albert Einstein

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Andrew Morton]

On Fri, 8 Jan 2010 19:00:52 -0800 Joel Becker <Joel.Becker@oracle.com> wrote:

> On Fri, Jan 08, 2010 at 05:41:51PM -0800, Andrew Morton wrote:
> > My observation is that every commit email I send out prominently includes the text
> > 
> > : Before you just go and hit "reply", please:
> > :    a) Consider who else should be cc'ed
> > :    b) Prefer to cc a suitable mailing list as well
> > :    c) Ideally: find the original patch on the mailing list and do a
> > :       reply-to-all to that, adding suitable additional cc's
> > 
> > <gives Joel a wedgie>
> 
> 1) I added ocfs2-devel, as (b).

heh, oop, so you did, sorry.

> 3) Ow!  ;-)

OK, that's one you owe me back.  I can take it!

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Andi Kleen]

> Accesses would be:
>         char *str; 
> 	rcu_read_lock();
>         str = rcu_deref(NAME.str);
>         ....
> 	rcu_read_unlock();
> 
> Updates would be:
>         char *next;
> 	mutex_lock(&rcu_string_mutex);
>         next = rcu_string->data;
>         if (next == rcu_string->str)
>         	next = rcu_string->data + rcu_string->size;
>         memcpy(next, somestring_from_somewhere, rcu_string->size);
>         next[rcu_string->size - 1] = '\0';
>         rcu_string->str = next;
>         synchronize_rcu();
>         mutex_unlock(&rcu_string_mutex);

synchronize_rcu inside a lock is a bit nasty, but ok none of these
are critical.

> 
> Thoughts?

It's not fully clear to me how this is better than my solution?

-Andi
-- 
ak at linux.intel.com -- Speaking for myself only.

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Andi Kleen]

On Fri, Jan 08, 2010 at 04:08:53PM -0800, Joel Becker wrote:
> On Fri, Jan 08, 2010 at 03:54:20PM -0800, akpm at linux-foundation.org wrote:
> > 
> > The patch titled
> >      sysctl: use RCU protected sysctl for ocfs group add helper
> > has been added to the -mm tree.  Its filename is
> >      sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch
> 
> NAK until I can figure out how to make it always succeed.  We can't have
> umount "succeed" like this.

You can never make a usermode helper execution always succeed. fork()+exec()
can always fail for a zillion different reasons, for example memory
allocation. Even if you patched these paths to preallocate
everything it could still bump into other global process limits.

> 
> Maybe the solution is GFP_ATOMIC.  Maybe the solution is to lock on the
> setting end.  But this isn't it.

There is no solution if you goal is to make it always succeed,

My change doesn't change this anyways.

Besides right now you plain just have a race that can potentially
crash the kernel when the helper is executed in the wrong
window.

-Andi

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Joel Becker]

On Sat, Jan 09, 2010 at 07:44:38AM +0100, Andi Kleen wrote:
> > NAK until I can figure out how to make it always succeed.  We can't have
> > umount "succeed" like this.
> 
> You can never make a usermode helper execution always succeed. fork()+exec()
> can always fail for a zillion different reasons, for example memory
> allocation. Even if you patched these paths to preallocate
> everything it could still bump into other global process limits.

	Sure, usermode helpers can fail for a variety of reasons.  Why
add one?  Especially a kernel one.  Userspace memory is much more
flexible.

> > Maybe the solution is GFP_ATOMIC.  Maybe the solution is to lock on the
> > setting end.  But this isn't it.
> 
> There is no solution if you goal is to make it always succeed,

	My goal is to surprise the sysadmin as little as possible.

> Besides right now you plain just have a race that can potentially
> crash the kernel when the helper is executed in the wrong
> window.

	Oh, the race needs to be fixed.  I can put a mutex around the
string, or I can put GFP_ATOMIC into your patch.  I was wondering which
was preferable.

Joel

-- 

"The one important thing i have learned over the years is the
 difference between taking one's work seriously and taking one's self
 seriously.  The first is imperative and the second is disastrous."
	-Margot Fonteyn

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] + sysctl-use-rcu-protected-sysctl-for-ocfs-group-add-helper.patch added to -mm tree

[Andi Kleen]

On Fri, Jan 08, 2010 at 11:42:46PM -0800, Joel Becker wrote:
> On Sat, Jan 09, 2010 at 07:44:38AM +0100, Andi Kleen wrote:
> > > NAK until I can figure out how to make it always succeed.  We can't have
> > > umount "succeed" like this.
> > 
> > You can never make a usermode helper execution always succeed. fork()+exec()
> > can always fail for a zillion different reasons, for example memory
> > allocation. Even if you patched these paths to preallocate
> > everything it could still bump into other global process limits.
> 
> 	Sure, usermode helpers can fail for a variety of reasons.  Why
> add one?  Especially a kernel one.  Userspace memory is much more

There are already a multitude of ways the execution of the user mode
helper can fail in the kernel. Just take a look at all the error
conditions in fork.c and exec.c

Besides when memory allocation fails the system is typically in a already
mostly unusable state.

> 
> > > Maybe the solution is GFP_ATOMIC.  Maybe the solution is to lock on the
> > > setting end.  But this isn't it.
> > 
> > There is no solution if you goal is to make it always succeed,
> 
> 	My goal is to surprise the sysadmin as little as possible.

Again if your goal is to make the kernel execution of the user mode
helper never fail then you already missed it.

> > Besides right now you plain just have a race that can potentially
> > crash the kernel when the helper is executed in the wrong
> > window.
> 
> 	Oh, the race needs to be fixed.  I can put a mutex around the
> string, or I can put GFP_ATOMIC into your patch.  I was wondering which
> was preferable.

You can do that, but it doesn't buy you anything over rcu strings 
and in addition would be more custom code.

-Andi
-- 
ak at linux.intel.com -- Speaking for myself only.