Re: 2.6.33-rc1 Reboot right after bootloader

Re: 2.6.33-rc1 Reboot right after bootloader

[Michael Guntsche]

Good evening,

I took me some time but I tracked down my reboot problem. The culprit is
commit 

4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
x86

The system I am compiling the kernel on has dash as its defaults shell.
With the commit applied I get an instant reboot, reverting the patch
makes it work again. 
If I switch my default shell to bash it works BOTH times.
I do not know why the shell makes a difference here though.
The version of dash is 0.5.5.1 from debian unstable.

Kind regards,
Michael

Re: 2.6.33-rc1 Reboot right after bootloader

[Sam Ravnborg]

On Sun, Dec 20, 2009 at 12:34:57AM +0100, Michael Guntsche wrote:
> Good evening,
> 
> I took me some time but I tracked down my reboot problem. The culprit is
> commit 
> 
> 4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
> x86
> 
> The system I am compiling the kernel on has dash as its defaults shell.
> With the commit applied I get an instant reboot, reverting the patch
> makes it work again. 
> If I switch my default shell to bash it works BOTH times.
> I do not know why the shell makes a difference here though.
> The version of dash is 0.5.5.1 from debian unstable.

Hi Michael & Oliver.

Just to check what is going wrong here could you try to execute the following two commands:
printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
/usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x

Please try as above and also with full path to printf (/usr/bin/printf)

On my system it looks like this:

$ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004
$ /usr/bin/printf  \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004
$ echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004

	Sam

Re: 2.6.33-rc1 Reboot right after bootloader

[Johannes Stezenbach]

On Sun, Dec 20, 2009 at 09:46:56AM +0100, Sam Ravnborg wrote:
> On Sun, Dec 20, 2009 at 12:34:57AM +0100, Michael Guntsche wrote:
> > 
> > I took me some time but I tracked down my reboot problem. The culprit is
> > commit 
> > 
> > 4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
> > x86
> 
> Just to check what is going wrong here could you try to execute the following two commands:
> printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
> 
> Please try as above and also with full path to printf (/usr/bin/printf)

Debian dash has a built-in printf which doesn't support \x.
/usr/bin/printf works fine.

$ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 785c 3861 785c 3135 785c 3733 785c 3030
0000010
$ /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037                              
0000004
$ echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 6e2d 2065 785c 3861 785c 3135 785c 3733
0000010 785c 3030 000a                         
0000015


Johannes

Re: 2.6.33-rc1 Reboot right after bootloader

[Michael Tokarev]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

Johannes Stezenbach wrote:
> On Sun, Dec 20, 2009 at 09:46:56AM +0100, Sam Ravnborg wrote:
>> On Sun, Dec 20, 2009 at 12:34:57AM +0100, Michael Guntsche wrote:
>>> I took me some time but I tracked down my reboot problem. The culprit is
>>> commit 
>>>
>>> 4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
>>> x86
>> Just to check what is going wrong here could you try to execute the following two commands:
>> printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
>> /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
>> echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
>>
>> Please try as above and also with full path to printf (/usr/bin/printf)
> 
> Debian dash has a built-in printf which doesn't support \x.
> /usr/bin/printf works fine.

So it looks like I was wrong with the last patch here.
I changed `/bin/echo -ne' to printf.  And while it worked
for me at that time, apparently it does not work for others.

Now I wonder why it worked for me.  I can confirm that
in current debian testing (dash-0.5.5.1-3) and in debian
stable (dash-0.5.4-12) dash's built-in printf does not
interpret \x escape sequence.  I sure verified the fix
I proposed, rebuilding kernels in a freshly-installed
debian testing with /bin/sh pointing to dash.  I'll try
to investigate this.  (Side note: since the time this
issue first hit me, I carry 4a2ff67c88211026afcbdbc190c13f705dae1b59
locally.)

It is even more interesting.  I re-read POSIX description
of printf, see
http://www.opengroup.org/onlinepubs/000095399/utilities/printf.html
And this one, too, does NOT mention \x at all.  Printf is
still "better" than echo because for echo _no_ interpretation
of escape sequences is mandated -- see
http://www.opengroup.org/onlinepubs/000095399/utilities/echo.html
but that does not help much since for printf, while \-sequences
are mandated, particular \x is not, so \x is a {GNU|common sense|...}
extension.

What the...

So now I don't know what to do.  According to the standard, there's
no utility that will work here. ;)  Maybe the attached (together
with 4a2ff67c88211026afcbdbc190c13f705dae1b59) will make everyone happy?

/mjt

[-- Attachment #2: use-printf-octal.diff --]
[-- Type: text/x-patch, Size: 423 bytes --]

--- a/scripts/Makefile.lib.orig	2009-12-19 18:36:01.944153109 +0300
+++ a/scripts/Makefile.lib	2009-12-20 13:36:04.014530573 +0300
@@ -215,6 +215,6 @@
 	dec_size=$$(expr $$dec_size + $$fsize);				\
 done;									\
-printf "%08x" $$dec_size |						\
-	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
+printf "%012o" $$dec_size |						\
+	sed 's/\(...\)\(...\)\(...\)\(...\)/\\\\4\\\\3\\\\2\\\\1/g'	\
 )
 

Re: 2.6.33-rc1 Reboot right after bootloader

[Michael Tokarev]

Michael Tokarev wrote:
[]
> So now I don't know what to do.  According to the standard, there's
> no utility that will work here. ;)  Maybe the attached (together
> with 4a2ff67c88211026afcbdbc190c13f705dae1b59) will make everyone happy?

--- a/scripts/Makefile.lib.orig	2009-12-19 18:36:01.944153109 +0300
+++ a/scripts/Makefile.lib	2009-12-20 13:36:04.014530573 +0300
@@ -215,6 +215,6 @@
 	dec_size=$$(expr $$dec_size + $$fsize);				\
 done;									\
-printf "%08x" $$dec_size |						\
-	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
+printf "%012o" $$dec_size |						\
+	sed 's/\(...\)\(...\)\(...\)\(...\)/\\\\4\\\\3\\\\2\\\\1/g'	\
 )

Apparently that does not work with dash either.  It produces \4\3\2\1:

$ tail -c 4 arch/x86/boot/compressed/vmlinux.bin.lzma | hexdump
0000000 0304 0102

That's insane.

How about removing the whole hack and keeping the uncompressed size in
another file?  (I'll try to cook something today - it requires a bit
more changes than the 2-liner above).

/mjt

Re: 2.6.33-rc1 Reboot right after bootloader

[Michael Tokarev]

Michael Tokarev ?????:
> Michael Tokarev wrote:
> []
>> So now I don't know what to do.  According to the standard, there's
>> no utility that will work here. ;)  Maybe the attached (together
>> with 4a2ff67c88211026afcbdbc190c13f705dae1b59) will make everyone happy?
> 
> --- a/scripts/Makefile.lib.orig	2009-12-19 18:36:01.944153109 +0300
> +++ a/scripts/Makefile.lib	2009-12-20 13:36:04.014530573 +0300
> @@ -215,6 +215,6 @@
>  	dec_size=$$(expr $$dec_size + $$fsize);				\
>  done;									\
> -printf "%08x" $$dec_size |						\
> -	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
> +printf "%012o" $$dec_size |						\
> +	sed 's/\(...\)\(...\)\(...\)\(...\)/\\\\4\\\\3\\\\2\\\\1/g'	\
>  )
> 
> Apparently that does not work with dash either.  It produces \4\3\2\1:
..
..because one pair of backslashes are missing before each digit, should be:

+	sed 's/\(...\)\(...\)\(...\)\(...\)/\\\\\\4\\\\\\3\\\\\\2\\\\\\1/g'	\

But that still does not work, because a byte does not correspond to 3
octal characters, that's why it was hexadecimal...

/mjt

Re: 2.6.33-rc1 Reboot right after bootloader

[Michael Guntsche]

On 2009.12.20 09:46:56 , Sam Ravnborg wrote:
> Hi Michael & Oliver.
> 
> Just to check what is going wrong here could you try to execute the following two commands:
> printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
> 
> Please try as above and also with full path to printf (/usr/bin/printf)
> 
> On my system it looks like this:
> 
> $ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> 0000000 51a8 0037
> 0000004
Hi Sam,

With bash as shell I get the same results, dash is different though.

$ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 785c 3861 785c 3135 785c 3733 785c 3030
0000010

$ /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037                              
0000004

$ echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 6e2d 2065 785c 3861 785c 3135 785c 3733
0000010 785c 3030 000a                         
0000015

Kind regards,
Michael

[PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Sam Ravnborg]

We use ... printf \x ... when calculating the size of the
compressed kernel.
Unfortunately dash built-in printf does not support this notation
resulting in a non-bootable kernel.

Fix this by always using the external version of printf.

The commit that introduced this bug was:
4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
build for x86"

Reported-by: Michael Guntsche <mike@it-loops.com>
Cc: Oliver Hartkopp <oliver@hartkopp.net>
Cc: Johannes Stezenbach <js@sig21.net>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---

This should fix it but it would be great if it is tested.
Michal Marek, I think this needs to go upstream pretty quickly.

	Sam

 scripts/Makefile.lib |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..bd201d9 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -213,13 +213,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
 
 # Bzip2 and LZMA do not include size in file... so we have to fake that;
 # append the size as a 32-bit littleendian number as gzip does.
+# Note: dash built-in printf does not support \x so use /usr/bin version
 size_append = printf $(shell						\
 dec_size=0;								\
 for F in $1; do								\
 	fsize=$$(stat -c "%s" $$F);					\
 	dec_size=$$(expr $$dec_size + $$fsize);				\
 done;									\
-printf "%08x" $$dec_size |						\
+/usr/bin/printf "%08x" $$dec_size |						\
 	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
 )
 
-- 
1.6.0.6

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Oliver Hartkopp]

Sam Ravnborg wrote:
> We use ... printf \x ... when calculating the size of the
> compressed kernel.
> Unfortunately dash built-in printf does not support this notation
> resulting in a non-bootable kernel.
> 
> Fix this by always using the external version of printf.
> 
> The commit that introduced this bug was:
> 4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
> build for x86"
> 
> Reported-by: Michael Guntsche <mike@it-loops.com>
> Cc: Oliver Hartkopp <oliver@hartkopp.net>
> Cc: Johannes Stezenbach <js@sig21.net>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
> This should fix it but it would be great if it is tested.
> Michal Marek, I think this needs to go upstream pretty quickly.
> 
> 	Sam
> 
>  scripts/Makefile.lib |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index cd815ac..bd201d9 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -213,13 +213,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
>  
>  # Bzip2 and LZMA do not include size in file... so we have to fake that;
>  # append the size as a 32-bit littleendian number as gzip does.
> +# Note: dash built-in printf does not support \x so use /usr/bin version
>  size_append = printf $(shell						\


I thought *this* printf caused the problem?!?


>  dec_size=0;								\
>  for F in $1; do								\
>  	fsize=$$(stat -c "%s" $$F);					\
>  	dec_size=$$(expr $$dec_size + $$fsize);				\
>  done;									\
> -printf "%08x" $$dec_size |						\
> +/usr/bin/printf "%08x" $$dec_size |						\

Maybe this one too ...

Should i test it with this patch anyway??

Regards,
Oliver

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Willy Tarreau]

On Sun, Dec 20, 2009 at 11:03:44AM +0100, Sam Ravnborg wrote:
> We use ... printf \x ... when calculating the size of the
> compressed kernel.
> Unfortunately dash built-in printf does not support this notation
> resulting in a non-bootable kernel.
> 
> Fix this by always using the external version of printf.

Do we really want to workaround shells bugs ? I mean, either
the shell correctly implements the printf function or it does
not at all so that the correct printf is found in the path.
"man printf" clearly states that \x is supported, so what
should be fixed is the shell's implementation of printf.

The more absolute paths we specify, the less portable the
build system. And if linking /bin/sh to whatever shell works
but linking it to dash breaks, it's a shell bug.

Willy

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Michael Tokarev]

Willy Tarreau wrote:
> On Sun, Dec 20, 2009 at 11:03:44AM +0100, Sam Ravnborg wrote:
>> We use ... printf \x ... when calculating the size of the
>> compressed kernel.
>> Unfortunately dash built-in printf does not support this notation
>> resulting in a non-bootable kernel.
>>
>> Fix this by always using the external version of printf.
> 
> Do we really want to workaround shells bugs ? I mean, either

There's no bugs in dash, as far as I can see.  According to
POSIX, a) echo does not need to interpret _any_ escape sequences
at all, and b) printf is not required to interpret \x sequences.
Ref:

http://www.opengroup.org/onlinepubs/000095399/utilities/echo.html
...
  string
    A string to be written to standard output. If the first operand is -n,
    or if any of the operands contain a backslash ( '\' ) character,
   the results are implementation-defined.

    [XSI] On XSI-conformant systems, [..] the following character sequences
    shall be recognized on XSI-conformant systems within any of the arguments:

    \a \b \c \f \n \r \t \v \\
    \0num
        Write an 8-bit value that is the zero, one, two, or three-digit octal
        number num.

http://www.opengroup.org/onlinepubs/000095399/utilities/printf.html

 In addition to the escape sequences shown in the Base Definitions volume of IEEE
 Std 1003.1-2001, Chapter 5, File Format Notation ( '\\', '\a', '\b', '\f', '\n',
 '\r', '\t', '\v' ), "\ddd", where ddd is a one, two, or three-digit octal number,
 shall be written as a byte with the numeric value specified by the octal number.

[]
> The more absolute paths we specify, the less portable the
> build system. And if linking /bin/sh to whatever shell works
> but linking it to dash breaks, it's a shell bug.

Yes for absolute paths, and no for dash, see above.

/mjt

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Willy Tarreau]

On Sun, Dec 20, 2009 at 01:47:13PM +0300, Michael Tokarev wrote:
> Willy Tarreau wrote:
> > On Sun, Dec 20, 2009 at 11:03:44AM +0100, Sam Ravnborg wrote:
> >> We use ... printf \x ... when calculating the size of the
> >> compressed kernel.
> >> Unfortunately dash built-in printf does not support this notation
> >> resulting in a non-bootable kernel.
> >>
> >> Fix this by always using the external version of printf.
> > 
> > Do we really want to workaround shells bugs ? I mean, either
> 
> There's no bugs in dash, as far as I can see.  According to
> POSIX, a) echo does not need to interpret _any_ escape sequences
> at all, and b) printf is not required to interpret \x sequences.

Interesting.

> Ref:
> 
> http://www.opengroup.org/onlinepubs/000095399/utilities/echo.html
> ...
>   string
>     A string to be written to standard output. If the first operand is -n,
>     or if any of the operands contain a backslash ( '\' ) character,
>    the results are implementation-defined.
> 
>     [XSI] On XSI-conformant systems, [..] the following character sequences
>     shall be recognized on XSI-conformant systems within any of the arguments:
> 
>     \a \b \c \f \n \r \t \v \\
>     \0num
>         Write an 8-bit value that is the zero, one, two, or three-digit octal
>         number num.
> 
> http://www.opengroup.org/onlinepubs/000095399/utilities/printf.html
> 
>  In addition to the escape sequences shown in the Base Definitions volume of IEEE
>  Std 1003.1-2001, Chapter 5, File Format Notation ( '\\', '\a', '\b', '\f', '\n',
>  '\r', '\t', '\v' ), "\ddd", where ddd is a one, two, or three-digit octal number,
>  shall be written as a byte with the numeric value specified by the octal number.

OK so the bug will not be fixed by calling /usr/bin/printf. It
will still work by pure luck when a the proper printf utility
will be there, but not when we use a posix-compliant one. The
proper fix then consists in writing octal chars in the form
of "\ddd" instead of "\xhh". And if dash is posix-compliant,
no need for the absolute path.

Willy

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Michael Tokarev]

Sam Ravnborg wrote:
> We use ... printf \x ... when calculating the size of the
> compressed kernel.
> Unfortunately dash built-in printf does not support this notation
> resulting in a non-bootable kernel.
> 
> Fix this by always using the external version of printf.
> 
> The commit that introduced this bug was:
> 4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
> build for x86"

It's not that simple Sam.  This commit is a part of the game.
It works neither with printf as after my commit above, nor with
original fix with `/bin/echo -ne'.  In neither case the fix is
complete or robust.  Because there are two places in the few lines,
around this, both affected and both are non-POSIX-conformant.

> Reported-by: Michael Guntsche <mike@it-loops.com>
> Cc: Oliver Hartkopp <oliver@hartkopp.net>
> Cc: Johannes Stezenbach <js@sig21.net>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
> This should fix it but it would be great if it is tested.
> Michal Marek, I think this needs to go upstream pretty quickly.

This whole issue only affects debian and ubuntu for now.
But the bug is very difficult to track down, since the prob
happens on early boot.

For now, I think, the best is to ensure we're building with
bash not dash.  Ie, SHELL=/bin/bash make ... will work.
Because even with this second fix, it still does not work
correctly with dash's echo and printf behaving differently
than bash's (but still conforming to POSIX).  It produces
good kernel, or at least I don't see any obvious probs with
the resulting kernel, but at least `make V=1' output looks
all wrong in this area (since dash's echo interpret \nnn
by default and make uses echo to print commands as they're
executed).

The right fix is to eliminate all this hack altogether.
Imho anyway.

/mjt

> 	Sam
> 
>  scripts/Makefile.lib |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index cd815ac..bd201d9 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -213,13 +213,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
>  
>  # Bzip2 and LZMA do not include size in file... so we have to fake that;
>  # append the size as a 32-bit littleendian number as gzip does.
> +# Note: dash built-in printf does not support \x so use /usr/bin version
>  size_append = printf $(shell						\
>  dec_size=0;								\
>  for F in $1; do								\
>  	fsize=$$(stat -c "%s" $$F);					\
>  	dec_size=$$(expr $$dec_size + $$fsize);				\
>  done;									\
> -printf "%08x" $$dec_size |						\
> +/usr/bin/printf "%08x" $$dec_size |						\
>  	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
>  )
>  

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Andreas Schwab]

Michael Tokarev <mjt@tls.msk.ru> writes:

> Sam Ravnborg wrote:
>> We use ... printf \x ... when calculating the size of the
>> compressed kernel.
>> Unfortunately dash built-in printf does not support this notation
>> resulting in a non-bootable kernel.
>> 
>> Fix this by always using the external version of printf.

That changed the wrong printf. :-)

>> The commit that introduced this bug was:
>> 4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
>> build for x86"
>
> It's not that simple Sam.  This commit is a part of the game.
> It works neither with printf as after my commit above, nor with
> original fix with `/bin/echo -ne'.  In neither case the fix is
> complete or robust.  Because there are two places in the few lines,
> around this, both affected and both are non-POSIX-conformant.

Does dash's printf support %b?  Then this should work.

Andreas.

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..340813d 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -213,14 +213,16 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
 
 # Bzip2 and LZMA do not include size in file... so we have to fake that;
 # append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell						\
+size_append = printf %b $$(						\
 dec_size=0;								\
 for F in $1; do								\
 	fsize=$$(stat -c "%s" $$F);					\
 	dec_size=$$(expr $$dec_size + $$fsize);				\
 done;									\
-printf "%08x" $$dec_size |						\
-	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
+for i in 1 2 3 4; do							\
+	printf '\\%04o' $$(expr $$dec_size % 256);			\
+  	dec_size=$$(expr $$dec_size / 256);				\
+done									\
 )
 
 quiet_cmd_bzip2 = BZIP2   $@

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Sam Ravnborg]

On Sun, Dec 20, 2009 at 01:02:55PM +0100, Andreas Schwab wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
> > Sam Ravnborg wrote:
> >> We use ... printf \x ... when calculating the size of the
> >> compressed kernel.
> >> Unfortunately dash built-in printf does not support this notation
> >> resulting in a non-bootable kernel.
> >> 
> >> Fix this by always using the external version of printf.
> 
> That changed the wrong printf. :-)

Right - I have it correct in my repo here but "git commit --amend" fooled me.

	Sam

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Michal Marek]

Andreas Schwab napsal(a):
> Does dash's printf support %b?  Then this should work.

Good idea. %b is specified by posix, so there is hope :). Does Andreas'
patch work for others as well? I'd like to fix this upstream ASAP, OTOH
right now it's broken "only" for dash users, so let's not rush and make
it even worse.

Thanks,
Michal
> 
> Andreas.
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index cd815ac..340813d 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -213,14 +213,16 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
>  
>  # Bzip2 and LZMA do not include size in file... so we have to fake that;
>  # append the size as a 32-bit littleendian number as gzip does.
> -size_append = printf $(shell						\
> +size_append = printf %b $$(						\
>  dec_size=0;								\
>  for F in $1; do								\
>  	fsize=$$(stat -c "%s" $$F);					\
>  	dec_size=$$(expr $$dec_size + $$fsize);				\
>  done;									\
> -printf "%08x" $$dec_size |						\
> -	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
> +for i in 1 2 3 4; do							\
> +	printf '\\%04o' $$(expr $$dec_size % 256);			\
> +  	dec_size=$$(expr $$dec_size / 256);				\
> +done									\
>  )

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Andreas Schwab]

Michal Marek <mmarek@suse.cz> writes:

> Andreas Schwab napsal(a):
>> Does dash's printf support %b?  Then this should work.
>
> Good idea. %b is specified by posix, so there is hope :).

Actually you don't need it anyway.  (Still untested though.)

Andreas.

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..7173c0c 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -213,14 +213,16 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
 
 # Bzip2 and LZMA do not include size in file... so we have to fake that;
 # append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell						\
+size_append = printf $$(						\
 dec_size=0;								\
 for F in $1; do								\
 	fsize=$$(stat -c "%s" $$F);					\
 	dec_size=$$(expr $$dec_size + $$fsize);				\
 done;									\
-printf "%08x" $$dec_size |						\
-	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
+for i in 1 2 3 4; do							\
+	printf '\\%03o' $$(expr $$dec_size % 256);			\
+  	dec_size=$$(expr $$dec_size / 256);				\
+done									\
 )
 
 quiet_cmd_bzip2 = BZIP2   $@

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Sam Ravnborg]

On Sun, Dec 20, 2009 at 02:19:41PM +0300, Michael Tokarev wrote:
> Sam Ravnborg wrote:
> > We use ... printf \x ... when calculating the size of the
> > compressed kernel.
> > Unfortunately dash built-in printf does not support this notation
> > resulting in a non-bootable kernel.
> > 
> > Fix this by always using the external version of printf.
> > 
> > The commit that introduced this bug was:
> > 4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
> > build for x86"
> 
> It's not that simple Sam.

The whole business using shell scripts to append the size seems broken.
How about moving this functionality to mkpiggy where we are
less shell script dependent.

Something like the following.
I have only tested it lightly (vmlinux.bin did not differ
before/after the patch.

It includes the length also in the gzip case - I dunno if that matters.
Also I dunno if ".long" is the same on 32 and 64 bit.

	Sam

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index f8ed065..d39fe2e 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -54,7 +54,8 @@ suffix-$(CONFIG_KERNEL_BZIP2)	:= bz2
 suffix-$(CONFIG_KERNEL_LZMA)	:= lzma
 
 quiet_cmd_mkpiggy = MKPIGGY $@
-      cmd_mkpiggy = $(obj)/mkpiggy $< > $@ || ( rm -f $@ ; false )
+      cmd_mkpiggy = $(obj)/mkpiggy $< $(vmlinux.bin.all-y) > $@ || \
+                   ( rm -f $@ ; false )
 
 targets += piggy.S
 $(obj)/piggy.S: $(obj)/vmlinux.bin.$(suffix-y) $(obj)/mkpiggy FORCE
diff --git a/arch/x86/boot/compressed/mkpiggy.c b/arch/x86/boot/compressed/mkpiggy.c
index bcbd36c..f4a1a3f 100644
--- a/arch/x86/boot/compressed/mkpiggy.c
+++ b/arch/x86/boot/compressed/mkpiggy.c
@@ -29,6 +29,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <inttypes.h>
+#include <sys/stat.h>
 
 static uint32_t getle32(const void *p)
 {
@@ -38,12 +39,27 @@ static uint32_t getle32(const void *p)
 		((uint32_t)cp[2] << 16) + ((uint32_t)cp[3] << 24);
 }
 
+static file_size(const char *filename)
+{
+	struct stat statbuf;
+	int res;
+
+	res = stat(filename, &statbuf);
+	if (res < 0) {
+		perror(filename);
+		exit(1);
+	}
+	return (size_t)statbuf.st_size;
+}
+
 int main(int argc, char *argv[])
 {
 	uint32_t olen;
 	long ilen;
 	unsigned long offs;
+	size_t size;
 	FILE *f;
+	int i;
 
 	if (argc < 2) {
 		fprintf(stderr, "Usage: %s compressed_file\n", argv[0]);
@@ -67,6 +83,11 @@ int main(int argc, char *argv[])
 	olen = getle32(&olen);
 	fclose(f);
 
+	i = 2;
+	size = 0;
+	while (i < argc)
+		size += file_size(argv[i++]);
+
 	/*
 	 * Now we have the input (compressed) and output (uncompressed)
 	 * sizes, compute the necessary decompression offset...
@@ -91,6 +112,14 @@ int main(int argc, char *argv[])
 	printf(".globl input_data, input_data_end\n");
 	printf("input_data:\n");
 	printf(".incbin \"%s\"\n", argv[1]);
+
+        /*
+         * Bzip2 and LZMA do not include size in file... so we have to fake that;
+         * append the size as a 32-bit littleendian number as gzip does.
+         */
+	if (size > 0)
+		printf(".long %d\n", size);
+
 	printf("input_data_end:\n");
 
 	return 0;
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..10bef1c 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -211,27 +211,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
 # Bzip2
 # ---------------------------------------------------------------------------
 
-# Bzip2 and LZMA do not include size in file... so we have to fake that;
-# append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell						\
-dec_size=0;								\
-for F in $1; do								\
-	fsize=$$(stat -c "%s" $$F);					\
-	dec_size=$$(expr $$dec_size + $$fsize);				\
-done;									\
-printf "%08x" $$dec_size |						\
-	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
-)
-
 quiet_cmd_bzip2 = BZIP2   $@
-cmd_bzip2 = (cat $(filter-out FORCE,$^) | \
-	bzip2 -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
-	(rm -f $@ ; false)
+cmd_bzip2 = cat $(filter-out FORCE,$^) | \
+	bzip2 -9 > $@ || (rm -f $@ ; false)
 
 # Lzma
 # ---------------------------------------------------------------------------
 
 quiet_cmd_lzma = LZMA    $@
-cmd_lzma = (cat $(filter-out FORCE,$^) | \
-	lzma -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
-	(rm -f $@ ; false)
+cmd_lzma = cat $(filter-out FORCE,$^) | \
+	lzma -9 > $@ || (rm -f $@ ; false)
+

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Michal Marek]

Sam Ravnborg wrote:
> The whole business using shell scripts to append the size seems broken.
> How about moving this functionality to mkpiggy where we are
> less shell script dependent.
> 
> Something like the following.
> I have only tested it lightly (vmlinux.bin did not differ
> before/after the patch.


I think that it's a good idea to move this out of the makefiles. But for
2.6.33, I think I prefer the patch from Andreas, provided he signs it
off and Michael G confirms that it fixes his issue.


> It includes the length also in the gzip case - I dunno if that matters.

I don't know either, but this is trivially fixable by an option to
mkpiggy that is added only if CONFIG_KERNEL_BZIP2=y or CONFIG_KERNEL_LZMA=y.


> Also I dunno if ".long" is the same on 32 and 64 bit.

I think we can work this out until the .34 merge window :).

Michal
> 
> 	Sam
> 
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index f8ed065..d39fe2e 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -54,7 +54,8 @@ suffix-$(CONFIG_KERNEL_BZIP2)	:= bz2
>  suffix-$(CONFIG_KERNEL_LZMA)	:= lzma
>  
>  quiet_cmd_mkpiggy = MKPIGGY $@
> -      cmd_mkpiggy = $(obj)/mkpiggy $< > $@ || ( rm -f $@ ; false )
> +      cmd_mkpiggy = $(obj)/mkpiggy $< $(vmlinux.bin.all-y) > $@ || \
> +                   ( rm -f $@ ; false )
>  
>  targets += piggy.S
>  $(obj)/piggy.S: $(obj)/vmlinux.bin.$(suffix-y) $(obj)/mkpiggy FORCE
> diff --git a/arch/x86/boot/compressed/mkpiggy.c b/arch/x86/boot/compressed/mkpiggy.c
> index bcbd36c..f4a1a3f 100644
> --- a/arch/x86/boot/compressed/mkpiggy.c
> +++ b/arch/x86/boot/compressed/mkpiggy.c
> @@ -29,6 +29,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <inttypes.h>
> +#include <sys/stat.h>
>  
>  static uint32_t getle32(const void *p)
>  {
> @@ -38,12 +39,27 @@ static uint32_t getle32(const void *p)
>  		((uint32_t)cp[2] << 16) + ((uint32_t)cp[3] << 24);
>  }
>  
> +static file_size(const char *filename)
> +{
> +	struct stat statbuf;
> +	int res;
> +
> +	res = stat(filename, &statbuf);
> +	if (res < 0) {
> +		perror(filename);
> +		exit(1);
> +	}
> +	return (size_t)statbuf.st_size;
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	uint32_t olen;
>  	long ilen;
>  	unsigned long offs;
> +	size_t size;
>  	FILE *f;
> +	int i;
>  
>  	if (argc < 2) {
>  		fprintf(stderr, "Usage: %s compressed_file\n", argv[0]);
> @@ -67,6 +83,11 @@ int main(int argc, char *argv[])
>  	olen = getle32(&olen);
>  	fclose(f);
>  
> +	i = 2;
> +	size = 0;
> +	while (i < argc)
> +		size += file_size(argv[i++]);
> +
>  	/*
>  	 * Now we have the input (compressed) and output (uncompressed)
>  	 * sizes, compute the necessary decompression offset...
> @@ -91,6 +112,14 @@ int main(int argc, char *argv[])
>  	printf(".globl input_data, input_data_end\n");
>  	printf("input_data:\n");
>  	printf(".incbin \"%s\"\n", argv[1]);
> +
> +        /*
> +         * Bzip2 and LZMA do not include size in file... so we have to fake that;
> +         * append the size as a 32-bit littleendian number as gzip does.
> +         */
> +	if (size > 0)
> +		printf(".long %d\n", size);
> +
>  	printf("input_data_end:\n");
>  
>  	return 0;
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index cd815ac..10bef1c 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -211,27 +211,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
>  # Bzip2
>  # ---------------------------------------------------------------------------
>  
> -# Bzip2 and LZMA do not include size in file... so we have to fake that;
> -# append the size as a 32-bit littleendian number as gzip does.
> -size_append = printf $(shell						\
> -dec_size=0;								\
> -for F in $1; do								\
> -	fsize=$$(stat -c "%s" $$F);					\
> -	dec_size=$$(expr $$dec_size + $$fsize);				\
> -done;									\
> -printf "%08x" $$dec_size |						\
> -	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
> -)
> -
>  quiet_cmd_bzip2 = BZIP2   $@
> -cmd_bzip2 = (cat $(filter-out FORCE,$^) | \
> -	bzip2 -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> -	(rm -f $@ ; false)
> +cmd_bzip2 = cat $(filter-out FORCE,$^) | \
> +	bzip2 -9 > $@ || (rm -f $@ ; false)
>  
>  # Lzma
>  # ---------------------------------------------------------------------------
>  
>  quiet_cmd_lzma = LZMA    $@
> -cmd_lzma = (cat $(filter-out FORCE,$^) | \
> -	lzma -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> -	(rm -f $@ ; false)
> +cmd_lzma = cat $(filter-out FORCE,$^) | \
> +	lzma -9 > $@ || (rm -f $@ ; false)
> +

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Michael Guntsche]

On 21 Dec 09 15:17, Michal Marek wrote:
> > How about moving this functionality to mkpiggy where we are
> > less shell script dependent.
> > 
> > Something like the following.
> > I have only tested it lightly (vmlinux.bin did not differ
> > before/after the patch.
> 
> 
> I think that it's a good idea to move this out of the makefiles. But for
> 2.6.33, I think I prefer the patch from Andreas, provided he signs it
> off and Michael G confirms that it fixes his issue.

Just a quick update from my side. I tried the patch from Andreas and build a kernel
with dash as defaulti shell. I was able to boot the resulting kernel image
without any problems.

Kind regards,
Michael

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Michael Tokarev]

Michael Guntsche wrote:
> On 21 Dec 09 15:17, Michal Marek wrote:
>>> How about moving this functionality to mkpiggy where we are
>>> less shell script dependent.
>>>
>>> Something like the following.
>>> I have only tested it lightly (vmlinux.bin did not differ
>>> before/after the patch.
>>
>> I think that it's a good idea to move this out of the makefiles. But for
>> 2.6.33, I think I prefer the patch from Andreas, provided he signs it
>> off and Michael G confirms that it fixes his issue.
> 
> Just a quick update from my side. I tried the patch from Andreas and build a kernel
> with dash as defaulti shell. I was able to boot the resulting kernel image
> without any problems.

That's not a good test case.  Because even with bogus data the kernel might
boot: for example, allocating alot more memory for the unpacking than necessary -
it does not crash but the data is still bogus.

The better (and much faster) check is to verify that the z_output_len value
in arch/x86/boot/compressed/piggy.S matches the size of
arch/x86/boot/compressed/vmlinuz.bin.

/mjt

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Michael Guntsche]

On 2009.12.21 22:53:54 , Michael Tokarev wrote:
> Michael Guntsche wrote:
> > Just a quick update from my side. I tried the patch from Andreas and build a kernel
> > with dash as defaulti shell. I was able to boot the resulting kernel image
> > without any problems.
> 
> That's not a good test case.  Because even with bogus data the kernel might
> boot: for example, allocating alot more memory for the unpacking than necessary -
> it does not crash but the data is still bogus.
> 
> The better (and much faster) check is to verify that the z_output_len value
> in arch/x86/boot/compressed/piggy.S matches the size of
> arch/x86/boot/compressed/vmlinuz.bin.

Looks ok for me.

piggy.S:
z_output_len = 3550876

ls -la vmlinux.bin:
-rwxr-xr-x 1 root root 3550876 Dec 21 17:21 vmlinux.bin


Kind regards,
Michael

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[H. Peter Anvin]

On 12/20/2009 06:11 AM, Sam Ravnborg wrote:
> 
> The whole business using shell scripts to append the size seems broken.
> How about moving this functionality to mkpiggy where we are
> less shell script dependent.
> 

Seems a helluva lot saner to me...

> It includes the length also in the gzip case - I dunno if that matters.
> Also I dunno if ".long" is the same on 32 and 64 bit.

Well, gzip already has a length at the end... shouldn't really hurt
anything, I suppose.  ".long" is 32 bits on both 32 and 64 bits.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[H. Peter Anvin]

On 12/21/2009 10:28 AM, H. Peter Anvin wrote:
> On 12/20/2009 06:11 AM, Sam Ravnborg wrote:
>>
>> The whole business using shell scripts to append the size seems broken.
>> How about moving this functionality to mkpiggy where we are
>> less shell script dependent.
>>
> 
> Seems a helluva lot saner to me...
> 
>> It includes the length also in the gzip case - I dunno if that matters.
>> Also I dunno if ".long" is the same on 32 and 64 bit.
> 
> Well, gzip already has a length at the end... shouldn't really hurt
> anything, I suppose.  ".long" is 32 bits on both 32 and 64 bits.
> 

Note, though: mkpiggy is x86, but this also affects other
architectures... so probably additional changes are needed.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

[Arkadiusz Miskiewicz]

On Sunday 20 of December 2009, Sam Ravnborg wrote:
			\
> -printf "%08x" $$dec_size |						\
> +/usr/bin/printf "%08x" $$dec_size |						\
>  	sed 's/\(..\)\(..\)\(..\)\(..\)/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g'	\
>  )
> 

Here printf is in /bin/ and not in /usr/bin.

-- 
Arkadiusz Miśkiewicz        PLD/Linux Team
arekm / maven.pl            http://ftp.pld-linux.org/