login
Search
M68k builds with current mainline fail with
init/built-in.o: In function `kernel_init':
main.c:(.init.text+0x730): undefined reference to `strlen'
Reverting commit e662e1cfd434aa234b72fbc781f1d70211cb785b
Author: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Mon May 12 14:02:22 2008 -0700
init: don't lose initcall return values
There is an ability to lose an initcall return value if it happened with irq
disabled or imbalanced preemption (and if we debug initcall).
fixes the problem. My first guess is gcc is turning sizeof() into strlen()
again.
Sample build log available at
http://kisskb.ellerman.id.au/kisskb/buildresult/27596/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
Definitely not. sizeof is a compile-time constant, strlen is not. More likely the strlen call is embedded in the expansion of strncat. Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." --
I've missed to add #include <linux/string.h> sorry, will make the patch today evening (or maybe someone could make it). Thanks for report! --
hmm, string.h is aready there, so there are two reason for fail I could imagine at first glance: or gcc does something strange, or the problem with linking string.o. Will check it. --
m68k is one of the architectures not using -ffreestanding, so this
kind of problems is somehow expected...
We could add -ffreestanding on m68k.
Or replace all the strlen stuff in include/asm-m68k/string.h with a
function prototype, which lets gcc choose itself whether it wants to use
the builtin or the version from lib/string.c, and makes an out-of-line
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
I don't think the strlen macros has any influence in this case (it already uses __builtin_strlen anyway). I'd rather guess that gcc is expanding strncat internally to something involving strlen with non-constant argument, although I cannot see how that can happen from a quick look. Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." --
[Andreas Schwab - Wed, May 14, 2008 at 04:28:24PM +0200] | Adrian Bunk <bunk@kernel.org> writes: | | > Or replace all the strlen stuff in include/asm-m68k/string.h with a | | I don't think the strlen macros has any influence in this case (it | already uses __builtin_strlen anyway). I'd rather guess that gcc is | expanding strncat internally to something involving strlen with | non-constant argument, although I cannot see how that can happen from a | quick look. | | Andreas. | | -- | Andreas Schwab, SuSE Labs, schwab@suse.de | SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany | PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 | "And now for something completely different." | I think it would help to see ..tmp_vmlinux1.cmd to ensure for inclusion of lib/lib.a. strlen was there without my patch as Andreas already pointed, I've just added strncat wich is coming from lib/string.o for this arch. - Cyrill - --
Actually the way strncat is used here is broken anyway, it does not prevent array overrun. The third argument of strncat only limits the amount of characters copied, without taking into account the length of the string already in the buffer. Consequently gcc has optimized the call to strncat into a simple call to strcat, since none of the copied strings are longer than sizeof(msgbuf). This strcat call is then expanded to include a call to strlen. So a better fix would probably be to make msgbuf big enough and use strcat instead. Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." --
[Andreas Schwab - Wed, May 14, 2008 at 04:55:37PM +0200] | Cyrill Gorcunov <gorcunov@gmail.com> writes: | | > I think it would help to see ..tmp_vmlinux1.cmd to ensure for inclusion | > of lib/lib.a. strlen was there without my patch as Andreas already pointed, | > I've just added strncat wich is coming from lib/string.o for this arch. | | Actually the way strncat is used here is broken anyway, it does not | prevent array overrun. The third argument of strncat only limits the | amount of characters copied, without taking into account the length of | the string already in the buffer. Consequently gcc has optimized the | call to strncat into a simple call to strcat, since none of the copied | strings are longer than sizeof(msgbuf). This strcat call is then | expanded to include a call to strlen. | | So a better fix would probably be to make msgbuf big enough and use | strcat instead. | | Andreas. | | -- | Andreas Schwab, SuSE Labs, schwab@suse.de | SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany | PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 | "And now for something completely different." | Thanks Andreas, I'll fix it. - Cyrill - --
[Andreas Schwab - Wed, May 14, 2008 at 04:55:37PM +0200]
| Cyrill Gorcunov <gorcunov@gmail.com> writes:
|
| > I think it would help to see ..tmp_vmlinux1.cmd to ensure for inclusion
| > of lib/lib.a. strlen was there without my patch as Andreas already pointed,
| > I've just added strncat wich is coming from lib/string.o for this arch.
|
| Actually the way strncat is used here is broken anyway, it does not
| prevent array overrun. The third argument of strncat only limits the
| amount of characters copied, without taking into account the length of
| the string already in the buffer. Consequently gcc has optimized the
| call to strncat into a simple call to strcat, since none of the copied
| strings are longer than sizeof(msgbuf). This strcat call is then
| expanded to include a call to strlen.
|
| So a better fix would probably be to make msgbuf big enough and use
| strcat instead.
|
| Andreas.
|
|
Andreas, could you test/review the following patch
(what is worse - I *have* introduced buffer overflow with my
patch, so I think we should fix it asap)
---
Index: linux-2.6.git/init/main.c
===================================================================
--- linux-2.6.git.orig/init/main.c 2008-05-14 17:55:10.000000000 +0400
+++ linux-2.6.git/init/main.c 2008-05-14 19:11:18.000000000 +0400
@@ -702,7 +702,7 @@ static void __init do_initcalls(void)
for (call = __initcall_start; call < __initcall_end; call++) {
ktime_t t0, t1, delta;
- char msgbuf[40];
+ char msgbuf[64];
int result;
if (initcall_debug) {
@@ -729,11 +729,11 @@ static void __init do_initcalls(void)
sprintf(msgbuf, "error code %d ", result);
if (preempt_count() != count) {
- strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
+ strcat(msgbuf, "preemption imbalance ");
preempt_count() = count;
}
if (irqs_disabled()) {
- strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
+ strcat(msgbuf, "disabled interrupts ");
local_irq_enable();
...I've compiled the file with the various cross compilers I have laying around here, no reference to strlen any more. Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." --
[Andreas Schwab - Wed, May 14, 2008 at 05:27:35PM +0200] | Cyrill Gorcunov <gorcunov@gmail.com> writes: | | > Andreas, could you test/review the following patch | > (what is worse - I *have* introduced buffer overflow with my | > patch, so I think we should fix it asap) | | I've compiled the file with the various cross compilers I have laying | around here, no reference to strlen any more. | | Andreas. | | -- | Andreas Schwab, SuSE Labs, schwab@suse.de | SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany | PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 | "And now for something completely different." | Thanks a lot, Andreas! I'll sent the patch to Linus, 'case having potential buffer overflow is not good for mainline - Cyrill - --
Same here. Builds and runs fine (on ARAnyM). Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
[Andreas Schwab - Wed, May 14, 2008 at 04:28:24PM +0200] | Adrian Bunk <bunk@kernel.org> writes: | | > Or replace all the strlen stuff in include/asm-m68k/string.h with a | | I don't think the strlen macros has any influence in this case (it | already uses __builtin_strlen anyway). I'd rather guess that gcc is | expanding strncat internally to something involving strlen with | non-constant argument, although I cannot see how that can happen from a | quick look. | | Andreas. | | -- | Andreas Schwab, SuSE Labs, schwab@suse.de | SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany | PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 | "And now for something completely different." | actually, i can rewrite it using memcpy if strncat causes such a problems - Cyrill - --
The problem is that it sets __HAVE_ARCH_STRLEN, and therefore the
out-of-line function in lib/string.c is not built.
That breaks when gcc replaces a call to a different function with a call
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
[Adrian Bunk - Wed, May 14, 2008 at 06:30:59PM +0300] | On Wed, May 14, 2008 at 04:28:24PM +0200, Andreas Schwab wrote: | > Adrian Bunk <bunk@kernel.org> writes: | > | > > Or replace all the strlen stuff in include/asm-m68k/string.h with a | > | > I don't think the strlen macros has any influence in this case (it | > already uses __builtin_strlen anyway). | | The problem is that it sets __HAVE_ARCH_STRLEN, and therefore the | out-of-line function in lib/string.c is not built. | | That breaks when gcc replaces a call to a different function with a call | to strlen(). | | > I'd rather guess that gcc is | > expanding strncat internally to something involving strlen with | > non-constant argument, | | We agree on this one. | | > although I cannot see how that can happen from a | > quick look. | | It isn't the first time we have these problems in the kernel... | | > Andreas. | | cu | Adrian | | -- | | "Is there not promise of rain?" Ling Tan asked suddenly out | of the darkness. There had been need of rain for many days. | "Only a promise," Lao Er said. | Pearl S. Buck - Dragon Seed | | Adrian, I've just sent the updated patch, and accidentally forget to CC you - please take a look on http://lkml.org/lkml/2008/5/14/267 - Cyrill - --
It removes the strncat() calls and should therefore remove the current
compile error.
But the underlying problem that actually causes the compile error is not
fixed and might occur again in other places (but that's nothing you
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
[Adrian Bunk - Wed, May 14, 2008 at 06:56:11PM +0300] | On Wed, May 14, 2008 at 07:48:01PM +0400, Cyrill Gorcunov wrote: | > [Adrian Bunk - Wed, May 14, 2008 at 06:30:59PM +0300] | > | On Wed, May 14, 2008 at 04:28:24PM +0200, Andreas Schwab wrote: | > | > Adrian Bunk <bunk@kernel.org> writes: | > | > | > | > > Or replace all the strlen stuff in include/asm-m68k/string.h with a | > | > | > | > I don't think the strlen macros has any influence in this case (it | > | > already uses __builtin_strlen anyway). | > | | > | The problem is that it sets __HAVE_ARCH_STRLEN, and therefore the | > | out-of-line function in lib/string.c is not built. | > | | > | That breaks when gcc replaces a call to a different function with a call | > | to strlen(). | > | | > | > I'd rather guess that gcc is | > | > expanding strncat internally to something involving strlen with | > | > non-constant argument, | > | | > | We agree on this one. | > | | > | > although I cannot see how that can happen from a | > | > quick look. | > | | > | It isn't the first time we have these problems in the kernel... | > | | > | > Andreas. | > | | > | cu | > | Adrian | > | > Adrian, I've just sent the updated patch, and accidentally forget | > to CC you - please take a look on http://lkml.org/lkml/2008/5/14/267 | | It removes the strncat() calls and should therefore remove the current | compile error. | | But the underlying problem that actually causes the compile error is not | fixed and might occur again in other places (but that's nothing you | have to worry about). | | > - Cyrill - | | cu | Adrian | | -- | | "Is there not promise of rain?" Ling Tan asked suddenly out | of the darkness. There had been need of rain for many days. | "Only a promise," Lao Er said. | Pearl S. Buck - Dragon Seed | Yes, this problem is still here but I was really frowned 'cause it's my patch introduced potential memory overflow ...
Hi, If it's generated by gcc, we can simply provide an uninlined version of it in arch/m68k/lib/string.c similiar to strcpy (and there is already a __kernel_strlen). bye, Roman --
