* [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c)
@ 2004-12-07 21:23 Jesper Juhl
2004-12-07 21:29 ` Jens Axboe
2004-12-08 1:57 ` Rusty Russell
0 siblings, 2 replies; 6+ messages in thread
From: Jesper Juhl @ 2004-12-07 21:23 UTC (permalink / raw)
To: linux-kernel; +Cc: Katrina Tsipenyuk, katrina, Rusty Russell
Problem reported by Katrina Tsipenyuk and the Fortify Software engineering
team in thread with subject "PROBLEM: unchecked returns from kmalloc() in
linux-2.6.10-rc2".
The patch attempts to handle a failed kmalloc() a bit better than it
currently is. As I see it (and I'm not familliar with this code) there's
no really good way to cope with kmalloc failing on us here, so the best we
can do is print an error message and return a meaningful error value. As
the function is used with __initcall() I don't think much will actually
come of the negatve return, but returning -ENOMEM seems to me to be the
proper thing to do. Comments from someone who's actually familliar with
the code is very welcome.
Patch has been compile tested, boot tested, and didn't immediately blow
up my kernel, but that's all. Please review before applying.
Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
diff -up linux-2.6.10-rc3-bk2-orig/kernel/module.c linux-2.6.10-rc3-bk2/kernel/module.c
--- linux-2.6.10-rc3-bk2-orig/kernel/module.c 2004-12-06 22:24:56.000000000 +0100
+++ linux-2.6.10-rc3-bk2/kernel/module.c 2004-12-07 21:17:00.000000000 +0100
@@ -334,6 +334,10 @@ static int percpu_modinit(void)
pcpu_num_allocated = 2;
pcpu_size = kmalloc(sizeof(pcpu_size[0]) * pcpu_num_allocated,
GFP_KERNEL);
+ if (!pcpu_size) {
+ printk(KERN_ERR "Unable to allocate per-cpu memory for modules.");
+ return -ENOMEM;
+ }
/* Static in-kernel percpu data (used). */
pcpu_size[0] = -ALIGN(__per_cpu_end-__per_cpu_start, SMP_CACHE_BYTES);
/* Free room. */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) 2004-12-07 21:23 [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) Jesper Juhl @ 2004-12-07 21:29 ` Jens Axboe 2004-12-07 22:56 ` Jesper Juhl 2004-12-07 22:57 ` Andries Brouwer 2004-12-08 1:57 ` Rusty Russell 1 sibling, 2 replies; 6+ messages in thread From: Jens Axboe @ 2004-12-07 21:29 UTC (permalink / raw) To: Jesper Juhl; +Cc: linux-kernel, Katrina Tsipenyuk, katrina, Rusty Russell On Tue, Dec 07 2004, Jesper Juhl wrote: > > Problem reported by Katrina Tsipenyuk and the Fortify Software engineering > team in thread with subject "PROBLEM: unchecked returns from kmalloc() in > linux-2.6.10-rc2". > > The patch attempts to handle a failed kmalloc() a bit better than it > currently is. As I see it (and I'm not familliar with this code) there's > no really good way to cope with kmalloc failing on us here, so the best we > can do is print an error message and return a meaningful error value. As > the function is used with __initcall() I don't think much will actually > come of the negatve return, but returning -ENOMEM seems to me to be the > proper thing to do. Comments from someone who's actually familliar with > the code is very welcome. > > Patch has been compile tested, boot tested, and didn't immediately blow > up my kernel, but that's all. Please review before applying. > > Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk> > > diff -up linux-2.6.10-rc3-bk2-orig/kernel/module.c linux-2.6.10-rc3-bk2/kernel/module.c > --- linux-2.6.10-rc3-bk2-orig/kernel/module.c 2004-12-06 22:24:56.000000000 +0100 > +++ linux-2.6.10-rc3-bk2/kernel/module.c 2004-12-07 21:17:00.000000000 +0100 > @@ -334,6 +334,10 @@ static int percpu_modinit(void) > pcpu_num_allocated = 2; > pcpu_size = kmalloc(sizeof(pcpu_size[0]) * pcpu_num_allocated, > GFP_KERNEL); > + if (!pcpu_size) { > + printk(KERN_ERR "Unable to allocate per-cpu memory for modules."); > + return -ENOMEM; > + } I'd say these cases are similar to SLAB_PANIC. Since it runs at boot, if it fails it's likely an indication of some other problem, so dealing with it here is silly. Perhaps just panic() on a NULL return. Both of these fortify cases aren't real problems, imho. They trip a stupid (no offense to the analyzer, but it's not human :) static analyzer, that's all. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) 2004-12-07 21:29 ` Jens Axboe @ 2004-12-07 22:56 ` Jesper Juhl 2004-12-07 22:57 ` Andries Brouwer 1 sibling, 0 replies; 6+ messages in thread From: Jesper Juhl @ 2004-12-07 22:56 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-kernel, Katrina Tsipenyuk, katrina, Rusty Russell On Tue, 7 Dec 2004, Jens Axboe wrote: > On Tue, Dec 07 2004, Jesper Juhl wrote: > > > > Problem reported by Katrina Tsipenyuk and the Fortify Software engineering > > team in thread with subject "PROBLEM: unchecked returns from kmalloc() in > > linux-2.6.10-rc2". > > > > The patch attempts to handle a failed kmalloc() a bit better than it > > currently is. As I see it (and I'm not familliar with this code) there's > > no really good way to cope with kmalloc failing on us here, so the best we > > can do is print an error message and return a meaningful error value. As > > the function is used with __initcall() I don't think much will actually > > come of the negatve return, but returning -ENOMEM seems to me to be the > > proper thing to do. Comments from someone who's actually familliar with > > the code is very welcome. > > > > Patch has been compile tested, boot tested, and didn't immediately blow > > up my kernel, but that's all. Please review before applying. > > > > Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk> > > > > diff -up linux-2.6.10-rc3-bk2-orig/kernel/module.c linux-2.6.10-rc3-bk2/kernel/module.c > > --- linux-2.6.10-rc3-bk2-orig/kernel/module.c 2004-12-06 22:24:56.000000000 +0100 > > +++ linux-2.6.10-rc3-bk2/kernel/module.c 2004-12-07 21:17:00.000000000 +0100 > > @@ -334,6 +334,10 @@ static int percpu_modinit(void) > > pcpu_num_allocated = 2; > > pcpu_size = kmalloc(sizeof(pcpu_size[0]) * pcpu_num_allocated, > > GFP_KERNEL); > > + if (!pcpu_size) { > > + printk(KERN_ERR "Unable to allocate per-cpu memory for modules."); > > + return -ENOMEM; > > + } > > I'd say these cases are similar to SLAB_PANIC. Since it runs at boot, if > it fails it's likely an indication of some other problem, so dealing > with it here is silly. Perhaps just panic() on a NULL return. > > Both of these fortify cases aren't real problems, imho. They trip a > stupid (no offense to the analyzer, but it's not human :) static > analyzer, that's all. > So, this would be another user of GFP_PANIC if such a one is ever implemented. For now, how does a patch like this look to you? : Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk> diff -up linux-2.6.10-rc3-bk2-orig/kernel/module.c linux-2.6.10-rc3-bk2/kernel/module.c --- linux-2.6.10-rc3-bk2-orig/kernel/module.c 2004-12-06 22:24:56.000000000 +0100 +++ linux-2.6.10-rc3-bk2/kernel/module.c 2004-12-07 23:54:01.000000000 +0100 @@ -334,6 +334,8 @@ static int percpu_modinit(void) pcpu_num_allocated = 2; pcpu_size = kmalloc(sizeof(pcpu_size[0]) * pcpu_num_allocated, GFP_KERNEL); + if (!pcpu_size) + panic("Unable to allocate per-cpu memory for modules."); /* Static in-kernel percpu data (used). */ pcpu_size[0] = -ALIGN(__per_cpu_end-__per_cpu_start, SMP_CACHE_BYTES); /* Free room. */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) 2004-12-07 21:29 ` Jens Axboe 2004-12-07 22:56 ` Jesper Juhl @ 2004-12-07 22:57 ` Andries Brouwer 2004-12-08 7:01 ` Jens Axboe 1 sibling, 1 reply; 6+ messages in thread From: Andries Brouwer @ 2004-12-07 22:57 UTC (permalink / raw) To: Jens Axboe Cc: Jesper Juhl, linux-kernel, Katrina Tsipenyuk, katrina, Rusty Russell On Tue, Dec 07, 2004 at 10:29:58PM +0100, Jens Axboe wrote: > > diff -up linux-2.6.10-rc3-bk2-orig/kernel/module.c linux-2.6.10-rc3-bk2/kernel/module.c > > --- linux-2.6.10-rc3-bk2-orig/kernel/module.c 2004-12-06 22:24:56.000000000 +0100 > > +++ linux-2.6.10-rc3-bk2/kernel/module.c 2004-12-07 21:17:00.000000000 +0100 > > @@ -334,6 +334,10 @@ static int percpu_modinit(void) > > pcpu_num_allocated = 2; > > pcpu_size = kmalloc(sizeof(pcpu_size[0]) * pcpu_num_allocated, > > GFP_KERNEL); > > + if (!pcpu_size) { > > + printk(KERN_ERR "Unable to allocate per-cpu memory for modules."); > > + return -ENOMEM; > > + } > > I'd say these cases are similar to SLAB_PANIC. Since it runs at boot, if > it fails it's likely an indication of some other problem, so dealing > with it here is silly. Perhaps just panic() on a NULL return. > > Both of these fortify cases aren't real problems, imho. They trip a > stupid (no offense to the analyzer, but it's not human :) static > analyzer, that's all. Hi Jens - I think I disagree a little. Experience shows that if the stupid static analyzer spits out a hundred complaints, there are maybe five real problems. If the source is not fixed in some way then the real problem cases drown in the noise of "harmless" warnings. Remains the question how to fix the source without causing bloat by inserting lots of strings. Easiest is perhaps to write debug_printk() instead of printk() so that the string is compiled away for everyone except for me when I need debugging help to find out why my kernel does not boot. Andries ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) 2004-12-07 22:57 ` Andries Brouwer @ 2004-12-08 7:01 ` Jens Axboe 0 siblings, 0 replies; 6+ messages in thread From: Jens Axboe @ 2004-12-08 7:01 UTC (permalink / raw) To: Andries Brouwer Cc: Jesper Juhl, linux-kernel, Katrina Tsipenyuk, katrina, Rusty Russell On Tue, Dec 07 2004, Andries Brouwer wrote: > On Tue, Dec 07, 2004 at 10:29:58PM +0100, Jens Axboe wrote: > > > > diff -up linux-2.6.10-rc3-bk2-orig/kernel/module.c linux-2.6.10-rc3-bk2/kernel/module.c > > > --- linux-2.6.10-rc3-bk2-orig/kernel/module.c 2004-12-06 22:24:56.000000000 +0100 > > > +++ linux-2.6.10-rc3-bk2/kernel/module.c 2004-12-07 21:17:00.000000000 +0100 > > > @@ -334,6 +334,10 @@ static int percpu_modinit(void) > > > pcpu_num_allocated = 2; > > > pcpu_size = kmalloc(sizeof(pcpu_size[0]) * pcpu_num_allocated, > > > GFP_KERNEL); > > > + if (!pcpu_size) { > > > + printk(KERN_ERR "Unable to allocate per-cpu memory for modules."); > > > + return -ENOMEM; > > > + } > > > > I'd say these cases are similar to SLAB_PANIC. Since it runs at boot, if > > it fails it's likely an indication of some other problem, so dealing > > with it here is silly. Perhaps just panic() on a NULL return. > > > > Both of these fortify cases aren't real problems, imho. They trip a > > stupid (no offense to the analyzer, but it's not human :) static > > analyzer, that's all. > > Hi Jens - > > I think I disagree a little. Experience shows that if the stupid > static analyzer spits out a hundred complaints, there are maybe five > real problems. If the source is not fixed in some way then the real > problem cases drown in the noise of "harmless" warnings. I completely agree. But that's a case of silencing the cases that aren't interesting, so the interesting bits don't get lost in old noise. > Remains the question how to fix the source without causing bloat by > inserting lots of strings. Easiest is perhaps to write debug_printk() > instead of printk() so that the string is compiled away for everyone > except for me when I need debugging help to find out why my kernel > does not boot. The GFP_PANIC would at least only need one string :-) -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) 2004-12-07 21:23 [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) Jesper Juhl 2004-12-07 21:29 ` Jens Axboe @ 2004-12-08 1:57 ` Rusty Russell 1 sibling, 0 replies; 6+ messages in thread From: Rusty Russell @ 2004-12-08 1:57 UTC (permalink / raw) To: Jesper Juhl; +Cc: lkml - Kernel Mailing List, Katrina Tsipenyuk, katrina On Tue, 2004-12-07 at 22:23 +0100, Jesper Juhl wrote: > Problem reported by Katrina Tsipenyuk and the Fortify Software engineering > team in thread with subject "PROBLEM: unchecked returns from kmalloc() in > linux-2.6.10-rc2". IMHO a better fix is to (1) mark percpu_modinit() as __init. (2) ignore unhandled failures in __init functions. With some exceptions, I would prefer to see a not_on_init(cond) macro inside kmalloc et. al. which barfs in a consistent way when allocations, registrations, etc. fail on boot. Rusty. -- A bad analogy is like a leaky screwdriver -- Richard Braakman ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-12-08 7:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-12-07 21:23 [PATCH][1/2] fix unchecked returns from kmalloc() (in kernel/module.c) Jesper Juhl 2004-12-07 21:29 ` Jens Axboe 2004-12-07 22:56 ` Jesper Juhl 2004-12-07 22:57 ` Andries Brouwer 2004-12-08 7:01 ` Jens Axboe 2004-12-08 1:57 ` Rusty Russell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox