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 5 May 2010 03:30 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 (Артём Битюцкий) -- 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: Takashi Iwai on 5 May 2010 03:50 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 -- 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 5 May 2010 05:00 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(a)eru gadget]$ pwd /home/dedekind/git/linux-2.6-param-fixes/drivers/usb/gadget [dedekind(a)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(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 5 May 2010 05:10 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 (Артём Битюцкий) -- 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 22:30
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. -- 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/ |