Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sun, Feb 15, 2009 at 01:02:15PM -0800, Sitsofe Wheeler wrote:
> > From: Frederic Weisbecker <fweisbec@gmail.com>
> 
> > 
> > On Sun, Feb 15, 2009 at 11:24:41AM -0800, Sitsofe Wheeler wrote:
> > > > Could you please enable the CONFIG_DETECT_SOFTLOCKUP option in your kernel?
> > > 
> > > > It's on Kernel Hacking > Detect Soft Lockups.
> > > > With some chances you will have a relevant stacktrace of the lockup, in case
> > > > I couldn't reproduce it.
> > > 
> > > I'll try (the kernel I'm currently using is jam packed with with debug 
> > options) but I'm unconvinced it will unfreeze enough for anything useful...
> > 
> > No need to actually, I guess we found it...
> 
> That's good news - the only way I could have reported the soft lockup stack trace would have been by taking a photo of the screen. The stack trace was pretty much what is seen inside <http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/d4a6ce26360613fa/b362792a2debced6?#b362792a2debced6> anyway. The thing that I'm curious about is why did it show up as an oops last time and as a complete lockup this time?


I don't know :-)

> 
> My one thought is that this is actually a parameter that was not intended to be changed and shouldn't have accepted being written to.


I guess there aren't any module string parameter which expect to be changed like that,
just by changing the pointer value.
Usually, when you have a string parameter, you parse it at load time, and keep the value as an integer.
So it makes only sense to change such values at runtime through a callback given by the module. And that's
what mac80211 does through debugfs.

> However one wonders how the syfs framework was supposed to work in other cases. 


Actually, what I can see is that in case of module_param() with char * types, the permission
of the file is often 0444 or 0, so it is safe.

There are exceptions however:

$ git-grep -E "charp, 04?[123567]+"
arch/um/drivers/hostaudio_kern.c:module_param(dsp, charp, 0644);
arch/um/drivers/hostaudio_kern.c:module_param(mixer, charp, 0644);
drivers/misc/lkdtm.c:module_param(cpoint_name, charp, 0644);
drivers/misc/lkdtm.c:module_param(cpoint_type, charp, 0644);
drivers/net/wireless/libertas/if_sdio.c:module_param_named(helper_name, lbs_helper_name, charp, 0644);
drivers/net/wireless/libertas/if_sdio.c:module_param_named(fw_name, lbs_fw_name, charp, 0644);
drivers/net/wireless/libertas/if_usb.c:module_param_named(fw_name, lbs_fw_name, charp, 0644);
drivers/net/wireless/libertas_tf/if_usb.c:module_param_named(fw_name, lbtf_fw_name, charp, 0644);
drivers/video/vt8623fb.c:module_param(mode_option, charp, 0644);
net/mac80211/rate.c:module_param(ieee80211_default_rc_algo, charp, 0644);

I'm preparing a patchset to set them only readable, it doesn't fix this module bug,
but anyway, such string params writable at runtime don't make any sense.

>It sounds like there is a very
>small window for making your own private copy of whatever sysfs passes to you and under the current system who 
>throws away old strings (given that it looks like pointers are being swapped)? Are they refcounted or something?

When you register a module param, its address is stored inside a struct kernel_param.
Once you change the value of the param, it's totally fine for int/long/boolean...
since no memory is allocated for that, only the integer value is changed on the static memory, though
a module must deal with this asynchronous event given the fact it let it writable by the
user, and I guess it's not always a good idea.

So for an integer called a, you will put a module_param(a, int, ...) which will
allocate a struct kernel_param, and store &a inside.

For params such as strings, its very different, since you register a char *p,
its address is stored in, say,  pp (as a char **).
and later the equivalent of the following is done:

open: tmp = kzalloc(PAGE_SIZE, GFP_KERNEL);
write: fill(pp);
      *(char **)pp = tmp;
release: kfree(tmp)
read: read(**pp) => crash

And no there is no refcounting. But that would be perhaps too much for that.
Really I think a callback registered on module_param() would be better, not only
for appropriate allocation but for event handling too.