lkml.org 
[lkml]   [2010]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix
From
Date
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 (Артём Битюцкий)

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

\
 
 \ /
  Last update: 2010-05-04 20:11    [W:0.066 / U:6.384 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site