Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker <fweisbec@gmail.com>]

On Mon, Feb 16, 2009 at 02:40:15AM +0100, Frederic Weisbecker wrote:
> 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.


Well, actually all of these callsites seem to handle the case when their param value change.
So only the module param core should be fixed...

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