Re: [PATCH] [3/11] SYSCTL: Add proc_rcu_string to manage sysctls using rcu strings

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, Dec 21, 2009 at 07:00:44PM -0800, Eric W. Biederman wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> 
> > On Mon, Dec 21, 2009 at 02:20:24AM +0100, Andi Kleen wrote:
> >> 
> >> Add a helper to use the new rcu strings for managing access
> >> to text sysctls. Conversions will be in follow-on patches.
> >> 
> >> An alternative would be to use seqlocks here, but RCU seemed
> >> cleaner.
> >> 
> >> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> >
> > Using the below as an example of my concern about access_rcu_string(), FYI.
> >
> >> ---
> >>  include/linux/sysctl.h |    2 +
> >>  kernel/sysctl.c        |   66 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  kernel/sysctl_check.c  |    1 
> >>  3 files changed, 69 insertions(+)
> >> 
> >> Index: linux-2.6.33-rc1-ak/include/linux/sysctl.h
> >> ===================================================================
> >> --- linux-2.6.33-rc1-ak.orig/include/linux/sysctl.h
> >> +++ linux-2.6.33-rc1-ak/include/linux/sysctl.h
> >> @@ -969,6 +969,8 @@ typedef int proc_handler (struct ctl_tab
> >> 
> >>  extern int proc_dostring(struct ctl_table *, int,
> >>  			 void __user *, size_t *, loff_t *);
> >> +extern int proc_rcu_string(struct ctl_table *, int,
> >> +			 void __user *, size_t *, loff_t *);
> >>  extern int proc_dointvec(struct ctl_table *, int,
> >>  			 void __user *, size_t *, loff_t *);
> >>  extern int proc_dointvec_minmax(struct ctl_table *, int,
> >> Index: linux-2.6.33-rc1-ak/kernel/sysctl.c
> >> ===================================================================
> >> --- linux-2.6.33-rc1-ak.orig/kernel/sysctl.c
> >> +++ linux-2.6.33-rc1-ak/kernel/sysctl.c
> >> @@ -50,6 +50,7 @@
> >>  #include <linux/ftrace.h>
> >>  #include <linux/slow-work.h>
> >>  #include <linux/perf_event.h>
> >> +#include <linux/rcustring.h>
> >> 
> >>  #include <asm/uaccess.h>
> >>  #include <asm/processor.h>
> >> @@ -2016,6 +2017,60 @@ static int _proc_do_string(void* data, i
> >>  }
> >> 
> >>  /**
> >> + * proc_rcu_string - sysctl string with rcu protection
> >> + * @table: the sysctl table
> >> + * @write: %TRUE if this is a write to the sysctl file
> >> + * @buffer: the user buffer
> >> + * @lenp: the size of the user buffer
> >> + * @ppos: file position
> >> + *
> >> + * Handle a string sysctl similar to proc_dostring.
> >> + * The main difference is that the data pointer in the table
> >> + * points to a pointer to a string. The string should be initially
> >> + * pointing to a statically allocated (as a C object, not on the heap)
> >> + * default. When it is replaced old uses will be protected by
> >> + * RCU. The reader should use rcu_read_lock()/unlock() or
> >> + * access_rcu_string().
> >> + */
> >> +int proc_rcu_string(struct ctl_table *table, int write,
> >> +		void __user *buffer, size_t *lenp, loff_t *ppos)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (write) {
> >> +		/* protect writers against each other */
> >> +		static DEFINE_MUTEX(rcu_string_mutex);
> >> +		char *old;
> >> +		char *new;
> >> +
> >> +		new = alloc_rcu_string(table->maxlen, GFP_KERNEL);
> >> +		if (!new)
> >> +			return -ENOMEM;
> >> +		mutex_lock(&rcu_string_mutex);
> >> +		old = *(char **)(table->data);
> >> +		strcpy(new, old);
> >> +		ret = _proc_do_string(new, table->maxlen, write, buffer, lenp, ppos);
> >> +		rcu_assign_pointer(*(char **)(table->data), new);
> >> +		/*
> >> +		 * For the first initialization allow constant strings.
> >> +		 */
> >> +		if (!kernel_address((unsigned long)old))
> >> +			free_rcu_string(old);
> >> +		mutex_unlock(&rcu_string_mutex);
> >> +	} else {
> >> +		char *str;
> >> +
> >> +		str = access_rcu_string(*(char **)(table->data), table->maxlen,
> >> +					GFP_KERNEL);
> >
> > So the above statement picks up table->data, then some other CPU comes
> > in and executes the "write" side of this "if" statement, we get
> > preempted before access_rcu_string() enters its RCU read-side critical
> > section, the grace period elapse, we resume, and ... ouch!
> >
> > One trick would be to make access_rcu_string() be a macro that does
> > first access to its first argument in an RCU read-side critical section.
> > Alternatively, pass in the address of the pointer, rather than the
> > pointer itself.
> >
> > Or explain to me how I am confused.
> 
> That sounds correct to me.  There is also the missing rcu_dereference.
> 
> Which is less important but it would make clear that access_rcu_string
> does the dereference outside of the rcu critical section.

Good point, there does indeed need to be an rcu_dereference() in any case.

							Thanx, Paul