Re: Problems with string (charp) module parameters

[Takashi Iwai <takashi.iwai@gmail.com>]

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.

Anyway, I've thought of this for a while, but couldn't find any smart solution.
Judging from the current usage pattern, I think we should rather take
a simpler solution:
don't allow sysfs-write for charp type but use string type for
writable parameters.

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.)

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.


thanks,

Takashi