Poking ieee80211_default_rc_algo causes kernel lockups

Poking ieee80211_default_rc_algo causes kernel lockups

[Sitsofe Wheeler]

Hi,

As mentioned on
http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/d4a6ce26360613fa/b362792a2debced6?#b362792a2debced6
, poking /sys/module/mac80211/parameters/ieee80211_default_rc_algo and
then trying to read values back is currently causing the kernel to go
funny. E.g.
echo blah > /sys/module/mac80211/parameters/ieee80211_default_rc_algo
cat /sys/module/mac80211/parameters/ieee80211_default_rc_algo

Causes 2.6.29rc5 on my EeePC 900 to lock up.

-- 
Sitsofe

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker]

On Sun, Feb 15, 2009 at 04:09:57PM +0000, Sitsofe Wheeler wrote:
> Hi,
> 
> As mentioned on
> http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/d4a6ce26360613fa/b362792a2debced6?#b362792a2debced6
> , poking /sys/module/mac80211/parameters/ieee80211_default_rc_algo and
> then trying to read values back is currently causing the kernel to go
> funny. E.g.
> echo blah > /sys/module/mac80211/parameters/ieee80211_default_rc_algo
> cat /sys/module/mac80211/parameters/ieee80211_default_rc_algo
> 
> Causes 2.6.29rc5 on my EeePC 900 to lock up.
> 
> -- 
> Sitsofe


Hi,

I'm currently building a 2.6.29-rc5 with CONFIG_MAC80211 enabled
to test it.

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.

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

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Sitsofe Wheeler]

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



      

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker]

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

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Sitsofe Wheeler]

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

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. However one wonders how the syfs framework was supposed to work in other cases. 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?



      

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker]

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.

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker]

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.

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker]

On Sun, Feb 15, 2009 at 04:09:57PM +0000, Sitsofe Wheeler wrote:
> Hi,
> 
> As mentioned on
> http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/d4a6ce26360613fa/b362792a2debced6?#b362792a2debced6
> , poking /sys/module/mac80211/parameters/ieee80211_default_rc_algo and
> then trying to read values back is currently causing the kernel to go
> funny. E.g.
> echo blah > /sys/module/mac80211/parameters/ieee80211_default_rc_algo
> cat /sys/module/mac80211/parameters/ieee80211_default_rc_algo
> 
> Causes 2.6.29rc5 on my EeePC 900 to lock up.
> 
> -- 
> Sitsofe
> 


On Sun, Feb 15, 2009 at 04:09:57PM +0000, Sitsofe Wheeler wrote:
> Hi,
> 
> As mentioned on
> http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/d4a6ce26360613fa/b362792a2debced6?#b362792a2debced6
> , poking /sys/module/mac80211/parameters/ieee80211_default_rc_algo and
> then trying to read values back is currently causing the kernel to go
> funny. E.g.
> echo blah > /sys/module/mac80211/parameters/ieee80211_default_rc_algo
> cat /sys/module/mac80211/parameters/ieee80211_default_rc_algo
> 
> Causes 2.6.29rc5 on my EeePC 900 to lock up.
> 

I think I've found something:

When you set a new value to a module parameter through
sysfs, here is what happens (traced with the function_graph_tracer,
I've zapped a lot of entries to keep it clear)


 1)               |  sys_open() {
 1)               |            sysfs_open_file() {
 1)               |              kmem_cache_alloc() {
 1)   0.609 us    |                __might_sleep();
 1)   2.046 us    |              }
 1) + 58.976 us   |            }
 1) ! 471.823 us  |  }


The file is opened here, and sysfs will allocate a private buffer for this
file...


 1)               |  sys_write() {
 1)               |    vfs_write() {
 1)               |      sysfs_write_file() {
 1) + 26.446 us   |        get_zeroed_page();
 1)   0.789 us    |        sysfs_get_active_two();
 1)               |        module_attr_store() {
 1)               |          param_attr_store() {
 1)   5.625 us    |            param_set_charp();
 1)   7.279 us    |          }
 1)   8.919 us    |        }
 1)   4.113 us    |      }
 1) + 62.502 us   |    }
 1) + 64.961 us   |  }

Then we go to write the file, the buffer we allocated is used to store what
the user gave us as the new value of the module parameter.

This filled buffer is relayed to the module parameter and the address of the buffer
is directly assigned to the pointer of the module parameter:

int param_set_charp(const char *val, struct kernel_param *kp)
{
	[...]
	*(char **)kp->arg = (char *)val;
	return 0;
}

At this stage, all is good.

But just after we close the file:


 1)               |            sysfs_release() {
 1)               |              kfree() {
 1)   1.105 us    |                __phys_addr();
 1)   2.842 us    |              }
 1)               |              free_pages() {
 1)   0.609 us    |                __phys_addr();
 1)               |                __free_pages() {
 1)               |                  free_hot_page() {
 1)               |                    free_hot_cold_page() {
 1)   1.113 us    |                      get_pageblock_flags_group();
 1)   3.061 us    |                    }
 1)   4.466 us    |                  }
 1)   5.744 us    |                }
 1)   8.166 us    |              }
 1)               |              kfree() {
 1)   0.601 us    |                __phys_addr();
 1)   2.052 us    |              }
 1) + 18.686 us   |            }

The buffer we just assigned to the module parameter is freed. So we can't reuse it later,
but that's what is done later when we read it, since this bad address is dereferenced:

int param_get_charp(char *buffer, struct kernel_param *kp)
{
	return sprintf(buffer, "%s", *((char **)kp->arg));
}

So the problem seems not related to mac80211 but actually to module parameter management.
I'm not sure what is the right fix.

I guess we should use the following field in struct kernel_param:
struct kparam_string *str;

But this implies that string module params must not be simply pointers but static arrays.
Although I find it a bit insane to modify a string parameter at runtime.

The string is parsed by a module on load but not after, so a callback given by the module
seems to me more sane...

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Rusty Russell]

On Monday 16 February 2009 06:05:23 Frederic Weisbecker wrote:
> So the problem seems not related to mac80211 but actually to module parameter management.
> I'm not sure what is the right fix.

Good spotting.  And in fact this bug has been here for quite a while...

The "simple" fix is to kstrdup, but that leaks.

How's this?

param: fix charp parameters set via sysfs (eg. ieee80211_default_rc_algo)

The module_param type "charp" simply sets a char * pointer in the
module to the parameter in the commandline string: this is why we keep
the (mangled) module command line around.  But when set via sysfs (as
about 11 charp parameters can be) this memory is freed on the way
out of the write().

So we kstrdup instead: this is fine, but it means we have to note when
we've used it so we can reliably kfree the parameter when it's next
overwritten, and also on module unload.

Reported-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>

diff --git a/include/linux/module.h b/include/linux/module.h
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -247,6 +247,10 @@ struct module
 	const struct kernel_symbol *syms;
 	const unsigned long *crcs;
 	unsigned int num_syms;
+
+	/* Kernel parameters. */
+	struct kernel_param *kp;
+	unsigned int num_kp;
 
 	/* GPL-only exported symbols. */
 	unsigned int num_gpl_syms;
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -138,6 +138,9 @@ extern int parse_args(const char *name,
 		      unsigned num,
 		      int (*unknown)(char *param, char *val));
 
+/* Called by module remove. */
+extern void destroy_params(const struct kernel_param *params, unsigned num);
+
 /* All the helper functions */
 /* The macros to do compile-time type checking stolen from Jakub
    Jelinek, who IIRC came up with this idea for the 2.4 module init code. */
diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1457,6 +1457,9 @@ static void free_module(struct module *m
 	/* Module unload stuff */
 	module_unload_free(mod);
 
+	/* Free any allocated parameters. */
+	destroy_params(mod->kp, mod->num_kp);
+
 	/* release any pointers to mcount in this module */
 	ftrace_release(mod->module_core, mod->core_size);
 
@@ -1870,8 +1873,7 @@ static noinline struct module *load_modu
 	unsigned int symindex = 0;
 	unsigned int strindex = 0;
 	unsigned int modindex, versindex, infoindex, pcpuindex;
-	unsigned int num_kp, num_mcount;
-	struct kernel_param *kp;
+	unsigned int num_mcount;
 	struct module *mod;
 	long err = 0;
 	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
@@ -2116,8 +2118,8 @@ static noinline struct module *load_modu
 
 	/* Now we've got everything in the final locations, we can
 	 * find optional sections. */
-	kp = section_objs(hdr, sechdrs, secstrings, "__param", sizeof(*kp),
-			  &num_kp);
+	mod->kp = section_objs(hdr, sechdrs, secstrings, "__param",
+			       sizeof(*mod->kp), &mod->num_kp);
 	mod->syms = section_objs(hdr, sechdrs, secstrings, "__ksymtab",
 				 sizeof(*mod->syms), &mod->num_syms);
 	mod->crcs = section_addr(hdr, sechdrs, secstrings, "__kcrctab");
@@ -2262,11 +2264,11 @@ static noinline struct module *load_modu
 	 */
 	list_add_rcu(&mod->list, &modules);
 
-	err = parse_args(mod->name, mod->args, kp, num_kp, NULL);
+	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
 	if (err < 0)
 		goto unlink;
 
-	err = mod_sysfs_setup(mod, kp, num_kp);
+	err = mod_sysfs_setup(mod, mod->kp, mod->num_kp);
 	if (err < 0)
 		goto unlink;
 	add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
diff --git a/kernel/params.c b/kernel/params.c
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -23,6 +23,9 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+
+/* We abuse the high bits of "perm" to record whether we kmalloc'ed. */
+#define KPARAM_KMALLOCED	0x80000000
 
 #if 0
 #define DEBUGP printk
@@ -217,7 +220,13 @@ int param_set_charp(const char *val, str
 		return -ENOSPC;
 	}
 
-	*(char **)kp->arg = (char *)val;
+	if (kp->perm & KPARAM_KMALLOCED)
+		kfree(*(char **)kp->arg);
+
+	kp->perm |= KPARAM_KMALLOCED;
+	*(char **)kp->arg = kstrdup(val, GFP_KERNEL);
+	if (!kp->arg)
+		return -ENOMEM;
 	return 0;
 }
 
@@ -571,6 +580,15 @@ void module_param_sysfs_remove(struct mo
 }
 #endif
 
+void destroy_params(const struct kernel_param *params, unsigned num)
+{
+	unsigned int i;
+
+	for (i = 0; i < num; i++)
+		if (params[i].perm & KPARAM_KMALLOCED)
+			kfree(*(char **)params[i].arg);
+}
+
 static void __init kernel_add_sysfs_param(const char *name,
 					  struct kernel_param *kparam,
 					  unsigned int name_skip)

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Frederic Weisbecker]

On Mon, Feb 16, 2009 at 08:40:43PM +1030, Rusty Russell wrote:
> On Monday 16 February 2009 06:05:23 Frederic Weisbecker wrote:
> > So the problem seems not related to mac80211 but actually to module parameter management.
> > I'm not sure what is the right fix.
> 
> Good spotting.  And in fact this bug has been here for quite a while...
> 
> The "simple" fix is to kstrdup, but that leaks.
> 
> How's this?


Simple solution, and fixes the problem :-)
I've tested the string parameter change and the mac80211 module unloading as well.
No obvious problem.

Tested-by: Frederic Weisbecker <fweisbec@gmail.com>

And also, Reported-by: Sitsofe Wheeler <sitsofe@yahoo.com>

Just one last little thing, it would be good to strip the ending newline
while using echo without -n
ie:

root@nowhere:/sys/module/mac80211/parameters# echo heh_look_at_my_newline > ieee80211_default_rc_algo 
root@nowhere:/sys/module/mac80211/parameters# cat ieee80211_default_rc_algo 
heh_look_at_my_newline

root@nowhere:/sys/module/mac80211/parameters# echo -n heh_look_at_hum_nothing > ieee80211_default_rc_algo 
root@nowhere:/sys/module/mac80211/parameters# cat ieee80211_default_rc_algo 
heh_look_at_hum_nothing
root@nowhere:/sys/module/mac80211/parameters#

Because I guess modules don't expect this newline on their strcmp.

Thanks :-)

 
> param: fix charp parameters set via sysfs (eg. ieee80211_default_rc_algo)
> 
> The module_param type "charp" simply sets a char * pointer in the
> module to the parameter in the commandline string: this is why we keep
> the (mangled) module command line around.  But when set via sysfs (as
> about 11 charp parameters can be) this memory is freed on the way
> out of the write().
> 
> So we kstrdup instead: this is fine, but it means we have to note when
> we've used it so we can reliably kfree the parameter when it's next
> overwritten, and also on module unload.
> 
> Reported-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -247,6 +247,10 @@ struct module
>  	const struct kernel_symbol *syms;
>  	const unsigned long *crcs;
>  	unsigned int num_syms;
> +
> +	/* Kernel parameters. */
> +	struct kernel_param *kp;
> +	unsigned int num_kp;
>  
>  	/* GPL-only exported symbols. */
>  	unsigned int num_gpl_syms;
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -138,6 +138,9 @@ extern int parse_args(const char *name,
>  		      unsigned num,
>  		      int (*unknown)(char *param, char *val));
>  
> +/* Called by module remove. */
> +extern void destroy_params(const struct kernel_param *params, unsigned num);
> +
>  /* All the helper functions */
>  /* The macros to do compile-time type checking stolen from Jakub
>     Jelinek, who IIRC came up with this idea for the 2.4 module init code. */
> diff --git a/kernel/module.c b/kernel/module.c
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1457,6 +1457,9 @@ static void free_module(struct module *m
>  	/* Module unload stuff */
>  	module_unload_free(mod);
>  
> +	/* Free any allocated parameters. */
> +	destroy_params(mod->kp, mod->num_kp);
> +
>  	/* release any pointers to mcount in this module */
>  	ftrace_release(mod->module_core, mod->core_size);
>  
> @@ -1870,8 +1873,7 @@ static noinline struct module *load_modu
>  	unsigned int symindex = 0;
>  	unsigned int strindex = 0;
>  	unsigned int modindex, versindex, infoindex, pcpuindex;
> -	unsigned int num_kp, num_mcount;
> -	struct kernel_param *kp;
> +	unsigned int num_mcount;
>  	struct module *mod;
>  	long err = 0;
>  	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
> @@ -2116,8 +2118,8 @@ static noinline struct module *load_modu
>  
>  	/* Now we've got everything in the final locations, we can
>  	 * find optional sections. */
> -	kp = section_objs(hdr, sechdrs, secstrings, "__param", sizeof(*kp),
> -			  &num_kp);
> +	mod->kp = section_objs(hdr, sechdrs, secstrings, "__param",
> +			       sizeof(*mod->kp), &mod->num_kp);
>  	mod->syms = section_objs(hdr, sechdrs, secstrings, "__ksymtab",
>  				 sizeof(*mod->syms), &mod->num_syms);
>  	mod->crcs = section_addr(hdr, sechdrs, secstrings, "__kcrctab");
> @@ -2262,11 +2264,11 @@ static noinline struct module *load_modu
>  	 */
>  	list_add_rcu(&mod->list, &modules);
>  
> -	err = parse_args(mod->name, mod->args, kp, num_kp, NULL);
> +	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
>  	if (err < 0)
>  		goto unlink;
>  
> -	err = mod_sysfs_setup(mod, kp, num_kp);
> +	err = mod_sysfs_setup(mod, mod->kp, mod->num_kp);
>  	if (err < 0)
>  		goto unlink;
>  	add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
> diff --git a/kernel/params.c b/kernel/params.c
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -23,6 +23,9 @@
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +
> +/* We abuse the high bits of "perm" to record whether we kmalloc'ed. */
> +#define KPARAM_KMALLOCED	0x80000000
>  
>  #if 0
>  #define DEBUGP printk
> @@ -217,7 +220,13 @@ int param_set_charp(const char *val, str
>  		return -ENOSPC;
>  	}
>  
> -	*(char **)kp->arg = (char *)val;
> +	if (kp->perm & KPARAM_KMALLOCED)
> +		kfree(*(char **)kp->arg);
> +
> +	kp->perm |= KPARAM_KMALLOCED;
> +	*(char **)kp->arg = kstrdup(val, GFP_KERNEL);
> +	if (!kp->arg)
> +		return -ENOMEM;
>  	return 0;
>  }
>  
> @@ -571,6 +580,15 @@ void module_param_sysfs_remove(struct mo
>  }
>  #endif
>  
> +void destroy_params(const struct kernel_param *params, unsigned num)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < num; i++)
> +		if (params[i].perm & KPARAM_KMALLOCED)
> +			kfree(*(char **)params[i].arg);
> +}
> +
>  static void __init kernel_add_sysfs_param(const char *name,
>  					  struct kernel_param *kparam,
>  					  unsigned int name_skip)

Re: Poking ieee80211_default_rc_algo causes kernel lockups

[Rusty Russell]

On Tuesday 17 February 2009 04:29:08 Frederic Weisbecker wrote:
> Tested-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> And also, Reported-by: Sitsofe Wheeler <sitsofe@yahoo.com>

Thanks, done, and I'll send to Linus and -stable immediately.

> Just one last little thing, it would be good to strip the ending newline
> while using echo without -n

Agreed, but fixing that is much less urgent.  (It also effects "string"
module parameters).

Thanks!
Rusty.

What made this bug report better? (was Re: Poking ieee80211_default_rc_algo causes kernel lockups)

[Sitsofe Wheeler]

On Tue, Feb 17, 2009 at 10:07:27AM +1030, Rusty Russell wrote:
> 
> Thanks, done, and I'll send to Linus and -stable immediately.

I'm curious - only two days after I reposted this the problem [1] it
appears to have been resolved. When I posted it the first time [2] I
only received one reply (privately) and that was from Greg wondering why
I had only sent the email to him (which was an unfortunate problem and
down to how I was trying to send LKML mails at the time).

What happened this time that made things succesful? Was the original
mail too big? Did it not make it to the list?

[1] http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/2a816ca521d72494/bd607c7e143def6f?#bd607c7e143def6f
[2] http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/d4a6ce26360613fa/b362792a2debced6?#b362792a2debced6

-- 
Sitsofe | http://sucs.org/~sits/

Re: What made this bug report better? (was Re: Poking ieee80211_default_rc_algo causes kernel lockups)

[Ingo Molnar]

* Sitsofe Wheeler <sitsofe@yahoo.com> wrote:

> On Tue, Feb 17, 2009 at 10:07:27AM +1030, Rusty Russell wrote:
> > 
> > Thanks, done, and I'll send to Linus and -stable immediately.
> 
> I'm curious - only two days after I reposted this the problem 
> [1] it appears to have been resolved. When I posted it the 
> first time [2] I only received one reply (privately) and that 
> was from Greg wondering why I had only sent the email to him 
> (which was an unfortunate problem and down to how I was trying 
> to send LKML mails at the time).
> 
> What happened this time that made things succesful? Was the 
> original mail too big? Did it not make it to the list?
> 
> [1] http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/2a816ca521d72494/bd607c7e143def6f?#bd607c7e143def6f
> [2] http://groups.google.com/group/fa.linux.kernel/browse_frm/thread/d4a6ce26360613fa/b362792a2debced6?#b362792a2debced6

It does not seem to have made it to everyone - it's not in my 
lkml folder for example. Sometimes vger eats emails. Filing a 
bugzilla for reproducible bugs makes sense - especially if you 
see that no action has been taken by anyone.

But note that even with expanded Cc:s and a properly working 
list it needed Frederic's excellent function-graph trace and 
analysis.

A crash in the wireless code rarely gets tracked back to a core 
kernel module support bug, and it would have taken quite some 
time for someone to make that connection.

	Ingo

Re: What made this bug report better? (was Re: Poking ieee80211_default_rc_algo causes kernel lockups)

[Rusty Russell]

On Friday 20 February 2009 20:57:05 Sitsofe Wheeler wrote:
> What happened this time that made things succesful? Was the original
> mail too big? Did it not make it to the list?

Frederic was the difference; he pulled me in by CC'ing me, once he figured
out it was my bug.

Cheers,
Rusty.

Re: What made this bug report better?

[Sitsofe Wheeler]

On Tue, Feb 24, 2009 at 01:43:22PM +1030, Rusty Russell wrote:
> On Friday 20 February 2009 20:57:05 Sitsofe Wheeler wrote:
> > What happened this time that made things succesful? Was the original
> > mail too big? Did it not make it to the list?
> 
> Frederic was the difference; he pulled me in by CC'ing me, once he figured
> out it was my bug.

Rusty, Ingo: Thanks for letting me know!

Frederic: What information are you looking for in bug reports? What sort
of areas interest you?

-- 
Sitsofe | http://sucs.org/~sits/

Re: What made this bug report better?

[Frederic Weisbecker]

On Tue, Feb 24, 2009 at 08:16:12AM +0000, Sitsofe Wheeler wrote:
> On Tue, Feb 24, 2009 at 01:43:22PM +1030, Rusty Russell wrote:
> > On Friday 20 February 2009 20:57:05 Sitsofe Wheeler wrote:
> > > What happened this time that made things succesful? Was the original
> > > mail too big? Did it not make it to the list?
> > 
> > Frederic was the difference; he pulled me in by CC'ing me, once he figured
> > out it was my bug.
> 
> Rusty, Ingo: Thanks for letting me know!
> 
> Frederic: What information are you looking for in bug reports? 


Like everyone, the most possible matching traces, locking states, and other
states (irq, atomic, ...), well it depends on the particular case.

The best thing is to provide a way to reproduce it, that's what you did, so
that we can use specific tracers for specific issues.


> What sort of areas interest you?

You mean, what I'm interested in the kernel?
All the kernel :-)
Though I have a preference for the kernel core, and improving tracing
because I'm lazy and I like when there are tools which do most of the things
for me.
 
> -- 
> Sitsofe | http://sucs.org/~sits/