2022-01-09 18:16:25

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 1/5] sh: rename suffix-y to suffix_y

'export suffix-y' does not work reliably because hyphens are disallowed
in shell variables.

A similar issue was fixed by commit 2bfbe7881ee0 ("kbuild: Do not use
hyphen in exported variable name").

If I do similar in dash, ARCH=sh fails to build.

$ mv linux linux~
$ cd linux~
$ dash
$ make O=foo/bar ARCH=sh CROSS_COMPILE=sh4-linux-gnu- defconfig all
make[1]: Entering directory '/home/masahiro/linux~/foo/bar'
[ snip ]
make[4]: *** No rule to make target 'arch/sh/boot/compressed/vmlinux.bin.', needed by 'arch/sh/boot/compressed/piggy.o'. Stop.
make[3]: *** [/home/masahiro/linux~/arch/sh/boot/Makefile:40: arch/sh/boot/compressed/vmlinux] Error 2
make[2]: *** [/home/masahiro/linux~/arch/sh/Makefile:194: zImage] Error 2
make[1]: *** [/home/masahiro/linux~/Makefile:350: __build_one_by_one] Error 2
make[1]: Leaving directory '/home/masahiro/linux~/foo/bar'
make: *** [Makefile:219: __sub-make] Error 2

The maintainer of GNU Make stated that there is no consistent way to
export variables that do not meet the shell's naming criteria.
(https://savannah.gnu.org/bugs/?55719)

Consequently, you cannot use hyphens in exported variables.

Signed-off-by: Masahiro Yamada <[email protected]>
---

arch/sh/boot/Makefile | 16 ++++++++--------
arch/sh/boot/compressed/Makefile | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/sh/boot/Makefile b/arch/sh/boot/Makefile
index 5c123f5b2797..1f5d2df3c7e0 100644
--- a/arch/sh/boot/Makefile
+++ b/arch/sh/boot/Makefile
@@ -19,12 +19,12 @@ CONFIG_ZERO_PAGE_OFFSET ?= 0x00001000
CONFIG_ENTRY_OFFSET ?= 0x00001000
CONFIG_PHYSICAL_START ?= $(CONFIG_MEMORY_START)

-suffix-y := bin
-suffix-$(CONFIG_KERNEL_GZIP) := gz
-suffix-$(CONFIG_KERNEL_BZIP2) := bz2
-suffix-$(CONFIG_KERNEL_LZMA) := lzma
-suffix-$(CONFIG_KERNEL_XZ) := xz
-suffix-$(CONFIG_KERNEL_LZO) := lzo
+suffix_y := bin
+suffix_$(CONFIG_KERNEL_GZIP) := gz
+suffix_$(CONFIG_KERNEL_BZIP2) := bz2
+suffix_$(CONFIG_KERNEL_LZMA) := lzma
+suffix_$(CONFIG_KERNEL_XZ) := xz
+suffix_$(CONFIG_KERNEL_LZO) := lzo

targets := zImage vmlinux.srec romImage uImage uImage.srec uImage.gz \
uImage.bz2 uImage.lzma uImage.xz uImage.lzo uImage.bin \
@@ -106,10 +106,10 @@ OBJCOPYFLAGS_uImage.srec := -I binary -O srec
$(obj)/uImage.srec: $(obj)/uImage FORCE
$(call if_changed,objcopy)

-$(obj)/uImage: $(obj)/uImage.$(suffix-y)
+$(obj)/uImage: $(obj)/uImage.$(suffix_y)
@ln -sf $(notdir $<) $@
@echo ' Image $@ is ready'

export CONFIG_PAGE_OFFSET CONFIG_MEMORY_START CONFIG_BOOT_LINK_OFFSET \
CONFIG_PHYSICAL_START CONFIG_ZERO_PAGE_OFFSET CONFIG_ENTRY_OFFSET \
- KERNEL_MEMORY suffix-y
+ KERNEL_MEMORY suffix_y
diff --git a/arch/sh/boot/compressed/Makefile b/arch/sh/boot/compressed/Makefile
index cf3174df7859..c1eb9a62de55 100644
--- a/arch/sh/boot/compressed/Makefile
+++ b/arch/sh/boot/compressed/Makefile
@@ -64,5 +64,5 @@ OBJCOPYFLAGS += -R .empty_zero_page

LDFLAGS_piggy.o := -r --format binary --oformat $(ld-bfd) -T

-$(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.$(suffix-y) FORCE
+$(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.$(suffix_y) FORCE
$(call if_changed,ld)
--
2.32.0



2022-01-09 18:16:25

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

GZIP-compressed files end with 4 byte data that represents the size
of the original input. The decompressors (the self-extracting kernel)
exploit it to know the vmlinux size beforehand. To mimic the GZIP's
trailer, Kbuild provides cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}.
Unfortunately these macros are used everywhere despite the appended
size data is only useful for the decompressors.

There is no guarantee that such hand-crafted trailers are safely ignored.
In fact, the kernel refuses compressed initramdisks with the garbage
data. That is why usr/Makefile overrides size_append to make it no-op.

To limit the use of such broken compressed files, this commit renames
the existing macros as follows:

cmd_bzip2 --> cmd_bzip2_with_size
cmd_lzma --> cmd_lzma_with_size
cmd_lzo --> cmd_lzo_with_size
cmd_lz4 --> cmd_lz4_with_size
cmd_xzkern --> cmd_xzkern_with_size
cmd_zstd22 --> cmd_zstd22_with_size

To keep the decompressors working, I updated the following Makefiles
accordingly:

arch/arm/boot/compressed/Makefile
arch/h8300/boot/compressed/Makefile
arch/mips/boot/compressed/Makefile
arch/parisc/boot/compressed/Makefile
arch/s390/boot/compressed/Makefile
arch/sh/boot/compressed/Makefile
arch/x86/boot/compressed/Makefile

I reused the current macro names for the normal usecases; they produce
the compressed data in the proper format.

I did not touch the following:

arch/arc/boot/Makefile
arch/arm64/boot/Makefile
arch/csky/boot/Makefile
arch/mips/boot/Makefile
arch/riscv/boot/Makefile
arch/sh/boot/Makefile
kernel/Makefile

This means those Makefiles will stop appending the size data.

I dropped the 'override size_append' hack from usr/Makefile.

Signed-off-by: Masahiro Yamada <[email protected]>
---

arch/arm/boot/compressed/Makefile | 8 ++++----
arch/h8300/boot/compressed/Makefile | 4 +++-
arch/mips/boot/compressed/Makefile | 12 +++++------
arch/parisc/boot/compressed/Makefile | 10 +++++-----
arch/s390/boot/compressed/Makefile | 12 +++++------
arch/sh/boot/compressed/Makefile | 8 ++++----
arch/x86/boot/compressed/Makefile | 12 +++++------
scripts/Makefile.lib | 30 ++++++++++++++++++++++------
usr/Makefile | 5 -----
9 files changed, 58 insertions(+), 43 deletions(-)

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index 91265e7ff672..adc0e318a1ea 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -77,10 +77,10 @@ CPPFLAGS_vmlinux.lds += -DTEXT_OFFSET="$(TEXT_OFFSET)"
CPPFLAGS_vmlinux.lds += -DMALLOC_SIZE="$(MALLOC_SIZE)"

compress-$(CONFIG_KERNEL_GZIP) = gzip
-compress-$(CONFIG_KERNEL_LZO) = lzo
-compress-$(CONFIG_KERNEL_LZMA) = lzma
-compress-$(CONFIG_KERNEL_XZ) = xzkern
-compress-$(CONFIG_KERNEL_LZ4) = lz4
+compress-$(CONFIG_KERNEL_LZO) = lzo_with_size
+compress-$(CONFIG_KERNEL_LZMA) = lzma_with_size
+compress-$(CONFIG_KERNEL_XZ) = xzkern_with_size
+compress-$(CONFIG_KERNEL_LZ4) = lz4_with_size

libfdt_objs := fdt_rw.o fdt_ro.o fdt_wip.o fdt.o

diff --git a/arch/h8300/boot/compressed/Makefile b/arch/h8300/boot/compressed/Makefile
index 5942793f77a0..6ab2fa5ba105 100644
--- a/arch/h8300/boot/compressed/Makefile
+++ b/arch/h8300/boot/compressed/Makefile
@@ -30,9 +30,11 @@ $(obj)/vmlinux.bin: vmlinux FORCE

suffix-$(CONFIG_KERNEL_GZIP) := gzip
suffix-$(CONFIG_KERNEL_LZO) := lzo
+compress-$(CONFIG_KERNEL_GZIP) := gzip
+compress-$(CONFIG_KERNEL_LZO) := lzo_with_size

$(obj)/vmlinux.bin.$(suffix-y): $(obj)/vmlinux.bin FORCE
- $(call if_changed,$(suffix-y))
+ $(call if_changed,$(compress-y))

LDFLAGS_piggy.o := -r --format binary --oformat elf32-h8300-linux -T
OBJCOPYFLAGS := -O binary
diff --git a/arch/mips/boot/compressed/Makefile b/arch/mips/boot/compressed/Makefile
index f27cf31b4140..832f8001d7d9 100644
--- a/arch/mips/boot/compressed/Makefile
+++ b/arch/mips/boot/compressed/Makefile
@@ -64,12 +64,12 @@ $(obj)/vmlinux.bin: $(KBUILD_IMAGE) FORCE
$(call if_changed,objcopy)

tool_$(CONFIG_KERNEL_GZIP) = gzip
-tool_$(CONFIG_KERNEL_BZIP2) = bzip2
-tool_$(CONFIG_KERNEL_LZ4) = lz4
-tool_$(CONFIG_KERNEL_LZMA) = lzma
-tool_$(CONFIG_KERNEL_LZO) = lzo
-tool_$(CONFIG_KERNEL_XZ) = xzkern
-tool_$(CONFIG_KERNEL_ZSTD) = zstd22
+tool_$(CONFIG_KERNEL_BZIP2) = bzip2_with_size
+tool_$(CONFIG_KERNEL_LZ4) = lz4_with_size
+tool_$(CONFIG_KERNEL_LZMA) = lzma_with_size
+tool_$(CONFIG_KERNEL_LZO) = lzo_with_size
+tool_$(CONFIG_KERNEL_XZ) = xzkern_with_size
+tool_$(CONFIG_KERNEL_ZSTD) = zstd22_with_size

targets += vmlinux.bin.z

diff --git a/arch/parisc/boot/compressed/Makefile b/arch/parisc/boot/compressed/Makefile
index bf4f2891d0b7..2640f72d69ce 100644
--- a/arch/parisc/boot/compressed/Makefile
+++ b/arch/parisc/boot/compressed/Makefile
@@ -70,15 +70,15 @@ suffix-$(CONFIG_KERNEL_XZ) := xz
$(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
$(call if_changed,gzip)
$(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,bzip2)
+ $(call if_changed,bzip2_with_size)
$(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lz4)
+ $(call if_changed,lz4_with_size)
$(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzma)
+ $(call if_changed,lzma_with_size)
$(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzo)
+ $(call if_changed,lzo_with_size)
$(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,xzkern)
+ $(call if_changed,xzkern_with_size)

LDFLAGS_piggy.o := -r --format binary --oformat $(LD_BFD) -T
$(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.$(suffix-y) FORCE
diff --git a/arch/s390/boot/compressed/Makefile b/arch/s390/boot/compressed/Makefile
index 3b860061e84d..8ea880b7c3ec 100644
--- a/arch/s390/boot/compressed/Makefile
+++ b/arch/s390/boot/compressed/Makefile
@@ -71,17 +71,17 @@ suffix-$(CONFIG_KERNEL_ZSTD) := .zst
$(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
$(call if_changed,gzip)
$(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,bzip2)
+ $(call if_changed,bzip2_with_size)
$(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lz4)
+ $(call if_changed,lz4_with_size)
$(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzma)
+ $(call if_changed,lzma_with_size)
$(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzo)
+ $(call if_changed,lzo_with_size)
$(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,xzkern)
+ $(call if_changed,xzkern_with_size)
$(obj)/vmlinux.bin.zst: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,zstd22)
+ $(call if_changed,zstd22_with_size)

OBJCOPYFLAGS_piggy.o := -I binary -O elf64-s390 -B s390:64-bit --rename-section .data=.vmlinux.bin.compressed
$(obj)/piggy.o: $(obj)/vmlinux.bin$(suffix-y) FORCE
diff --git a/arch/sh/boot/compressed/Makefile b/arch/sh/boot/compressed/Makefile
index c1eb9a62de55..a6808a403f4b 100644
--- a/arch/sh/boot/compressed/Makefile
+++ b/arch/sh/boot/compressed/Makefile
@@ -52,13 +52,13 @@ vmlinux.bin.all-y := $(obj)/vmlinux.bin
$(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
$(call if_changed,gzip)
$(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,bzip2)
+ $(call if_changed,bzip2_with_size)
$(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzma)
+ $(call if_changed,lzma_with_size)
$(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,xzkern)
+ $(call if_changed,xzkern_with_size)
$(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzo)
+ $(call if_changed,lzo_with_size)

OBJCOPYFLAGS += -R .empty_zero_page

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 431bf7f846c3..2825c74bcae3 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -123,17 +123,17 @@ vmlinux.bin.all-$(CONFIG_X86_NEED_RELOCS) += $(obj)/vmlinux.relocs
$(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
$(call if_changed,gzip)
$(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,bzip2)
+ $(call if_changed,bzip2_with_size)
$(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzma)
+ $(call if_changed,lzma_with_size)
$(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,xzkern)
+ $(call if_changed,xzkern_with_size)
$(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lzo)
+ $(call if_changed,lzo_with_size)
$(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,lz4)
+ $(call if_changed,lz4_with_size)
$(obj)/vmlinux.bin.zst: $(vmlinux.bin.all-y) FORCE
- $(call if_changed,zstd22)
+ $(call if_changed,zstd22_with_size)

suffix-$(CONFIG_KERNEL_GZIP) := gz
suffix-$(CONFIG_KERNEL_BZIP2) := bz2
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 5366466ea0e4..4207a72d429f 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -395,19 +395,31 @@ printf "%08x\n" $$dec_size | \
)

quiet_cmd_bzip2 = BZIP2 $@
- cmd_bzip2 = { cat $(real-prereqs) | $(KBZIP2) -9; $(size_append); } > $@
+ cmd_bzip2 = cat $(real-prereqs) | $(KBZIP2) -9 > $@
+
+quiet_cmd_bzip2_with_size = BZIP2 $@
+ cmd_bzip2_with_size = { cat $(real-prereqs) | $(KBZIP2) -9; $(size_append); } > $@

# Lzma
# ---------------------------------------------------------------------------

quiet_cmd_lzma = LZMA $@
- cmd_lzma = { cat $(real-prereqs) | $(LZMA) -9; $(size_append); } > $@
+ cmd_lzma = cat $(real-prereqs) | $(LZMA) -9 > $@
+
+quiet_cmd_lzma_with_size = LZMA $@
+ cmd_lzma_with_size = { cat $(real-prereqs) | $(LZMA) -9; $(size_append); } > $@

quiet_cmd_lzo = LZO $@
- cmd_lzo = { cat $(real-prereqs) | $(KLZOP) -9; $(size_append); } > $@
+ cmd_lzo = cat $(real-prereqs) | $(KLZOP) -9 > $@
+
+quiet_cmd_lzo_with_size = LZO $@
+ cmd_lzo_with_size = { cat $(real-prereqs) | $(KLZOP) -9; $(size_append); } > $@

quiet_cmd_lz4 = LZ4 $@
- cmd_lz4 = { cat $(real-prereqs) | $(LZ4) -l -c1 stdin stdout; \
+ cmd_lz4 = cat $(real-prereqs) | $(LZ4) -l -c1 stdin stdout > $@
+
+quiet_cmd_lz4_with_size = LZ4 $@
+ cmd_lz4_with_size = { cat $(real-prereqs) | $(LZ4) -l -c1 stdin stdout; \
$(size_append); } > $@

# U-Boot mkimage
@@ -450,7 +462,10 @@ quiet_cmd_uimage = UIMAGE $@
# big dictionary would increase the memory usage too much in the multi-call
# decompression mode. A BCJ filter isn't used either.
quiet_cmd_xzkern = XZKERN $@
- cmd_xzkern = { cat $(real-prereqs) | sh $(srctree)/scripts/xz_wrap.sh; \
+ cmd_xzkern = cat $(real-prereqs) | sh $(srctree)/scripts/xz_wrap.sh > $@
+
+quiet_cmd_xzkern_with_size = XZKERN $@
+ cmd_xzkern_with_size = { cat $(real-prereqs) | sh $(srctree)/scripts/xz_wrap.sh; \
$(size_append); } > $@

quiet_cmd_xzmisc = XZMISC $@
@@ -476,7 +491,10 @@ quiet_cmd_zstd = ZSTD $@
cmd_zstd = cat $(real-prereqs) | $(ZSTD) -19 > $@

quiet_cmd_zstd22 = ZSTD22 $@
- cmd_zstd22 = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
+ cmd_zstd22 = cat $(real-prereqs) | $(ZSTD) -22 --ultra > $@
+
+quiet_cmd_zstd22_with_size = ZSTD22 $@
+ cmd_zstd22_with_size = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@

# ASM offsets
# ---------------------------------------------------------------------------
diff --git a/usr/Makefile b/usr/Makefile
index b1a81a40eab1..7b89c0175a3a 100644
--- a/usr/Makefile
+++ b/usr/Makefile
@@ -3,11 +3,6 @@
# kbuild file for usr/ - including initramfs image
#

-# cmd_bzip2, cmd_lzma, cmd_lzo, cmd_lz4 from scripts/Makefile.lib appends the
-# size at the end of the compressed file, which unfortunately does not work
-# with unpack_to_rootfs(). Make size_append no-op.
-override size_append := :
-
compress-y := shipped
compress-$(CONFIG_INITRAMFS_COMPRESSION_GZIP) := gzip
compress-$(CONFIG_INITRAMFS_COMPRESSION_BZIP2) := bzip2
--
2.32.0


2022-01-10 11:32:24

by Nicolas Schier

[permalink] [raw]
Subject: Re: [PATCH 1/5] sh: rename suffix-y to suffix_y

On Mon, Jan 10, 2022 at 03:15:25AM +0900, Masahiro Yamada wrote:
> 'export suffix-y' does not work reliably because hyphens are disallowed
> in shell variables.
>
> A similar issue was fixed by commit 2bfbe7881ee0 ("kbuild: Do not use
> hyphen in exported variable name").
>
> If I do similar in dash, ARCH=sh fails to build.
>
> $ mv linux linux~
> $ cd linux~
> $ dash
> $ make O=foo/bar ARCH=sh CROSS_COMPILE=sh4-linux-gnu- defconfig all
> make[1]: Entering directory '/home/masahiro/linux~/foo/bar'
> [ snip ]
> make[4]: *** No rule to make target 'arch/sh/boot/compressed/vmlinux.bin.', needed by 'arch/sh/boot/compressed/piggy.o'. Stop.
> make[3]: *** [/home/masahiro/linux~/arch/sh/boot/Makefile:40: arch/sh/boot/compressed/vmlinux] Error 2
> make[2]: *** [/home/masahiro/linux~/arch/sh/Makefile:194: zImage] Error 2
> make[1]: *** [/home/masahiro/linux~/Makefile:350: __build_one_by_one] Error 2
> make[1]: Leaving directory '/home/masahiro/linux~/foo/bar'
> make: *** [Makefile:219: __sub-make] Error 2
>
> The maintainer of GNU Make stated that there is no consistent way to
> export variables that do not meet the shell's naming criteria.
> (https://savannah.gnu.org/bugs/?55719)
>
> Consequently, you cannot use hyphens in exported variables.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---

Reviewed-by: Nicolas Schier <[email protected]>

>
> arch/sh/boot/Makefile | 16 ++++++++--------
> arch/sh/boot/compressed/Makefile | 2 +-
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/sh/boot/Makefile b/arch/sh/boot/Makefile
> index 5c123f5b2797..1f5d2df3c7e0 100644
> --- a/arch/sh/boot/Makefile
> +++ b/arch/sh/boot/Makefile
> @@ -19,12 +19,12 @@ CONFIG_ZERO_PAGE_OFFSET ?= 0x00001000
> CONFIG_ENTRY_OFFSET ?= 0x00001000
> CONFIG_PHYSICAL_START ?= $(CONFIG_MEMORY_START)
>
> -suffix-y := bin
> -suffix-$(CONFIG_KERNEL_GZIP) := gz
> -suffix-$(CONFIG_KERNEL_BZIP2) := bz2
> -suffix-$(CONFIG_KERNEL_LZMA) := lzma
> -suffix-$(CONFIG_KERNEL_XZ) := xz
> -suffix-$(CONFIG_KERNEL_LZO) := lzo
> +suffix_y := bin
> +suffix_$(CONFIG_KERNEL_GZIP) := gz
> +suffix_$(CONFIG_KERNEL_BZIP2) := bz2
> +suffix_$(CONFIG_KERNEL_LZMA) := lzma
> +suffix_$(CONFIG_KERNEL_XZ) := xz
> +suffix_$(CONFIG_KERNEL_LZO) := lzo
>
> targets := zImage vmlinux.srec romImage uImage uImage.srec uImage.gz \
> uImage.bz2 uImage.lzma uImage.xz uImage.lzo uImage.bin \
> @@ -106,10 +106,10 @@ OBJCOPYFLAGS_uImage.srec := -I binary -O srec
> $(obj)/uImage.srec: $(obj)/uImage FORCE
> $(call if_changed,objcopy)
>
> -$(obj)/uImage: $(obj)/uImage.$(suffix-y)
> +$(obj)/uImage: $(obj)/uImage.$(suffix_y)
> @ln -sf $(notdir $<) $@
> @echo ' Image $@ is ready'
>
> export CONFIG_PAGE_OFFSET CONFIG_MEMORY_START CONFIG_BOOT_LINK_OFFSET \
> CONFIG_PHYSICAL_START CONFIG_ZERO_PAGE_OFFSET CONFIG_ENTRY_OFFSET \
> - KERNEL_MEMORY suffix-y
> + KERNEL_MEMORY suffix_y
> diff --git a/arch/sh/boot/compressed/Makefile b/arch/sh/boot/compressed/Makefile
> index cf3174df7859..c1eb9a62de55 100644
> --- a/arch/sh/boot/compressed/Makefile
> +++ b/arch/sh/boot/compressed/Makefile
> @@ -64,5 +64,5 @@ OBJCOPYFLAGS += -R .empty_zero_page
>
> LDFLAGS_piggy.o := -r --format binary --oformat $(ld-bfd) -T
>
> -$(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.$(suffix-y) FORCE
> +$(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.$(suffix_y) FORCE
> $(call if_changed,ld)
> --
> 2.32.0
>

2022-01-10 11:35:09

by Nicolas Schier

[permalink] [raw]
Subject: Re: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

On Mon, Jan 10, 2022 at 03:15:27AM +0900, Masahiro Yamada wrote:
> GZIP-compressed files end with 4 byte data that represents the size
> of the original input. The decompressors (the self-extracting kernel)
> exploit it to know the vmlinux size beforehand. To mimic the GZIP's
> trailer, Kbuild provides cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}.
> Unfortunately these macros are used everywhere despite the appended
> size data is only useful for the decompressors.
>
> There is no guarantee that such hand-crafted trailers are safely ignored.
> In fact, the kernel refuses compressed initramdisks with the garbage
> data. That is why usr/Makefile overrides size_append to make it no-op.
>
> To limit the use of such broken compressed files, this commit renames
> the existing macros as follows:
>
> cmd_bzip2 --> cmd_bzip2_with_size
> cmd_lzma --> cmd_lzma_with_size
> cmd_lzo --> cmd_lzo_with_size
> cmd_lz4 --> cmd_lz4_with_size
> cmd_xzkern --> cmd_xzkern_with_size
> cmd_zstd22 --> cmd_zstd22_with_size
>
> To keep the decompressors working, I updated the following Makefiles
> accordingly:
>
> arch/arm/boot/compressed/Makefile
> arch/h8300/boot/compressed/Makefile
> arch/mips/boot/compressed/Makefile
> arch/parisc/boot/compressed/Makefile
> arch/s390/boot/compressed/Makefile
> arch/sh/boot/compressed/Makefile
> arch/x86/boot/compressed/Makefile
>
> I reused the current macro names for the normal usecases; they produce
> the compressed data in the proper format.
>
> I did not touch the following:
>
> arch/arc/boot/Makefile
> arch/arm64/boot/Makefile
> arch/csky/boot/Makefile
> arch/mips/boot/Makefile
> arch/riscv/boot/Makefile
> arch/sh/boot/Makefile
> kernel/Makefile
>
> This means those Makefiles will stop appending the size data.
>
> I dropped the 'override size_append' hack from usr/Makefile.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---

Reviewed-by: Nicolas Schier <[email protected]>

>
> arch/arm/boot/compressed/Makefile | 8 ++++----
> arch/h8300/boot/compressed/Makefile | 4 +++-
> arch/mips/boot/compressed/Makefile | 12 +++++------
> arch/parisc/boot/compressed/Makefile | 10 +++++-----
> arch/s390/boot/compressed/Makefile | 12 +++++------
> arch/sh/boot/compressed/Makefile | 8 ++++----
> arch/x86/boot/compressed/Makefile | 12 +++++------
> scripts/Makefile.lib | 30 ++++++++++++++++++++++------
> usr/Makefile | 5 -----
> 9 files changed, 58 insertions(+), 43 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index 91265e7ff672..adc0e318a1ea 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -77,10 +77,10 @@ CPPFLAGS_vmlinux.lds += -DTEXT_OFFSET="$(TEXT_OFFSET)"
> CPPFLAGS_vmlinux.lds += -DMALLOC_SIZE="$(MALLOC_SIZE)"
>
> compress-$(CONFIG_KERNEL_GZIP) = gzip
> -compress-$(CONFIG_KERNEL_LZO) = lzo
> -compress-$(CONFIG_KERNEL_LZMA) = lzma
> -compress-$(CONFIG_KERNEL_XZ) = xzkern
> -compress-$(CONFIG_KERNEL_LZ4) = lz4
> +compress-$(CONFIG_KERNEL_LZO) = lzo_with_size
> +compress-$(CONFIG_KERNEL_LZMA) = lzma_with_size
> +compress-$(CONFIG_KERNEL_XZ) = xzkern_with_size
> +compress-$(CONFIG_KERNEL_LZ4) = lz4_with_size
>
> libfdt_objs := fdt_rw.o fdt_ro.o fdt_wip.o fdt.o
>
> diff --git a/arch/h8300/boot/compressed/Makefile b/arch/h8300/boot/compressed/Makefile
> index 5942793f77a0..6ab2fa5ba105 100644
> --- a/arch/h8300/boot/compressed/Makefile
> +++ b/arch/h8300/boot/compressed/Makefile
> @@ -30,9 +30,11 @@ $(obj)/vmlinux.bin: vmlinux FORCE
>
> suffix-$(CONFIG_KERNEL_GZIP) := gzip
> suffix-$(CONFIG_KERNEL_LZO) := lzo
> +compress-$(CONFIG_KERNEL_GZIP) := gzip
> +compress-$(CONFIG_KERNEL_LZO) := lzo_with_size
>
> $(obj)/vmlinux.bin.$(suffix-y): $(obj)/vmlinux.bin FORCE
> - $(call if_changed,$(suffix-y))
> + $(call if_changed,$(compress-y))
>
> LDFLAGS_piggy.o := -r --format binary --oformat elf32-h8300-linux -T
> OBJCOPYFLAGS := -O binary
> diff --git a/arch/mips/boot/compressed/Makefile b/arch/mips/boot/compressed/Makefile
> index f27cf31b4140..832f8001d7d9 100644
> --- a/arch/mips/boot/compressed/Makefile
> +++ b/arch/mips/boot/compressed/Makefile
> @@ -64,12 +64,12 @@ $(obj)/vmlinux.bin: $(KBUILD_IMAGE) FORCE
> $(call if_changed,objcopy)
>
> tool_$(CONFIG_KERNEL_GZIP) = gzip
> -tool_$(CONFIG_KERNEL_BZIP2) = bzip2
> -tool_$(CONFIG_KERNEL_LZ4) = lz4
> -tool_$(CONFIG_KERNEL_LZMA) = lzma
> -tool_$(CONFIG_KERNEL_LZO) = lzo
> -tool_$(CONFIG_KERNEL_XZ) = xzkern
> -tool_$(CONFIG_KERNEL_ZSTD) = zstd22
> +tool_$(CONFIG_KERNEL_BZIP2) = bzip2_with_size
> +tool_$(CONFIG_KERNEL_LZ4) = lz4_with_size
> +tool_$(CONFIG_KERNEL_LZMA) = lzma_with_size
> +tool_$(CONFIG_KERNEL_LZO) = lzo_with_size
> +tool_$(CONFIG_KERNEL_XZ) = xzkern_with_size
> +tool_$(CONFIG_KERNEL_ZSTD) = zstd22_with_size
>
> targets += vmlinux.bin.z
>
> diff --git a/arch/parisc/boot/compressed/Makefile b/arch/parisc/boot/compressed/Makefile
> index bf4f2891d0b7..2640f72d69ce 100644
> --- a/arch/parisc/boot/compressed/Makefile
> +++ b/arch/parisc/boot/compressed/Makefile
> @@ -70,15 +70,15 @@ suffix-$(CONFIG_KERNEL_XZ) := xz
> $(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
> $(call if_changed,gzip)
> $(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,bzip2)
> + $(call if_changed,bzip2_with_size)
> $(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lz4)
> + $(call if_changed,lz4_with_size)
> $(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzma)
> + $(call if_changed,lzma_with_size)
> $(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzo)
> + $(call if_changed,lzo_with_size)
> $(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,xzkern)
> + $(call if_changed,xzkern_with_size)
>
> LDFLAGS_piggy.o := -r --format binary --oformat $(LD_BFD) -T
> $(obj)/piggy.o: $(obj)/vmlinux.scr $(obj)/vmlinux.bin.$(suffix-y) FORCE
> diff --git a/arch/s390/boot/compressed/Makefile b/arch/s390/boot/compressed/Makefile
> index 3b860061e84d..8ea880b7c3ec 100644
> --- a/arch/s390/boot/compressed/Makefile
> +++ b/arch/s390/boot/compressed/Makefile
> @@ -71,17 +71,17 @@ suffix-$(CONFIG_KERNEL_ZSTD) := .zst
> $(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
> $(call if_changed,gzip)
> $(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,bzip2)
> + $(call if_changed,bzip2_with_size)
> $(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lz4)
> + $(call if_changed,lz4_with_size)
> $(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzma)
> + $(call if_changed,lzma_with_size)
> $(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzo)
> + $(call if_changed,lzo_with_size)
> $(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,xzkern)
> + $(call if_changed,xzkern_with_size)
> $(obj)/vmlinux.bin.zst: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,zstd22)
> + $(call if_changed,zstd22_with_size)
>
> OBJCOPYFLAGS_piggy.o := -I binary -O elf64-s390 -B s390:64-bit --rename-section .data=.vmlinux.bin.compressed
> $(obj)/piggy.o: $(obj)/vmlinux.bin$(suffix-y) FORCE
> diff --git a/arch/sh/boot/compressed/Makefile b/arch/sh/boot/compressed/Makefile
> index c1eb9a62de55..a6808a403f4b 100644
> --- a/arch/sh/boot/compressed/Makefile
> +++ b/arch/sh/boot/compressed/Makefile
> @@ -52,13 +52,13 @@ vmlinux.bin.all-y := $(obj)/vmlinux.bin
> $(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
> $(call if_changed,gzip)
> $(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,bzip2)
> + $(call if_changed,bzip2_with_size)
> $(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzma)
> + $(call if_changed,lzma_with_size)
> $(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,xzkern)
> + $(call if_changed,xzkern_with_size)
> $(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzo)
> + $(call if_changed,lzo_with_size)
>
> OBJCOPYFLAGS += -R .empty_zero_page
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 431bf7f846c3..2825c74bcae3 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -123,17 +123,17 @@ vmlinux.bin.all-$(CONFIG_X86_NEED_RELOCS) += $(obj)/vmlinux.relocs
> $(obj)/vmlinux.bin.gz: $(vmlinux.bin.all-y) FORCE
> $(call if_changed,gzip)
> $(obj)/vmlinux.bin.bz2: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,bzip2)
> + $(call if_changed,bzip2_with_size)
> $(obj)/vmlinux.bin.lzma: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzma)
> + $(call if_changed,lzma_with_size)
> $(obj)/vmlinux.bin.xz: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,xzkern)
> + $(call if_changed,xzkern_with_size)
> $(obj)/vmlinux.bin.lzo: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lzo)
> + $(call if_changed,lzo_with_size)
> $(obj)/vmlinux.bin.lz4: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,lz4)
> + $(call if_changed,lz4_with_size)
> $(obj)/vmlinux.bin.zst: $(vmlinux.bin.all-y) FORCE
> - $(call if_changed,zstd22)
> + $(call if_changed,zstd22_with_size)
>
> suffix-$(CONFIG_KERNEL_GZIP) := gz
> suffix-$(CONFIG_KERNEL_BZIP2) := bz2
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 5366466ea0e4..4207a72d429f 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -395,19 +395,31 @@ printf "%08x\n" $$dec_size | \
> )
>
> quiet_cmd_bzip2 = BZIP2 $@
> - cmd_bzip2 = { cat $(real-prereqs) | $(KBZIP2) -9; $(size_append); } > $@
> + cmd_bzip2 = cat $(real-prereqs) | $(KBZIP2) -9 > $@
> +
> +quiet_cmd_bzip2_with_size = BZIP2 $@
> + cmd_bzip2_with_size = { cat $(real-prereqs) | $(KBZIP2) -9; $(size_append); } > $@
>
> # Lzma
> # ---------------------------------------------------------------------------
>
> quiet_cmd_lzma = LZMA $@
> - cmd_lzma = { cat $(real-prereqs) | $(LZMA) -9; $(size_append); } > $@
> + cmd_lzma = cat $(real-prereqs) | $(LZMA) -9 > $@
> +
> +quiet_cmd_lzma_with_size = LZMA $@
> + cmd_lzma_with_size = { cat $(real-prereqs) | $(LZMA) -9; $(size_append); } > $@
>
> quiet_cmd_lzo = LZO $@
> - cmd_lzo = { cat $(real-prereqs) | $(KLZOP) -9; $(size_append); } > $@
> + cmd_lzo = cat $(real-prereqs) | $(KLZOP) -9 > $@
> +
> +quiet_cmd_lzo_with_size = LZO $@
> + cmd_lzo_with_size = { cat $(real-prereqs) | $(KLZOP) -9; $(size_append); } > $@
>
> quiet_cmd_lz4 = LZ4 $@
> - cmd_lz4 = { cat $(real-prereqs) | $(LZ4) -l -c1 stdin stdout; \
> + cmd_lz4 = cat $(real-prereqs) | $(LZ4) -l -c1 stdin stdout > $@
> +
> +quiet_cmd_lz4_with_size = LZ4 $@
> + cmd_lz4_with_size = { cat $(real-prereqs) | $(LZ4) -l -c1 stdin stdout; \
> $(size_append); } > $@
>
> # U-Boot mkimage
> @@ -450,7 +462,10 @@ quiet_cmd_uimage = UIMAGE $@
> # big dictionary would increase the memory usage too much in the multi-call
> # decompression mode. A BCJ filter isn't used either.
> quiet_cmd_xzkern = XZKERN $@
> - cmd_xzkern = { cat $(real-prereqs) | sh $(srctree)/scripts/xz_wrap.sh; \
> + cmd_xzkern = cat $(real-prereqs) | sh $(srctree)/scripts/xz_wrap.sh > $@
> +
> +quiet_cmd_xzkern_with_size = XZKERN $@
> + cmd_xzkern_with_size = { cat $(real-prereqs) | sh $(srctree)/scripts/xz_wrap.sh; \
> $(size_append); } > $@
>
> quiet_cmd_xzmisc = XZMISC $@
> @@ -476,7 +491,10 @@ quiet_cmd_zstd = ZSTD $@
> cmd_zstd = cat $(real-prereqs) | $(ZSTD) -19 > $@
>
> quiet_cmd_zstd22 = ZSTD22 $@
> - cmd_zstd22 = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
> + cmd_zstd22 = cat $(real-prereqs) | $(ZSTD) -22 --ultra > $@
> +
> +quiet_cmd_zstd22_with_size = ZSTD22 $@
> + cmd_zstd22_with_size = { cat $(real-prereqs) | $(ZSTD) -22 --ultra; $(size_append); } > $@
>
> # ASM offsets
> # ---------------------------------------------------------------------------
> diff --git a/usr/Makefile b/usr/Makefile
> index b1a81a40eab1..7b89c0175a3a 100644
> --- a/usr/Makefile
> +++ b/usr/Makefile
> @@ -3,11 +3,6 @@
> # kbuild file for usr/ - including initramfs image
> #
>
> -# cmd_bzip2, cmd_lzma, cmd_lzo, cmd_lz4 from scripts/Makefile.lib appends the
> -# size at the end of the compressed file, which unfortunately does not work
> -# with unpack_to_rootfs(). Make size_append no-op.
> -override size_append := :
> -
> compress-y := shipped
> compress-$(CONFIG_INITRAMFS_COMPRESSION_GZIP) := gzip
> compress-$(CONFIG_INITRAMFS_COMPRESSION_BZIP2) := bzip2
> --
> 2.32.0
>

2023-06-23 15:11:40

by Eugeniu Rosca

[permalink] [raw]
Subject: Re: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

Hello Yamada-san,
Hello Nicolas,
Cc: SzuWei Lin (committer of the patch in AOSP [1])
Cc: Kbuild

On Mon, Jan 10, 2022 at 12:33:15PM +0100, Nicolas Schier wrote:
> On Mon, Jan 10, 2022 at 03:15:27AM +0900, Masahiro Yamada wrote:
> > GZIP-compressed files end with 4 byte data that represents the size
> > of the original input. The decompressors (the self-extracting kernel)
> > exploit it to know the vmlinux size beforehand. To mimic the GZIP's
> > trailer, Kbuild provides cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}.
> > Unfortunately these macros are used everywhere despite the appended
> > size data is only useful for the decompressors.
> >
> > There is no guarantee that such hand-crafted trailers are safely ignored.
> > In fact, the kernel refuses compressed initramdisks with the garbage
> > data. That is why usr/Makefile overrides size_append to make it no-op.
> >
> > To limit the use of such broken compressed files, this commit renames
> > the existing macros as follows:
> >
> > cmd_bzip2 --> cmd_bzip2_with_size
> > cmd_lzma --> cmd_lzma_with_size
> > cmd_lzo --> cmd_lzo_with_size
> > cmd_lz4 --> cmd_lz4_with_size
> > cmd_xzkern --> cmd_xzkern_with_size
> > cmd_zstd22 --> cmd_zstd22_with_size
> >
> > To keep the decompressors working, I updated the following Makefiles
> > accordingly:
> >
> > arch/arm/boot/compressed/Makefile
> > arch/h8300/boot/compressed/Makefile
> > arch/mips/boot/compressed/Makefile
> > arch/parisc/boot/compressed/Makefile
> > arch/s390/boot/compressed/Makefile
> > arch/sh/boot/compressed/Makefile
> > arch/x86/boot/compressed/Makefile
> >
> > I reused the current macro names for the normal usecases; they produce
> > the compressed data in the proper format.
> >
> > I did not touch the following:
> >
> > arch/arc/boot/Makefile
> > arch/arm64/boot/Makefile
> > arch/csky/boot/Makefile
> > arch/mips/boot/Makefile
> > arch/riscv/boot/Makefile
> > arch/sh/boot/Makefile
> > kernel/Makefile
> >
> > This means those Makefiles will stop appending the size data.
> >
> > I dropped the 'override size_append' hack from usr/Makefile.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
>
> Reviewed-by: Nicolas Schier <[email protected]>

If you don't mind, I would like to report another instance of
"/bin/sh: Argument list too long" while building some out-of-tree *ko
in a number of downstream v5.15.78+ kernels containing [1].

For some time now, we've been living with ugly hacks to overcome it.

Fortunately, recent git bisecting efforts apparently reveal that
current v5.17-rc1 commit (and its backports in downstream) look to
act as the culprit (confirmed on several host machines). So, I
started to have some hopes of a long-term solution and hence
sharing the findings as a first step.

I am not entirely clear how to properly trace this behavior, since no
amount of "make V=1/V=2" uncovers more details. Purely by accident, I
looked into the top/htop output (while running the repro) and
noticed several processes doing:

/bin/sh -c dec_size=0; for F in <humongous list of filenames>; do \
fsize=$(sh /abs/path/to/scripts/file-size.sh $F); \
dec_size=$(expr $dec_size + $fsize); done; printf "%08x\n" $dec_size \
| sed 's/\(..\)/\1 /g' | { read ch0 ch1 ch2 ch3; for ch in \
$ch3 $ch2 $ch1 $ch0; do printf '%s%03o' '\\' $((0x$ch)); done; }

As it was the case in the recent report [2], the above command seems
to require/assume generous amount of space for the shell arguments.

I still haven't compared the exact traces before and after this commit,
to quantify by how much the shell argument list is increased (TODO).

Another aspect is that current commit seems to introduce the
regression in a multi-threaded make only. The issue is apparently
masked by 'make -j1' (TBC), which adds another level of complexity.

Unfortunately, the build use-case is highly tailored to downstream
and is not repeatable against vanilla out of the box.

I will continue to increase my understanding behind what's happening.
In case there are already any suggestions, would appreciate those.

[1] https://android.googlesource.com/kernel/common/+/bc6d3d83539512
("UPSTREAM: kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}")

[2] https://lore.kernel.org/linux-kbuild/20230616194505.GA27753@lxhi-065/

--
Best regards,
Eugeniu Rosca

2023-07-19 19:28:11

by Eugeniu Rosca

[permalink] [raw]
Subject: Re: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

Hello Yamada-san,

On Fri, Jun 23, 2023 at 04:45:44PM +0200, Eugeniu Rosca wrote:
> Hello Yamada-san,
> Hello Nicolas,
> Cc: SzuWei Lin (committer of the patch in AOSP [1])
> Cc: Kbuild
>
> On Mon, Jan 10, 2022 at 12:33:15PM +0100, Nicolas Schier wrote:
> > On Mon, Jan 10, 2022 at 03:15:27AM +0900, Masahiro Yamada wrote:
> > > GZIP-compressed files end with 4 byte data that represents the size
> > > of the original input. The decompressors (the self-extracting kernel)
> > > exploit it to know the vmlinux size beforehand. To mimic the GZIP's
> > > trailer, Kbuild provides cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}.
> > > Unfortunately these macros are used everywhere despite the appended
> > > size data is only useful for the decompressors.
> > >
> > > There is no guarantee that such hand-crafted trailers are safely ignored.
> > > In fact, the kernel refuses compressed initramdisks with the garbage
> > > data. That is why usr/Makefile overrides size_append to make it no-op.
> > >
> > > To limit the use of such broken compressed files, this commit renames
> > > the existing macros as follows:
> > >
> > > cmd_bzip2 --> cmd_bzip2_with_size
> > > cmd_lzma --> cmd_lzma_with_size
> > > cmd_lzo --> cmd_lzo_with_size
> > > cmd_lz4 --> cmd_lz4_with_size
> > > cmd_xzkern --> cmd_xzkern_with_size
> > > cmd_zstd22 --> cmd_zstd22_with_size
> > >
> > > To keep the decompressors working, I updated the following Makefiles
> > > accordingly:
> > >
> > > arch/arm/boot/compressed/Makefile
> > > arch/h8300/boot/compressed/Makefile
> > > arch/mips/boot/compressed/Makefile
> > > arch/parisc/boot/compressed/Makefile
> > > arch/s390/boot/compressed/Makefile
> > > arch/sh/boot/compressed/Makefile
> > > arch/x86/boot/compressed/Makefile
> > >
> > > I reused the current macro names for the normal usecases; they produce
> > > the compressed data in the proper format.
> > >
> > > I did not touch the following:
> > >
> > > arch/arc/boot/Makefile
> > > arch/arm64/boot/Makefile
> > > arch/csky/boot/Makefile
> > > arch/mips/boot/Makefile
> > > arch/riscv/boot/Makefile
> > > arch/sh/boot/Makefile
> > > kernel/Makefile
> > >
> > > This means those Makefiles will stop appending the size data.
> > >
> > > I dropped the 'override size_append' hack from usr/Makefile.
> > >
> > > Signed-off-by: Masahiro Yamada <[email protected]>
> > > ---
> >
> > Reviewed-by: Nicolas Schier <[email protected]>
>
> If you don't mind, I would like to report another instance of
> "/bin/sh: Argument list too long" while building some out-of-tree *ko
> in a number of downstream v5.15.78+ kernels containing [1].
>
> For some time now, we've been living with ugly hacks to overcome it.
>
> Fortunately, recent git bisecting efforts apparently reveal that
> current v5.17-rc1 commit (and its backports in downstream) look to
> act as the culprit (confirmed on several host machines). So, I
> started to have some hopes of a long-term solution and hence
> sharing the findings as a first step.
>
> I am not entirely clear how to properly trace this behavior, since no
> amount of "make V=1/V=2" uncovers more details. Purely by accident, I
> looked into the top/htop output (while running the repro) and
> noticed several processes doing:
>
> /bin/sh -c dec_size=0; for F in <humongous list of filenames>; do \
> fsize=$(sh /abs/path/to/scripts/file-size.sh $F); \
> dec_size=$(expr $dec_size + $fsize); done; printf "%08x\n" $dec_size \
> | sed 's/\(..\)/\1 /g' | { read ch0 ch1 ch2 ch3; for ch in \
> $ch3 $ch2 $ch1 $ch0; do printf '%s%03o' '\\' $((0x$ch)); done; }
>
> As it was the case in the recent report [2], the above command seems
> to require/assume generous amount of space for the shell arguments.
>
> I still haven't compared the exact traces before and after this commit,
> to quantify by how much the shell argument list is increased (TODO).
>
> Another aspect is that current commit seems to introduce the
> regression in a multi-threaded make only. The issue is apparently
> masked by 'make -j1' (TBC), which adds another level of complexity.
>
> Unfortunately, the build use-case is highly tailored to downstream
> and is not repeatable against vanilla out of the box.
>
> I will continue to increase my understanding behind what's happening.
> In case there are already any suggestions, would appreciate those.

JFYI, we've got confirmation from Qualcomm Customer Support interface
that reverting [1] heals the issue on QC end as well. However, it looks
like none of us has clear understanding how to properly
troubleshoot/trace/compare the behavior before and after the commit.

I would happily follow any suggestions.

> [1] https://android.googlesource.com/kernel/common/+/bc6d3d83539512
> ("UPSTREAM: kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}")
>
> [2] https://lore.kernel.org/linux-kbuild/20230616194505.GA27753@lxhi-065/

--
Best regards,
Eugeniu Rosca

2023-07-22 17:24:30

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

On Thu, Jul 20, 2023 at 4:09 AM Eugeniu Rosca <[email protected]> wrote:
>
> Hello Yamada-san,
>
> On Fri, Jun 23, 2023 at 04:45:44PM +0200, Eugeniu Rosca wrote:
> > Hello Yamada-san,
> > Hello Nicolas,
> > Cc: SzuWei Lin (committer of the patch in AOSP [1])
> > Cc: Kbuild
> >
> > On Mon, Jan 10, 2022 at 12:33:15PM +0100, Nicolas Schier wrote:
> > > On Mon, Jan 10, 2022 at 03:15:27AM +0900, Masahiro Yamada wrote:
> > > > GZIP-compressed files end with 4 byte data that represents the size
> > > > of the original input. The decompressors (the self-extracting kernel)
> > > > exploit it to know the vmlinux size beforehand. To mimic the GZIP's
> > > > trailer, Kbuild provides cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}.
> > > > Unfortunately these macros are used everywhere despite the appended
> > > > size data is only useful for the decompressors.
> > > >
> > > > There is no guarantee that such hand-crafted trailers are safely ignored.
> > > > In fact, the kernel refuses compressed initramdisks with the garbage
> > > > data. That is why usr/Makefile overrides size_append to make it no-op.
> > > >
> > > > To limit the use of such broken compressed files, this commit renames
> > > > the existing macros as follows:
> > > >
> > > > cmd_bzip2 --> cmd_bzip2_with_size
> > > > cmd_lzma --> cmd_lzma_with_size
> > > > cmd_lzo --> cmd_lzo_with_size
> > > > cmd_lz4 --> cmd_lz4_with_size
> > > > cmd_xzkern --> cmd_xzkern_with_size
> > > > cmd_zstd22 --> cmd_zstd22_with_size
> > > >
> > > > To keep the decompressors working, I updated the following Makefiles
> > > > accordingly:
> > > >
> > > > arch/arm/boot/compressed/Makefile
> > > > arch/h8300/boot/compressed/Makefile
> > > > arch/mips/boot/compressed/Makefile
> > > > arch/parisc/boot/compressed/Makefile
> > > > arch/s390/boot/compressed/Makefile
> > > > arch/sh/boot/compressed/Makefile
> > > > arch/x86/boot/compressed/Makefile
> > > >
> > > > I reused the current macro names for the normal usecases; they produce
> > > > the compressed data in the proper format.
> > > >
> > > > I did not touch the following:
> > > >
> > > > arch/arc/boot/Makefile
> > > > arch/arm64/boot/Makefile
> > > > arch/csky/boot/Makefile
> > > > arch/mips/boot/Makefile
> > > > arch/riscv/boot/Makefile
> > > > arch/sh/boot/Makefile
> > > > kernel/Makefile
> > > >
> > > > This means those Makefiles will stop appending the size data.
> > > >
> > > > I dropped the 'override size_append' hack from usr/Makefile.
> > > >
> > > > Signed-off-by: Masahiro Yamada <[email protected]>
> > > > ---
> > >
> > > Reviewed-by: Nicolas Schier <[email protected]>
> >
> > If you don't mind, I would like to report another instance of
> > "/bin/sh: Argument list too long" while building some out-of-tree *ko
> > in a number of downstream v5.15.78+ kernels containing [1].
> >
> > For some time now, we've been living with ugly hacks to overcome it.
> >
> > Fortunately, recent git bisecting efforts apparently reveal that
> > current v5.17-rc1 commit (and its backports in downstream) look to
> > act as the culprit (confirmed on several host machines). So, I
> > started to have some hopes of a long-term solution and hence
> > sharing the findings as a first step.
> >
> > I am not entirely clear how to properly trace this behavior, since no
> > amount of "make V=1/V=2" uncovers more details. Purely by accident, I
> > looked into the top/htop output (while running the repro) and
> > noticed several processes doing:
> >
> > /bin/sh -c dec_size=0; for F in <humongous list of filenames>; do \
> > fsize=$(sh /abs/path/to/scripts/file-size.sh $F); \
> > dec_size=$(expr $dec_size + $fsize); done; printf "%08x\n" $dec_size \
> > | sed 's/\(..\)/\1 /g' | { read ch0 ch1 ch2 ch3; for ch in \
> > $ch3 $ch2 $ch1 $ch0; do printf '%s%03o' '\\' $((0x$ch)); done; }
> >
> > As it was the case in the recent report [2], the above command seems
> > to require/assume generous amount of space for the shell arguments.
> >
> > I still haven't compared the exact traces before and after this commit,
> > to quantify by how much the shell argument list is increased (TODO).
> >
> > Another aspect is that current commit seems to introduce the
> > regression in a multi-threaded make only. The issue is apparently
> > masked by 'make -j1' (TBC), which adds another level of complexity.
> >
> > Unfortunately, the build use-case is highly tailored to downstream
> > and is not repeatable against vanilla out of the box.
> >
> > I will continue to increase my understanding behind what's happening.
> > In case there are already any suggestions, would appreciate those.
>
> JFYI, we've got confirmation from Qualcomm Customer Support interface
> that reverting [1] heals the issue on QC end as well. However, it looks
> like none of us has clear understanding how to properly
> troubleshoot/trace/compare the behavior before and after the commit.
>
> I would happily follow any suggestions.
>
> > [1] https://android.googlesource.com/kernel/common/+/bc6d3d83539512
> > ("UPSTREAM: kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}")
> >
> > [2] https://lore.kernel.org/linux-kbuild/20230616194505.GA27753@lxhi-065/
>
> --
> Best regards,
> Eugeniu Rosca





The only suspicious code I found in the Android common kernel
is the following line in scripts/Makefile.lib



quiet_cmd_zstd = ZSTD $@
cmd_zstd = { cat $(real-prereqs) | $(ZSTD) -19; $(size_append); } > $@







If you see the corresponding line in the mainline kernel,
it looks as follows:


quiet_cmd_zstd = ZSTD $@
cmd_zstd = cat $(real-prereqs) | $(ZSTD) -19 > $@








7ce7e984ab2b218d6e92d5165629022fe2daf9ee depends on
64d8aaa4ef388b22372de4dc9ce3b9b3e5f45b6c


But, Android common kernel back-ported only
7ce7e984ab2b218d6e92d5165629022fe2daf9ee



Please backport 64d8aaa4ef388b22372de4dc9ce3b9b3e5f45b6c
and see if the problem goes away.


--
Best Regards
Masahiro Yamada

2023-07-25 09:29:58

by Eugeniu Rosca

[permalink] [raw]
Subject: Re: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

Hello Yamada-san,

Appreciate your willingness to support. Some findings below.

On Sun, Jul 23, 2023 at 01:08:46AM +0900, Masahiro Yamada wrote:
> On Thu, Jul 20, 2023 at 4:09 AM Eugeniu Rosca <[email protected]> wrote:
> > On Fri, Jun 23, 2023 at 04:45:44PM +0200, Eugeniu Rosca wrote:

[..]

> > > I will continue to increase my understanding behind what's happening.
> > > In case there are already any suggestions, would appreciate those.
> >
> > JFYI, we've got confirmation from Qualcomm Customer Support interface
> > that reverting [1] heals the issue on QC end as well. However, it looks
> > like none of us has clear understanding how to properly
> > troubleshoot/trace/compare the behavior before and after the commit.
> >
> > I would happily follow any suggestions.
> >
> > > [1] https://android.googlesource.com/kernel/common/+/bc6d3d83539512
> > > ("UPSTREAM: kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}")
> > >
> > > [2] https://lore.kernel.org/linux-kbuild/20230616194505.GA27753@lxhi-065/

[..]

> Please backport 64d8aaa4ef388b22372de4dc9ce3b9b3e5f45b6c
> and see if the problem goes away.

Unfortunately, the problem remains after backporting the above commit.

After some more bisecting and some more trial-and-error, I finally came
up with a reproduction scenario against vanilla. It also shows that
after reverting 7ce7e984ab2b21 ("kbuild: rename
cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}"), the problem goes away.

It takes <30 seconds to reproduce the issue on my machine (on 2nd run).

In order to make the test self-sufficient, it also clones the Linux
sources (only during 1st run, with --depth 1, for minimal footprint),
hence ~1.8 GB free space is required in /tmp .

The repro.sh script:
=> https://gist.github.com/erosca/1372fdc24126dc98031444613450c494

Output against vanilla on 1st run (always OK, matches real-life case):
=> https://gist.github.com/erosca/0f5b8e0a00a256d80f0c8a6364d81568

Output against vanilla on 2nd/Nth run (NOK: Argument list too long):
=> https://gist.github.com/erosca/e5c2c6479cc32244cc38d308deea4cf5

Output against vanilla + revert_of_7ce7e984ab2b2 on Nth run (always OK):
=> https://gist.github.com/erosca/57e114f92ea20132e19fc7f5a46e7c65

Would it be possible to get your thoughts on the above?

--
Best regards,
Eugeniu Rosca

2023-08-02 10:25:38

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

On Tue, Jul 25, 2023 at 6:24 PM Eugeniu Rosca <[email protected]> wrote:
>
> Hello Yamada-san,
>
> Appreciate your willingness to support. Some findings below.
>
> On Sun, Jul 23, 2023 at 01:08:46AM +0900, Masahiro Yamada wrote:
> > On Thu, Jul 20, 2023 at 4:09 AM Eugeniu Rosca <[email protected]> wrote:
> > > On Fri, Jun 23, 2023 at 04:45:44PM +0200, Eugeniu Rosca wrote:
>
> [..]
>
> > > > I will continue to increase my understanding behind what's happening.
> > > > In case there are already any suggestions, would appreciate those.
> > >
> > > JFYI, we've got confirmation from Qualcomm Customer Support interface
> > > that reverting [1] heals the issue on QC end as well. However, it looks
> > > like none of us has clear understanding how to properly
> > > troubleshoot/trace/compare the behavior before and after the commit.
> > >
> > > I would happily follow any suggestions.
> > >
> > > > [1] https://android.googlesource.com/kernel/common/+/bc6d3d83539512
> > > > ("UPSTREAM: kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}")
> > > >
> > > > [2] https://lore.kernel.org/linux-kbuild/20230616194505.GA27753@lxhi-065/
>
> [..]
>
> > Please backport 64d8aaa4ef388b22372de4dc9ce3b9b3e5f45b6c
> > and see if the problem goes away.
>
> Unfortunately, the problem remains after backporting the above commit.
>
> After some more bisecting and some more trial-and-error, I finally came
> up with a reproduction scenario against vanilla. It also shows that
> after reverting 7ce7e984ab2b21 ("kbuild: rename
> cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}"), the problem goes away.
>
> It takes <30 seconds to reproduce the issue on my machine (on 2nd run).
>
> In order to make the test self-sufficient, it also clones the Linux
> sources (only during 1st run, with --depth 1, for minimal footprint),
> hence ~1.8 GB free space is required in /tmp .
>
> The repro.sh script:
> => https://gist.github.com/erosca/1372fdc24126dc98031444613450c494
>
> Output against vanilla on 1st run (always OK, matches real-life case):
> => https://gist.github.com/erosca/0f5b8e0a00a256d80f0c8a6364d81568
>
> Output against vanilla on 2nd/Nth run (NOK: Argument list too long):
> => https://gist.github.com/erosca/e5c2c6479cc32244cc38d308deea4cf5
>
> Output against vanilla + revert_of_7ce7e984ab2b2 on Nth run (always OK):
> => https://gist.github.com/erosca/57e114f92ea20132e19fc7f5a46e7c65
>
> Would it be possible to get your thoughts on the above?
>
> --
> Best regards,
> Eugeniu Rosca





Indeed, reverting 7ce7e984ab2b218d6e92d5165629022fe2daf9ee
makes qcom's external module build successfully
(but rebuilding is super slow).

Interestingly, revert 7ce7e984ab2b218d6e92d5165629022fe2daf9ee
then apply the attached patch, then
'Argument list too long' will come back.

So, this is unrelated to the actual build commands.




I suspect bare 'export', which expands all variables
while apparently most of them are not meant exported.



Insert the following in your reproducer, then it will work.


# qcom's audio-kernel sprinkles 'export' everywhere.
# Remove bare use of 'export'
find "$ABS_KMOD" -name Kbuild | xargs sed -i '/export$/d'




--
Best Regards
Masahiro Yamada

Attachments:
0001-Add-dummy-commands-to-make-qcom-s-external-module-fa.patch (1.60 kB)

2023-08-11 12:09:05

by Eugeniu Rosca

[permalink] [raw]
Subject: Re: [PATCH 3/5] kbuild: rename cmd_{bzip2,lzma,lzo,lz4,xzkern,zstd22}

Hello Yamada-san,

Appreciate your generous support, which allowed finding the root-cause.

On Wed, Aug 02, 2023 at 06:21:14PM +0900, Masahiro Yamada wrote:

[..]

> Indeed, reverting 7ce7e984ab2b218d6e92d5165629022fe2daf9ee
> makes qcom's external module build successfully
> (but rebuilding is super slow).

Same observation.

>
> Interestingly, revert 7ce7e984ab2b218d6e92d5165629022fe2daf9ee
> then apply the attached patch, then
> 'Argument list too long' will come back.
>
> So, this is unrelated to the actual build commands.
>
> I suspect bare 'export', which expands all variables
> while apparently most of them are not meant exported.

Indeed, that seems to be the case and more evidence supports this:

* Several pre-existing commits point out to the same root-cause:

https://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/audio-kernel-ar/-/commit/4025a25a2479fc34
("makefile: kona: remove make export <all variables> instances")

https://github.com/sonyxperiadev/kernel-techpack-audio/commit/02f00754120df2
("audio-kernel: Fix build time issue")

* The more recent GNU Make 4.4.1 (called with -ddd) reveals more
details just before hitting 'Argument list too long':

scripts/Makefile.lib:431: not recursively expanding size_append to export to shell function
scripts/Makefile.lib:447: not recursively expanding cmd_file_size to export to shell function
scripts/Makefile.lib:453: not recursively expanding cmd_bzip2_with_size to export to shell function
scripts/Makefile.lib:462: not recursively expanding cmd_lzma_with_size to export to shell function
scripts/Makefile.lib:468: not recursively expanding cmd_lzo_with_size to export to shell function
scripts/Makefile.lib:474: not recursively expanding cmd_lz4_with_size to export to shell function
scripts/Makefile.lib:520: not recursively expanding cmd_xzkern_with_size to export to shell function
scripts/Makefile.lib:549: not recursively expanding cmd_zstd22_with_size to export to shell function

The "not recursively expanding" messages above come from GNU
make commit https://gnu.googlesource.com/make/+/7d484017077089a
("[SV 63016] Don't fail exporting to $(shell ...)"), which appears
to be saying that our issue is caused by exporting a makefile
variable which contains the 'shell' directive in its definition.

I think it would be too much of a burden for Kbuild not to make use
of any variables using the 'shell' keyword, so I tend to agree that
defending against bare 'export' statements in Kbuild is rather not
feasible and it should be fixed in the makefiles of out-of-tree *ko.

> Insert the following in your reproducer, then it will work.

Thanks for the demo code snippet.

--
Best regards,
Eugeniu Rosca