[PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Rusty Russell]

(Thanks to Takashi-san, who found the oops and kept reading the code to spot
 the others.  A more complete fix is pending, but this works for 2.6.32 and
 -stable: see commit message and FIXME in code.)

The following changes since commit 964fe080d94db82a3268443e9b9ece4c60246414:
  Linus Torvalds (1):
        Merge git://git.kernel.org/.../rusty/linux-2.6-for-linus

are available in the git repository at:

  ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-param-fixes.git master

Rusty Russell (3):
      param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
      param: fix NULL comparison on oom
      param: fix setting arrays of bool

 include/linux/moduleparam.h |    1 -
 kernel/params.c             |   17 ++++++-----------
 2 files changed, 6 insertions(+), 12 deletions(-)

commit 65afac7d80ab3bc9f81e75eafb71eeb92a3ebdef
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Thu Oct 29 08:56:16 2009 -0600

    param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
    
    e180a6b7759a "param: fix charp parameters set via sysfs" fixed the case
    where charp parameters written via sysfs were freed, leaving drivers
    accessing random memory.
    
    Unfortunately, storing a flag in the kparam struct was a bad idea: it's
    rodata so setting it causes an oops on some archs.  But that's not all:
    
    1) module_param_array() on charp doesn't work reliably, since we use an
       uninitialized temporary struct kernel_param.
    2) there's a fundamental race if a module uses this parameter and then
       it's changed: they will still access the old, freed, memory.
    
    The simplest fix (ie. for 2.6.32) is to never free the memory.  This
    prevents all these problems, at cost of a memory leak.  In practice, there
    are only 18 places where a charp is writable via sysfs, and all are
    root-only writable.
    
    Reported-by: Takashi Iwai <tiwai@suse.de>
    Cc: Sitsofe Wheeler <sitsofe@yahoo.com>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Christof Schmitt <christof.schmitt@de.ibm.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
    Cc: stable@kernel.org

 include/linux/moduleparam.h |    1 -
 kernel/params.c             |   10 +---------
 2 files changed, 1 insertions(+), 10 deletions(-)

commit d553ad864e3b3dde3f1038d491e207021b2d6293
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Thu Oct 29 08:56:17 2009 -0600

    param: fix NULL comparison on oom
    
    kp->arg is always true: it's the contents of that pointer we care about.
    
    Reported-by: Takashi Iwai <tiwai@suse.de>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
    Cc: stable@kernel.org

 kernel/params.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

commit 3c7d76e371ac1a3802ae1673f5c63554af59325c
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Thu Oct 29 08:56:19 2009 -0600

    param: fix setting arrays of bool
    
    We create a dummy struct kernel_param on the stack for parsing each
    array element, but we didn't initialize the flags word.  This matters
    for arrays of type "bool", where the flag indicates if it really is
    an array of bools or unsigned int (old-style).
    
    Reported-by: Takashi Iwai <tiwai@suse.de>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
    Cc: stable@kernel.org

 kernel/params.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 6547c3c..82a9124 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -37,7 +37,6 @@ typedef int (*param_set_fn)(const char *val, struct kernel_param *kp);
 typedef int (*param_get_fn)(char *buffer, struct kernel_param *kp);
 
 /* Flag bits for kernel_param.flags */
-#define KPARAM_KMALLOCED	1
 #define KPARAM_ISBOOL		2
 
 struct kernel_param {
diff --git a/kernel/params.c b/kernel/params.c
index 9da58ea..d656c27 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -218,15 +218,11 @@ int param_set_charp(const char *val, struct kernel_param *kp)
 		return -ENOSPC;
 	}
 
-	if (kp->flags & KPARAM_KMALLOCED)
-		kfree(*(char **)kp->arg);
-
 	/* This is a hack.  We can't need to strdup in early boot, and we
 	 * don't need to; this mangled commandline is preserved. */
 	if (slab_is_available()) {
-		kp->flags |= KPARAM_KMALLOCED;
 		*(char **)kp->arg = kstrdup(val, GFP_KERNEL);
-		if (!kp->arg)
+		if (!*(char **)kp->arg)
 			return -ENOMEM;
 	} else
 		*(const char **)kp->arg = val;
@@ -304,6 +300,7 @@ static int param_array(const char *name,
 		       unsigned int min, unsigned int max,
 		       void *elem, int elemsize,
 		       int (*set)(const char *, struct kernel_param *kp),
+		       u16 flags,
 		       unsigned int *num)
 {
 	int ret;
@@ -313,6 +310,7 @@ static int param_array(const char *name,
 	/* Get the name right for errors. */
 	kp.name = name;
 	kp.arg = elem;
+	kp.flags = flags;
 
 	/* No equals sign? */
 	if (!val) {
@@ -358,7 +356,8 @@ int param_array_set(const char *val, struct kernel_param *kp)
 	unsigned int temp_num;
 
 	return param_array(kp->name, val, 1, arr->max, arr->elem,
-			   arr->elemsize, arr->set, arr->num ?: &temp_num);
+			   arr->elemsize, arr->set, kp->flags,
+			   arr->num ?: &temp_num);
 }
 
 int param_array_get(char *buffer, struct kernel_param *kp)
@@ -605,11 +604,7 @@ void module_param_sysfs_remove(struct module *mod)
 
 void destroy_params(const struct kernel_param *params, unsigned num)
 {
-	unsigned int i;
-
-	for (i = 0; i < num; i++)
-		if (params[i].flags & KPARAM_KMALLOCED)
-			kfree(*(char **)params[i].arg);
+	/* FIXME: This should free kmalloced charp parameters.  It doesn't. */
 }
 
 static void __init kernel_add_sysfs_param(const char *name,

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Artem Bityutskiy]

On Thu, 2009-10-29 at 09:02 +1030, Rusty Russell wrote:
> (Thanks to Takashi-san, who found the oops and kept reading the code to spot
>  the others.  A more complete fix is pending, but this works for 2.6.32 and
>  -stable: see commit message and FIXME in code.)
> 
> The following changes since commit 964fe080d94db82a3268443e9b9ece4c60246414:
>   Linus Torvalds (1):
>         Merge git://git.kernel.org/.../rusty/linux-2.6-for-linus
> 
> are available in the git repository at:
> 
>   ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-param-fixes.git master
> 
> Rusty Russell (3):
>       param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
>       param: fix NULL comparison on oom
>       param: fix setting arrays of bool
> 
>  include/linux/moduleparam.h |    1 -
>  kernel/params.c             |   17 ++++++-----------
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> commit 65afac7d80ab3bc9f81e75eafb71eeb92a3ebdef
> Author: Rusty Russell <rusty@rustcorp.com.au>
> Date:   Thu Oct 29 08:56:16 2009 -0600
> 
>     param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
>     
>     e180a6b7759a "param: fix charp parameters set via sysfs" fixed the case
>     where charp parameters written via sysfs were freed, leaving drivers
>     accessing random memory.
>     
>     Unfortunately, storing a flag in the kparam struct was a bad idea: it's
>     rodata so setting it causes an oops on some archs.  But that's not all:
>     
>     1) module_param_array() on charp doesn't work reliably, since we use an
>        uninitialized temporary struct kernel_param.
>     2) there's a fundamental race if a module uses this parameter and then
>        it's changed: they will still access the old, freed, memory.
>     
>     The simplest fix (ie. for 2.6.32) is to never free the memory.  This
>     prevents all these problems, at cost of a memory leak.  In practice, there
>     are only 18 places where a charp is writable via sysfs, and all are
>     root-only writable.

Hmm, is it really only about changing the parameters via sysfs? We see
the following kmemleak complaints in 2.6.32 (I think it will be the same
in the latest kernel, but I did not check):

kmemleak: unreferenced object 0xdeff3c80 (size 64):
kmemleak:   comm "modprobe", pid 788, jiffies 4294933427
kmemleak:   backtrace:
kmemleak:     [<c00e59b8>] __save_stack_trace+0x34/0x40
kmemleak:     [<c00e5ad0>] create_object+0x10c/0x208
kmemleak:     [<c03ae0ec>] kmemleak_alloc+0x40/0x84
kmemleak:     [<c00dca74>] __kmalloc_track_caller+0x140/0x154
kmemleak:     [<c00c47ac>] kstrdup+0x38/0x54
kmemleak:     [<c0081854>] param_set_charp+0x68/0x94
kmemleak:     [<c0081108>] parse_args+0x18c/0x280
kmemleak:     [<c009fc74>] load_module+0x11e8/0x1644
kmemleak:     [<c00a0130>] sys_init_module+0x60/0x1f4
kmemleak:     [<c003c040>] ret_fast_syscall+0x0/0x38

So we are leaking on every insmod/rmmod, AFAICS, not just in the sysfs
case.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Artem Bityutskiy]

On Tue, 2010-04-27 at 13:31 +0300, Artem Bityutskiy wrote:
> On Thu, 2009-10-29 at 09:02 +1030, Rusty Russell wrote:
> > (Thanks to Takashi-san, who found the oops and kept reading the code to spot
> >  the others.  A more complete fix is pending, but this works for 2.6.32 and
> >  -stable: see commit message and FIXME in code.)
> > 
> > The following changes since commit 964fe080d94db82a3268443e9b9ece4c60246414:
> >   Linus Torvalds (1):
> >         Merge git://git.kernel.org/.../rusty/linux-2.6-for-linus
> > 
> > are available in the git repository at:
> > 
> >   ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-param-fixes.git master
> > 
> > Rusty Russell (3):
> >       param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
> >       param: fix NULL comparison on oom
> >       param: fix setting arrays of bool
> > 
> >  include/linux/moduleparam.h |    1 -
> >  kernel/params.c             |   17 ++++++-----------
> >  2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > commit 65afac7d80ab3bc9f81e75eafb71eeb92a3ebdef
> > Author: Rusty Russell <rusty@rustcorp.com.au>
> > Date:   Thu Oct 29 08:56:16 2009 -0600
> > 
> >     param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
> >     
> >     e180a6b7759a "param: fix charp parameters set via sysfs" fixed the case
> >     where charp parameters written via sysfs were freed, leaving drivers
> >     accessing random memory.
> >     
> >     Unfortunately, storing a flag in the kparam struct was a bad idea: it's
> >     rodata so setting it causes an oops on some archs.  But that's not all:
> >     
> >     1) module_param_array() on charp doesn't work reliably, since we use an
> >        uninitialized temporary struct kernel_param.
> >     2) there's a fundamental race if a module uses this parameter and then
> >        it's changed: they will still access the old, freed, memory.
> >     
> >     The simplest fix (ie. for 2.6.32) is to never free the memory.  This
> >     prevents all these problems, at cost of a memory leak.  In practice, there
> >     are only 18 places where a charp is writable via sysfs, and all are
> >     root-only writable.
> 
> Hmm, is it really only about changing the parameters via sysfs? We see
> the following kmemleak complaints in 2.6.32 (I think it will be the same
> in the latest kernel, but I did not check):
> 
> kmemleak: unreferenced object 0xdeff3c80 (size 64):
> kmemleak:   comm "modprobe", pid 788, jiffies 4294933427
> kmemleak:   backtrace:
> kmemleak:     [<c00e59b8>] __save_stack_trace+0x34/0x40
> kmemleak:     [<c00e5ad0>] create_object+0x10c/0x208
> kmemleak:     [<c03ae0ec>] kmemleak_alloc+0x40/0x84
> kmemleak:     [<c00dca74>] __kmalloc_track_caller+0x140/0x154
> kmemleak:     [<c00c47ac>] kstrdup+0x38/0x54
> kmemleak:     [<c0081854>] param_set_charp+0x68/0x94
> kmemleak:     [<c0081108>] parse_args+0x18c/0x280
> kmemleak:     [<c009fc74>] load_module+0x11e8/0x1644
> kmemleak:     [<c00a0130>] sys_init_module+0x60/0x1f4
> kmemleak:     [<c003c040>] ret_fast_syscall+0x0/0x38
> 
> So we are leaking on every insmod/rmmod, AFAICS, not just in the sysfs
> case.

Rusty, correct me if I'm wrong, but it looks like the above memleak was
introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added
the kstrdup(). So you kinda fixed the sysfs case (it still memleaks
though), but at the cost of additional insmod/rmmod leak, right?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Rusty Russell]

On Tue, 27 Apr 2010 08:23:24 pm Artem Bityutskiy wrote:
> On Tue, 2010-04-27 at 13:31 +0300, Artem Bityutskiy wrote:
> > On Thu, 2009-10-29 at 09:02 +1030, Rusty Russell wrote:
> > > (Thanks to Takashi-san, who found the oops and kept reading the code to spot
> > >  the others.  A more complete fix is pending, but this works for 2.6.32 and
> > >  -stable: see commit message and FIXME in code.)
> > > 
> > > The following changes since commit 964fe080d94db82a3268443e9b9ece4c60246414:
> > >   Linus Torvalds (1):
> > >         Merge git://git.kernel.org/.../rusty/linux-2.6-for-linus
> > > 
> > > are available in the git repository at:
> > > 
> > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-param-fixes.git master
> > > 
> > > Rusty Russell (3):
> > >       param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
> > >       param: fix NULL comparison on oom
> > >       param: fix setting arrays of bool
> > > 
> > >  include/linux/moduleparam.h |    1 -
> > >  kernel/params.c             |   17 ++++++-----------
> > >  2 files changed, 6 insertions(+), 12 deletions(-)
> > > 
> > > commit 65afac7d80ab3bc9f81e75eafb71eeb92a3ebdef
> > > Author: Rusty Russell <rusty@rustcorp.com.au>
> > > Date:   Thu Oct 29 08:56:16 2009 -0600
> > > 
> > >     param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
> > >     
> > >     e180a6b7759a "param: fix charp parameters set via sysfs" fixed the case
> > >     where charp parameters written via sysfs were freed, leaving drivers
> > >     accessing random memory.
> > >     
> > >     Unfortunately, storing a flag in the kparam struct was a bad idea: it's
> > >     rodata so setting it causes an oops on some archs.  But that's not all:
> > >     
> > >     1) module_param_array() on charp doesn't work reliably, since we use an
> > >        uninitialized temporary struct kernel_param.
> > >     2) there's a fundamental race if a module uses this parameter and then
> > >        it's changed: they will still access the old, freed, memory.
> > >     
> > >     The simplest fix (ie. for 2.6.32) is to never free the memory.  This
> > >     prevents all these problems, at cost of a memory leak.  In practice, there
> > >     are only 18 places where a charp is writable via sysfs, and all are
> > >     root-only writable.
> > 
> > Hmm, is it really only about changing the parameters via sysfs? We see
> > the following kmemleak complaints in 2.6.32 (I think it will be the same
> > in the latest kernel, but I did not check):
> > 
> > kmemleak: unreferenced object 0xdeff3c80 (size 64):
> > kmemleak:   comm "modprobe", pid 788, jiffies 4294933427
> > kmemleak:   backtrace:
> > kmemleak:     [<c00e59b8>] __save_stack_trace+0x34/0x40
> > kmemleak:     [<c00e5ad0>] create_object+0x10c/0x208
> > kmemleak:     [<c03ae0ec>] kmemleak_alloc+0x40/0x84
> > kmemleak:     [<c00dca74>] __kmalloc_track_caller+0x140/0x154
> > kmemleak:     [<c00c47ac>] kstrdup+0x38/0x54
> > kmemleak:     [<c0081854>] param_set_charp+0x68/0x94
> > kmemleak:     [<c0081108>] parse_args+0x18c/0x280
> > kmemleak:     [<c009fc74>] load_module+0x11e8/0x1644
> > kmemleak:     [<c00a0130>] sys_init_module+0x60/0x1f4
> > kmemleak:     [<c003c040>] ret_fast_syscall+0x0/0x38
> > 
> > So we are leaking on every insmod/rmmod, AFAICS, not just in the sysfs
> > case.
> 
> Rusty, correct me if I'm wrong, but it looks like the above memleak was
> introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added
> the kstrdup(). So you kinda fixed the sysfs case (it still memleaks
> though), but at the cost of additional insmod/rmmod leak, right?

Yep!

Cheers,
Rusty.

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Artem Bityutskiy]

On Tue, 2010-05-04 at 11:53 +0930, Rusty Russell wrote:
> On Tue, 27 Apr 2010 08:23:24 pm Artem Bityutskiy wrote:
> > On Tue, 2010-04-27 at 13:31 +0300, Artem Bityutskiy wrote:
> > > On Thu, 2009-10-29 at 09:02 +1030, Rusty Russell wrote:
> > > > (Thanks to Takashi-san, who found the oops and kept reading the code to spot
> > > >  the others.  A more complete fix is pending, but this works for 2.6.32 and
> > > >  -stable: see commit message and FIXME in code.)
> > > > 
> > > > The following changes since commit 964fe080d94db82a3268443e9b9ece4c60246414:
> > > >   Linus Torvalds (1):
> > > >         Merge git://git.kernel.org/.../rusty/linux-2.6-for-linus
> > > > 
> > > > are available in the git repository at:
> > > > 
> > > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-param-fixes.git master
> > > > 
> > > > Rusty Russell (3):
> > > >       param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
> > > >       param: fix NULL comparison on oom
> > > >       param: fix setting arrays of bool
> > > > 
> > > >  include/linux/moduleparam.h |    1 -
> > > >  kernel/params.c             |   17 ++++++-----------
> > > >  2 files changed, 6 insertions(+), 12 deletions(-)
> > > > 
> > > > commit 65afac7d80ab3bc9f81e75eafb71eeb92a3ebdef
> > > > Author: Rusty Russell <rusty@rustcorp.com.au>
> > > > Date:   Thu Oct 29 08:56:16 2009 -0600
> > > > 
> > > >     param: fix lots of bugs with writing charp params from sysfs, by leaking mem.
> > > >     
> > > >     e180a6b7759a "param: fix charp parameters set via sysfs" fixed the case
> > > >     where charp parameters written via sysfs were freed, leaving drivers
> > > >     accessing random memory.
> > > >     
> > > >     Unfortunately, storing a flag in the kparam struct was a bad idea: it's
> > > >     rodata so setting it causes an oops on some archs.  But that's not all:
> > > >     
> > > >     1) module_param_array() on charp doesn't work reliably, since we use an
> > > >        uninitialized temporary struct kernel_param.
> > > >     2) there's a fundamental race if a module uses this parameter and then
> > > >        it's changed: they will still access the old, freed, memory.
> > > >     
> > > >     The simplest fix (ie. for 2.6.32) is to never free the memory.  This
> > > >     prevents all these problems, at cost of a memory leak.  In practice, there
> > > >     are only 18 places where a charp is writable via sysfs, and all are
> > > >     root-only writable.
> > > 
> > > Hmm, is it really only about changing the parameters via sysfs? We see
> > > the following kmemleak complaints in 2.6.32 (I think it will be the same
> > > in the latest kernel, but I did not check):
> > > 
> > > kmemleak: unreferenced object 0xdeff3c80 (size 64):
> > > kmemleak:   comm "modprobe", pid 788, jiffies 4294933427
> > > kmemleak:   backtrace:
> > > kmemleak:     [<c00e59b8>] __save_stack_trace+0x34/0x40
> > > kmemleak:     [<c00e5ad0>] create_object+0x10c/0x208
> > > kmemleak:     [<c03ae0ec>] kmemleak_alloc+0x40/0x84
> > > kmemleak:     [<c00dca74>] __kmalloc_track_caller+0x140/0x154
> > > kmemleak:     [<c00c47ac>] kstrdup+0x38/0x54
> > > kmemleak:     [<c0081854>] param_set_charp+0x68/0x94
> > > kmemleak:     [<c0081108>] parse_args+0x18c/0x280
> > > kmemleak:     [<c009fc74>] load_module+0x11e8/0x1644
> > > kmemleak:     [<c00a0130>] sys_init_module+0x60/0x1f4
> > > kmemleak:     [<c003c040>] ret_fast_syscall+0x0/0x38
> > > 
> > > So we are leaking on every insmod/rmmod, AFAICS, not just in the sysfs
> > > case.
> > 
> > Rusty, correct me if I'm wrong, but it looks like the above memleak was
> > introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added
> > the kstrdup(). So you kinda fixed the sysfs case (it still memleaks
> > though), but at the cost of additional insmod/rmmod leak, right?
> 
> Yep!

Are you working/planning to work on fixing this regression?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Rusty Russell]

On Wed, 5 May 2010 03:37:19 am Artem Bityutskiy wrote:
> On Tue, 2010-05-04 at 11:53 +0930, Rusty Russell wrote:
> > On Tue, 27 Apr 2010 08:23:24 pm Artem Bityutskiy wrote:
> > > Rusty, correct me if I'm wrong, but it looks like the above memleak was
> > > introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added
> > > the kstrdup(). So you kinda fixed the sysfs case (it still memleaks
> > > though), but at the cost of additional insmod/rmmod leak, right?
> > 
> > Yep!
> 
> Are you working/planning to work on fixing this regression?

I'm still ambivalent on it; I have patches but it's a lot of churn for not
much gain.

To fix this, we need a way to lock parameters against changing by sysfs, and
we need to use it everywhere.  Past experience has demonstrated that this will
never be maintained.  

On the other hand, the leak is trivial.

Here's a git tree if you want to look further:

The following changes since commit 05ce7bfe547c9fa967d9cab6c37867a9cb6fb3fa:
  Linus Torvalds (1):
        Merge branch 'for_linus' of git://git.kernel.org/.../jack/linux-fs-2.6

are available in the git repository at:

  git://git.kernel.org/rusty/linux-2.6-param-fixes.git master

Rusty Russell (17):
      param: move the EXPORT_SYMBOL to after the definitions.
      params: don't hand NULL values to param.set callbacks.
      param: use ops in struct kernel_param, rather than get and set fns directly
      param: silence .init.text references from param ops
      param: add a free hook to kernel_param_ops.
      param: use free hook for charp
      param: make param sections const.
      param: locking for kernel parameters
      param: add kerneldoc
      param: remove unnecessary writable charp
      param: simple locking for sysfs-writable charp parameters
      param: lock myri10ge_fw_name against sysfs changes.
      param: lock if_sdio's lbs_helper_name and lbs_fw_name against sysfs changes.
      param: update drivers/char/ipmi/ipmi_watchdog.c to new scheme
      ide: use module_param_named rather than module_param_call
      param: use module_param in drivers/message/fusion/mptbase.c
      param: update drivers/acpi/debug.c to new scheme

Sachin Sant (1):
      Add param ops struct for hvc_iucv driver.

 arch/um/drivers/hostaudio_kern.c          |   10 +
 drivers/acpi/debug.c                      |   32 +++-
 drivers/char/hvc_iucv.c                   |    9 +-
 drivers/char/ipmi/ipmi_watchdog.c         |   42 +++--
 drivers/ide/ide.c                         |   20 ++-
 drivers/input/misc/ati_remote2.c          |   26 ++-
 drivers/input/mouse/psmouse-base.c        |   14 +-
 drivers/message/fusion/mptbase.c          |    3 +-
 drivers/misc/lkdtm.c                      |    4 +-
 drivers/net/myri10ge/myri10ge.c           |   54 ++++--
 drivers/net/wireless/libertas/if_sdio.c   |   32 +++-
 drivers/net/wireless/libertas/if_usb.c    |    3 +
 drivers/net/wireless/libertas_tf/if_usb.c |    3 +
 drivers/scsi/bfa/bfad.c                   |    2 +
 drivers/staging/rtl8187se/r8180_core.c    |    6 +-
 drivers/staging/rtl8192e/r8192E_core.c    |    6 +-
 drivers/staging/rtl8192su/r8192U_core.c   |    6 +-
 drivers/usb/atm/ueagle-atm.c              |    2 +
 drivers/video/uvesafb.c                   |    7 +-
 drivers/video/vt8623fb.c                  |    2 +
 fs/nfs/callback.c                         |   11 +-
 include/linux/moduleparam.h               |  282 ++++++++++++++++++++++-------
 init/main.c                               |    8 +-
 kernel/params.c                           |  233 ++++++++++++++++--------
 net/mac80211/rate.c                       |    2 +
 net/sunrpc/xprtsock.c                     |   26 ++-
 scripts/mod/modpost.c                     |   13 ++
 27 files changed, 623 insertions(+), 235 deletions(-)

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Artem Bityutskiy]

On Wed, 2010-05-05 at 15:03 +0930, Rusty Russell wrote:
> On Wed, 5 May 2010 03:37:19 am Artem Bityutskiy wrote:
> > On Tue, 2010-05-04 at 11:53 +0930, Rusty Russell wrote:
> > > On Tue, 27 Apr 2010 08:23:24 pm Artem Bityutskiy wrote:
> > > > Rusty, correct me if I'm wrong, but it looks like the above memleak was
> > > > introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added
> > > > the kstrdup(). So you kinda fixed the sysfs case (it still memleaks
> > > > though), but at the cost of additional insmod/rmmod leak, right?
> > > 
> > > Yep!
> > 
> > Are you working/planning to work on fixing this regression?
> 
> I'm still ambivalent on it; I have patches but it's a lot of churn for not
> much gain.
> 
> To fix this, we need a way to lock parameters against changing by sysfs, and
> we need to use it everywhere.  Past experience has demonstrated that this will
> never be maintained.  
> 
> On the other hand, the leak is trivial.

Well, I am not very concerned with the "changing by sysfs" leak. This is
not a big deal, IMHO. I am concerned with the "rmmod" leak, which did
not exist before your patches, but exists now. People may do a lot of
insmod/rmmod, and on each rmmod they will loose this kstrdup-ed string. 

I'll take a look at this tree, thank you.

> Here's a git tree if you want to look further:
> 
> The following changes since commit 05ce7bfe547c9fa967d9cab6c37867a9cb6fb3fa:
>   Linus Torvalds (1):
>         Merge branch 'for_linus' of git://git.kernel.org/.../jack/linux-fs-2.6
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/rusty/linux-2.6-param-fixes.git master

Just in case others will clone, the correct URL seems to be

git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-param-fixes.git master

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Takashi Iwai]

At Wed, 05 May 2010 10:25:14 +0300,
Artem Bityutskiy wrote:
> 
> On Wed, 2010-05-05 at 15:03 +0930, Rusty Russell wrote:
> > On Wed, 5 May 2010 03:37:19 am Artem Bityutskiy wrote:
> > > On Tue, 2010-05-04 at 11:53 +0930, Rusty Russell wrote:
> > > > On Tue, 27 Apr 2010 08:23:24 pm Artem Bityutskiy wrote:
> > > > > Rusty, correct me if I'm wrong, but it looks like the above memleak was
> > > > > introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added
> > > > > the kstrdup(). So you kinda fixed the sysfs case (it still memleaks
> > > > > though), but at the cost of additional insmod/rmmod leak, right?
> > > > 
> > > > Yep!
> > > 
> > > Are you working/planning to work on fixing this regression?
> > 
> > I'm still ambivalent on it; I have patches but it's a lot of churn for not
> > much gain.
> > 
> > To fix this, we need a way to lock parameters against changing by sysfs, and
> > we need to use it everywhere.  Past experience has demonstrated that this will
> > never be maintained.  
> > 
> > On the other hand, the leak is trivial.
> 
> Well, I am not very concerned with the "changing by sysfs" leak. This is
> not a big deal, IMHO. I am concerned with the "rmmod" leak, which did
> not exist before your patches, but exists now. People may do a lot of
> insmod/rmmod, and on each rmmod they will loose this kstrdup-ed string. 

I don't think there are so many real cases that actually do leaking.
This is only for charp type parameters (not string), and no leak
happens unless user gives the value explicitly via a module option.

Fixing in the way of the later upstream is a bit too intrusive as a
stable patch.  So, I'm also not sure whether we should take it,
too...


thanks,

Takashi

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Artem Bityutskiy]

On Wed, 2010-05-05 at 09:44 +0200, Takashi Iwai wrote:
> At Wed, 05 May 2010 10:25:14 +0300,
> Artem Bityutskiy wrote:
> > 
> > On Wed, 2010-05-05 at 15:03 +0930, Rusty Russell wrote:
> > > On Wed, 5 May 2010 03:37:19 am Artem Bityutskiy wrote:
> > > > On Tue, 2010-05-04 at 11:53 +0930, Rusty Russell wrote:
> > > > > On Tue, 27 Apr 2010 08:23:24 pm Artem Bityutskiy wrote:
> > > > > > Rusty, correct me if I'm wrong, but it looks like the above memleak was
> > > > > > introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added
> > > > > > the kstrdup(). So you kinda fixed the sysfs case (it still memleaks
> > > > > > though), but at the cost of additional insmod/rmmod leak, right?
> > > > > 
> > > > > Yep!
> > > > 
> > > > Are you working/planning to work on fixing this regression?
> > > 
> > > I'm still ambivalent on it; I have patches but it's a lot of churn for not
> > > much gain.
> > > 
> > > To fix this, we need a way to lock parameters against changing by sysfs, and
> > > we need to use it everywhere.  Past experience has demonstrated that this will
> > > never be maintained.  
> > > 
> > > On the other hand, the leak is trivial.
> > 
> > Well, I am not very concerned with the "changing by sysfs" leak. This is
> > not a big deal, IMHO. I am concerned with the "rmmod" leak, which did
> > not exist before your patches, but exists now. People may do a lot of
> > insmod/rmmod, and on each rmmod they will loose this kstrdup-ed string. 
> 
> I don't think there are so many real cases that actually do leaking.

I am sorry, but let me disagree. Did you count these cases? Why are you
so sure?

We are one live case. We use drivers/usb/gadget/nokia.c. And this is
also used in production, in the Nokia N900 phone. 

IOW, I officially confirm that we are affected by this regression.

And there are many other potential charp users in drivers/usb/gadget.
Take a look at drivers/usb/gadget/composite.c:

static char *iManufacturer;
module_param(iManufacturer, charp, 0);
MODULE_PARM_DESC(iManufacturer, "USB Manufacturer string");

static char *iProduct;
module_param(iProduct, charp, 0);
MODULE_PARM_DESC(iProduct, "USB Product string");

static char *iSerialNumber;
module_param(iSerialNumber, charp, 0);
MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");

This file is included from many other files:

[dedekind@eru gadget]$ pwd
/home/dedekind/git/linux-2.6-param-fixes/drivers/usb/gadget
[dedekind@eru gadget]$ grep 'composite.c' *
audio.c:#include "composite.c"
cdc2.c:#include "composite.c"
composite.c: * composite.c - infrastructure for Composite USB Gadgets
ether.c:#include "composite.c"
mass_storage.c:#include "composite.c"
multi.c:#include "composite.c"
nokia.c:#include "composite.c"
serial.c:#include "composite.c"
zero.c:#include "composite.c"

They are potentially affected too.

> This is only for charp type parameters (not string), and no leak
> happens unless user gives the value explicitly via a module option.

We do use these options. Surely, if they exist, people probably use at
least some of them, right? Otherwise why would they exist?

And I officially confirm that we load/unload the g_nokia gadget
(corresponds to nokia.c) many times, and we are not very interested in
having (even though small) memory leak.

> Fixing in the way of the later upstream is a bit too intrusive as a
> stable patch.  So, I'm also not sure whether we should take it,
> too...

To be frank I do not really understand what you mean.

Anyway, I just humbly suggest not to have the "no one uses that, let's
have a leak" attitude. I do understand that this is a 'it's a lot of
churn for not much gain'. However, I think the rmmod leak is large
enough issue.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Artem Bityutskiy]

On Wed, 2010-05-05 at 11:49 +0300, Artem Bityutskiy wrote:
> > Fixing in the way of the later upstream is a bit too intrusive as a
> > stable patch.  So, I'm also not sure whether we should take it,
> > too...
> 
> To be frank I do not really understand what you mean.

If you meant 'let's not change it 2.6.34' - fair enough, it was already
in 2.6.32, so there is probably no reason to hurry. But I suggest to
still consider this as a big issue and fix as soon as we can.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Rusty Russell]

On Wed, 5 May 2010 06:19:29 pm Artem Bityutskiy wrote:
> > Fixing in the way of the later upstream is a bit too intrusive as a
> > stable patch.  So, I'm also not sure whether we should take it,
> > too...
> 
> To be frank I do not really understand what you mean.
> 
> Anyway, I just humbly suggest not to have the "no one uses that, let's
> have a leak" attitude. I do understand that this is a 'it's a lot of
> churn for not much gain'. However, I think the rmmod leak is large
> enough issue.

Thanks Artem, that's exactly the kind of feedback we need.

For most people, module parameters are rare, and module removal is rare.
So the amount of leak is less than the size of the code we would add to fix
it.

If this is hitting you, it clearly changes the priorities.  I will include
the patches now.

Thanks!
Rusty.

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Takashi Iwai]

At Wed, 05 May 2010 12:04:18 +0300,
Artem Bityutskiy wrote:
> 
> On Wed, 2010-05-05 at 11:49 +0300, Artem Bityutskiy wrote:
> > > Fixing in the way of the later upstream is a bit too intrusive as a
> > > stable patch.  So, I'm also not sure whether we should take it,
> > > too...
> > 
> > To be frank I do not really understand what you mean.
> 
> If you meant 'let's not change it 2.6.34' - fair enough, it was already
> in 2.6.32, so there is probably no reason to hurry. But I suggest to
> still consider this as a big issue and fix as soon as we can.

Ah, sorry I was confused.  I somehow thought Rusty's patch has been
already merged after 2.6.32, but it didn't happen in reality.

Then I'm for merging the changes even for 2.6.34 although it's late.
In the API side, there aren't many changes.  And it might reduce the
whole binary size in the end by the use of module_parm_ops pointer,
too.


thanks,

Takashi

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Phil Carmody]

On 06/05/10 04:28 +0200, ext Rusty Russell wrote:
> On Wed, 5 May 2010 06:19:29 pm Artem Bityutskiy wrote:
> > > Fixing in the way of the later upstream is a bit too intrusive as a
> > > stable patch.  So, I'm also not sure whether we should take it,
> > > too...
> > 
> > To be frank I do not really understand what you mean.
> > 
> > Anyway, I just humbly suggest not to have the "no one uses that, let's
> > have a leak" attitude. I do understand that this is a 'it's a lot of
> > churn for not much gain'. However, I think the rmmod leak is large
> > enough issue.
> 
> Thanks Artem, that's exactly the kind of feedback we need.
> 
> For most people, module parameters are rare, and module removal is rare.
> So the amount of leak is less than the size of the code we would add to fix
> it.
> 
> If this is hitting you, it clearly changes the priorities.  I will include
> the patches now.

Rusty,

Artem's passed the baton over to me to investigate, so I've reviewed
and back-ported the last known version of your patchset. I'm happy to 
report that the 100% reproducable leak that we were seeing before
cannot be reproduced. As expected, given review of the code. I have 
not been able to test the final driver-specific patches from your 
patchset, but up to and including

[PATCH 12/18] param: simple locking for sysfs-writable charp parameters

they can all have a:

Tested-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

I'm quite interested to see these pushed into the mainline so that I
can cherry-pick final versions for our internal tree, do you have any 
schedule for that?

Cheers,
Phil

Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix

[Rusty Russell]

On Wed, 23 Jun 2010 02:20:11 am Phil Carmody wrote:
> Artem's passed the baton over to me to investigate, so I've reviewed
> and back-ported the last known version of your patchset. I'm happy to 
> report that the 100% reproducable leak that we were seeing before
> cannot be reproduced. As expected, given review of the code. I have 
> not been able to test the final driver-specific patches from your 
> patchset, but up to and including
> 
> [PATCH 12/18] param: simple locking for sysfs-writable charp parameters
> 
> they can all have a:
> 
> Tested-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> I'm quite interested to see these pushed into the mainline so that I
> can cherry-pick final versions for our internal tree, do you have any 
> schedule for that?

Thanks, Phil, I've added that.  Testing is always good!

The patches are sitting in linux-next now, ready for the next merge window
(ie. 2.6.36)

Cheers,
Rusty.