param sysfs oops (simple, leaky) fix, bool arrays fix
in [Kernel]
|
Prev: [PATCH] x86, amd: Check X86_FEATURE_OSVW bit before accessing OSVW MSRs
Next: [PATCH 15/22] KVM: MMU: Introduce kvm_read_guest_page_x86()
From: Artem Bityutskiy on 27 Apr 2010 06:40 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(a)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 (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 27 Apr 2010 07:00 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(a)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 (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rusty Russell on 4 May 2010 00:20 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(a)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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 4 May 2010 14:10 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(a)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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rusty Russell on 5 May 2010 01:40
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |