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


\
 
 \ /
  Last update: 2010-05-04 06:21    [from the cache]
©2003-2011 Jasper Spaans