Re: Problems with string (charp) module parameters

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

On Fri, Oct 23, 2009 at 12:50:41AM +1030, Rusty Russell wrote:
> On Thu, 22 Oct 2009 12:43:36 pm Takashi Iwai wrote:
> > Hi Rusty,
> > 
> > (sending from gmail address since VPN doesn't work here in hotel...)
> > 
> > On Wed, Oct 21, 2009 at 3:11 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > On Tue, 13 Oct 2009 09:07:46 pm Takashi Iwai wrote:
> > >> * The handling of parameter array is pretty buggy now.
> > >>   kp->perm and kp->flags aren't properly initialized in
> > >>   param_array().  Thus, you might call kfree() with invalid pointers,
> > >>   or pass a wrong type for bool.
> > >
> > > Yes, an array of charp isn't going to work.  Erk, I switched one bug for
> > > another :(
> > >
> > >> So, the situation looks messy right now, not only about the section
> > >> issue.  If we allow kmalloc of each parameter array element, the flag
> > >> must be associated to each element, not a global one to the array.
> > >>
> > >> Thoughts?
> > >
> > > Yes, that's hard.  There's only one place which currently has a writable
> > > array parameter: drivers/usb/atm/ueagle-atm.c, and it's root only.
> > >
> > > OK, for 2.6.32, we remove the const.  In the longer term, I'm reworking how
> > > this is done entirely.
> > 
> > As far as I checked, removing only const doesn't suffice on x86.
> > The problem is rather the __param section assignment.
> > We'd need to get rid of that, too, if we keep the code in the current way.
> 
> Something like this?
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -147,6 +147,10 @@
>  	MEM_KEEP(init.data)						\
>  	MEM_KEEP(exit.data)						\
>  	. = ALIGN(8);							\
> +	VMLINUX_SYMBOL(__start___param) = .;				\
> +	*(__param)							\
> +	VMLINUX_SYMBOL(__stop___param) = .;				\
> +	. = ALIGN(8);							\
>  	VMLINUX_SYMBOL(__start___markers) = .;				\
>  	*(__markers)							\
>  	VMLINUX_SYMBOL(__stop___markers) = .;				\
> @@ -336,15 +340,7 @@
>  		MEM_KEEP(init.rodata)					\
>  		MEM_KEEP(exit.rodata)					\
>  	}								\
> -									\
> -	/* Built-in module parameters. */				\
> -	__param : AT(ADDR(__param) - LOAD_OFFSET) {			\
> -		VMLINUX_SYMBOL(__start___param) = .;			\
> -		*(__param)						\
> -		VMLINUX_SYMBOL(__stop___param) = .;			\
> -		. = ALIGN((align));					\
> -		VMLINUX_SYMBOL(__end_rodata) = .;			\
> -	}								\
> +	VMLINUX_SYMBOL(__end_rodata) = .;				\
>  	. = ALIGN((align));
> 
>  /* RODATA & RO_DATA provided for backward compatibility.
> 
> 
> This would work for 2.6.32, for 2.6.33 I have a different solution.
> 
> > It's not only for avoiding the mess to separate static and kmalloc
> > strings but also for
> > avoiding races between the referrer and the sysfs-write of char
> > pointer.  (In general, we
> > have no lock for parameters.)
> 
> Good point.  We should use rcu here.  But there's still a race with copying
> in strings of any kind.

Assuming that I actually understand the problem...  ;-)

The usual way of handling this sort of race is to allocate (or have
pre-allocated) a block of memory to hold the new string, copy it,
then publish a pointer to it.  That way readers either see the entire
new string or they don't, no partially copied strings.

						Thanx, Paul

> > As you pointed out, there are no many users of writable charp parameters.
> > So, replacing is easy task.  In that way, we can keep struct
> > kernel_parameter as const
> > gracefully without hustling any big code change.
> 
> But we'd need to make sure noone adds one in future.  After all, you tried
> to add one and found this problem!
> 
> I'll post my current patch series: it needs testing, but I'd appreciate
> your thoughts.
> 
> Thanks!
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/