LinuxLists.cc - [PATCH v2 0/2] use interpreters to invoke scripts

2020-10-12 17:11:09

Subject: [PATCH v2 0/2] use interpreters to invoke scripts

This patch series aims at removing the dependency on execute
bit of the scripts in the kbuild system.

If not working with fresh clone of linux-next, clean the srctree:
make distclean
make tools/clean

To test the dependency on execute bits, I tried building the
kernel after removing x-bits for all files in the repository.
Removing execute bits:
for i in $(find -executable -type f); do chmod -x $i; done

Any attempts to configure (or build) the kernel fail because of
'Permission denied' on scripts with the following error:
$ make allmodconfig
sh: ./scripts/gcc-version.sh: Permission denied
init/Kconfig:34: syntax error
init/Kconfig:33: invalid statement
init/Kconfig:34: invalid statement
sh: ./scripts/ld-version.sh: Permission denied
init/Kconfig:39: syntax error
init/Kconfig:38: invalid statement
sh: ./scripts/clang-version.sh: Permission denied
init/Kconfig:49: syntax error
init/Kconfig:48: invalid statement
make[1]: *** [scripts/kconfig/Makefile:71: allmodconfig] Error 1
make: *** [Makefile:606: allmodconfig] Error 2

Changes:
- Adds specific interpreters (in Kconfig) to invoke
scripts.

After this patch I could successfully do a kernel build
without any errors.

- Again, adds specific interpreters to other parts of
kbuild system.

I could successfully perform the following make targets after
applying the PATCH 2/2:
make headerdep
make kselftest-merge
make rpm-pkg
make perf-tar-src-pkg
make ARCH=ia64 defconfig
ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make prepare

Following changes in PATCH 2/2 are not yet tested:
arch/arm64/kernel/vdso32/Makefile
arch/nds32/kernel/vdso/Makefile
scripts/Makefile.build

---
Changes in v2:

- Changes suggested by Masahiro Yamada
$($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)

Ujjwal Kumar (2):
kconfig: use interpreters to invoke scripts
kbuild: use interpreters to invoke scripts

Makefile | 4 ++--
arch/arm64/kernel/vdso/Makefile | 2 +-
arch/arm64/kernel/vdso32/Makefile | 2 +-
arch/ia64/Makefile | 4 ++--
arch/nds32/kernel/vdso/Makefile | 2 +-
init/Kconfig | 16 ++++++++--------
scripts/Makefile.build | 2 +-
scripts/Makefile.package | 4 ++--
8 files changed, 18 insertions(+), 18 deletions(-)

base-commit: 2cab4ac556258c14f6ec8d2157654e95556bbb31
--
2.25.1

2020-10-12 17:13:11

by Ujjwal Kumar

[permalink] [raw]

Subject: [PATCH v2 2/2] kbuild: use interpreters to invoke scripts

We cannot rely on execute bits to be set on files in the repository.
The build script should use the explicit interpreter when invoking any
script from the repository.

Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/

Suggested-by: Andrew Morton <[email protected]>
Suggested-by: Kees Cook <[email protected]>
Suggested-by: Lukas Bulwahn <[email protected]>
Signed-off-by: Ujjwal Kumar <[email protected]>
---
Makefile | 4 ++--
arch/arm64/kernel/vdso/Makefile | 2 +-
arch/arm64/kernel/vdso32/Makefile | 2 +-
arch/ia64/Makefile | 4 ++--
arch/nds32/kernel/vdso/Makefile | 2 +-
scripts/Makefile.build | 2 +-
scripts/Makefile.package | 4 ++--
7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 0af7945caa61..df20e71dd7c8 100644
--- a/Makefile
+++ b/Makefile
@@ -1256,7 +1256,7 @@ include/generated/utsrelease.h: include/config/kernel.release FORCE
PHONY += headerdep
headerdep:
$(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
- $(srctree)/scripts/headerdep.pl -I$(srctree)/include
+ $(PERL) $(srctree)/scripts/headerdep.pl -I$(srctree)/include

# ---------------------------------------------------------------------------
# Kernel headers
@@ -1312,7 +1312,7 @@ PHONY += kselftest-merge
kselftest-merge:
$(if $(wildcard $(objtree)/.config),, $(error No .config exists, config your kernel first!))
$(Q)find $(srctree)/tools/testing/selftests -name config | \
- xargs $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
+ xargs $(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
$(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig

# ---------------------------------------------------------------------------
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index edccdb77c53e..fb07804b7fc1 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -65,7 +65,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
# Generate VDSO offsets using helper script
gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
quiet_cmd_vdsosym = VDSOSYM $@
- cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
+ cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@

include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
$(call if_changed,vdsosym)
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 7f96a1a9f68c..617c9ac58156 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -205,7 +205,7 @@ quiet_cmd_vdsomunge = MUNGE $@
gen-vdsosym := $(srctree)/$(src)/../vdso/gen_vdso_offsets.sh
quiet_cmd_vdsosym = VDSOSYM $@
# The AArch64 nm should be able to read an AArch32 binary
- cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
+ cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@

# Install commands for the unstripped file
quiet_cmd_vdso_install = INSTALL32 $@
diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
index 703b1c4f6d12..86d42a2d09cb 100644
--- a/arch/ia64/Makefile
+++ b/arch/ia64/Makefile
@@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
-falign-functions=32 -frename-registers -fno-optimize-sibling-calls
KBUILD_CFLAGS_KERNEL := -mconstant-gp

-GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
-KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
+GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
+KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")

ifeq ($(GAS_STATUS),buggy)
$(error Sorry, you need a newer version of the assember, one that is built from \
diff --git a/arch/nds32/kernel/vdso/Makefile b/arch/nds32/kernel/vdso/Makefile
index 55df25ef0057..e77d4bcfa7c1 100644
--- a/arch/nds32/kernel/vdso/Makefile
+++ b/arch/nds32/kernel/vdso/Makefile
@@ -39,7 +39,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
# Generate VDSO offsets using helper script
gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
quiet_cmd_vdsosym = VDSOSYM $@
- cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
+ cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@

include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
$(call if_changed,vdsosym)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index a467b9323442..893217ee4a17 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -104,7 +104,7 @@ else ifeq ($(KBUILD_CHECKSRC),2)
endif

ifneq ($(KBUILD_EXTRA_WARN),)
- cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $<
+ cmd_checkdoc = $(PERL) $(srctree)/scripts/kernel-doc -none $<
endif

# Compile C sources (.c)
diff --git a/scripts/Makefile.package b/scripts/Makefile.package
index f952fb64789d..4fc16c4776cc 100644
--- a/scripts/Makefile.package
+++ b/scripts/Makefile.package
@@ -44,7 +44,7 @@ if test "$(objtree)" != "$(srctree)"; then \
echo >&2; \
false; \
fi ; \
-$(srctree)/scripts/setlocalversion --save-scmversion; \
+$(CONFIG_SHELL) $(srctree)/scripts/setlocalversion --save-scmversion; \
tar -I $(KGZIP) -c $(RCS_TAR_IGNORE) -f $(2).tar.gz \
--transform 's:^:$(2)/:S' $(TAR_CONTENT) $(3); \
rm -f $(objtree)/.scmversion
@@ -123,7 +123,7 @@ git --git-dir=$(srctree)/.git archive --prefix=$(perf-tar)/ \
mkdir -p $(perf-tar); \
git --git-dir=$(srctree)/.git rev-parse HEAD > $(perf-tar)/HEAD; \
(cd $(srctree)/tools/perf; \
-util/PERF-VERSION-GEN $(CURDIR)/$(perf-tar)/); \
+$(CONFIG_SHELL) util/PERF-VERSION-GEN $(CURDIR)/$(perf-tar)/); \
tar rf $(perf-tar).tar $(perf-tar)/HEAD $(perf-tar)/PERF-VERSION-FILE; \
rm -r $(perf-tar); \
$(if $(findstring tar-src,$@),, \
--
2.25.1

2020-10-12 17:13:51

by Ujjwal Kumar

[permalink] [raw]

Subject: [PATCH v2 1/2] kconfig: use interpreters to invoke scripts

2020-10-12 18:22:23

by Lukas Bulwahn

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] kbuild: use interpreters to invoke scripts

On Mon, 12 Oct 2020, Ujjwal Kumar wrote:

> We cannot rely on execute bits to be set on files in the repository.
> The build script should use the explicit interpreter when invoking any
> script from the repository.
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
>
> Suggested-by: Andrew Morton <[email protected]>
> Suggested-by: Kees Cook <[email protected]>
> Suggested-by: Lukas Bulwahn <[email protected]>
> Signed-off-by: Ujjwal Kumar <[email protected]>
> ---
> Makefile | 4 ++--
> arch/arm64/kernel/vdso/Makefile | 2 +-
> arch/arm64/kernel/vdso32/Makefile | 2 +-
> arch/ia64/Makefile | 4 ++--
> arch/nds32/kernel/vdso/Makefile | 2 +-
> scripts/Makefile.build | 2 +-
> scripts/Makefile.package | 4 ++--
> 7 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 0af7945caa61..df20e71dd7c8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1256,7 +1256,7 @@ include/generated/utsrelease.h: include/config/kernel.release FORCE
> PHONY += headerdep
> headerdep:
> $(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
> - $(srctree)/scripts/headerdep.pl -I$(srctree)/include
> + $(PERL) $(srctree)/scripts/headerdep.pl -I$(srctree)/include
>
> # ---------------------------------------------------------------------------
> # Kernel headers
> @@ -1312,7 +1312,7 @@ PHONY += kselftest-merge
> kselftest-merge:
> $(if $(wildcard $(objtree)/.config),, $(error No .config exists, config your kernel first!))
> $(Q)find $(srctree)/tools/testing/selftests -name config | \
> - xargs $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
> + xargs $(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
> $(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig
>
> # ---------------------------------------------------------------------------
> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
> index edccdb77c53e..fb07804b7fc1 100644
> --- a/arch/arm64/kernel/vdso/Makefile
> +++ b/arch/arm64/kernel/vdso/Makefile
> @@ -65,7 +65,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
> # Generate VDSO offsets using helper script
> gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
> quiet_cmd_vdsosym = VDSOSYM $@
> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
>
> include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
> $(call if_changed,vdsosym)
> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
> index 7f96a1a9f68c..617c9ac58156 100644
> --- a/arch/arm64/kernel/vdso32/Makefile
> +++ b/arch/arm64/kernel/vdso32/Makefile
> @@ -205,7 +205,7 @@ quiet_cmd_vdsomunge = MUNGE $@
> gen-vdsosym := $(srctree)/$(src)/../vdso/gen_vdso_offsets.sh
> quiet_cmd_vdsosym = VDSOSYM $@
> # The AArch64 nm should be able to read an AArch32 binary
> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
>
> # Install commands for the unstripped file
> quiet_cmd_vdso_install = INSTALL32 $@
> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
> index 703b1c4f6d12..86d42a2d09cb 100644
> --- a/arch/ia64/Makefile
> +++ b/arch/ia64/Makefile
> @@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
> -falign-functions=32 -frename-registers -fno-optimize-sibling-calls
> KBUILD_CFLAGS_KERNEL := -mconstant-gp
>
> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> +GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> +KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")

Here is an instance of what Masahiro-san pointed out being wrong.

Ujjwal, will you send a v3?

>
> ifeq ($(GAS_STATUS),buggy)
> $(error Sorry, you need a newer version of the assember, one that is built from \
> diff --git a/arch/nds32/kernel/vdso/Makefile b/arch/nds32/kernel/vdso/Makefile
> index 55df25ef0057..e77d4bcfa7c1 100644
> --- a/arch/nds32/kernel/vdso/Makefile
> +++ b/arch/nds32/kernel/vdso/Makefile
> @@ -39,7 +39,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
> # Generate VDSO offsets using helper script
> gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
> quiet_cmd_vdsosym = VDSOSYM $@
> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
>
> include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
> $(call if_changed,vdsosym)
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index a467b9323442..893217ee4a17 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -104,7 +104,7 @@ else ifeq ($(KBUILD_CHECKSRC),2)
> endif
>
> ifneq ($(KBUILD_EXTRA_WARN),)
> - cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $<
> + cmd_checkdoc = $(PERL) $(srctree)/scripts/kernel-doc -none $<
> endif
>
> # Compile C sources (.c)
> diff --git a/scripts/Makefile.package b/scripts/Makefile.package
> index f952fb64789d..4fc16c4776cc 100644
> --- a/scripts/Makefile.package
> +++ b/scripts/Makefile.package
> @@ -44,7 +44,7 @@ if test "$(objtree)" != "$(srctree)"; then \
> echo >&2; \
> false; \
> fi ; \
> -$(srctree)/scripts/setlocalversion --save-scmversion; \
> +$(CONFIG_SHELL) $(srctree)/scripts/setlocalversion --save-scmversion; \
> tar -I $(KGZIP) -c $(RCS_TAR_IGNORE) -f $(2).tar.gz \
> --transform 's:^:$(2)/:S' $(TAR_CONTENT) $(3); \
> rm -f $(objtree)/.scmversion
> @@ -123,7 +123,7 @@ git --git-dir=$(srctree)/.git archive --prefix=$(perf-tar)/ \
> mkdir -p $(perf-tar); \
> git --git-dir=$(srctree)/.git rev-parse HEAD > $(perf-tar)/HEAD; \
> (cd $(srctree)/tools/perf; \
> -util/PERF-VERSION-GEN $(CURDIR)/$(perf-tar)/); \
> +$(CONFIG_SHELL) util/PERF-VERSION-GEN $(CURDIR)/$(perf-tar)/); \
> tar rf $(perf-tar).tar $(perf-tar)/HEAD $(perf-tar)/PERF-VERSION-FILE; \
> rm -r $(perf-tar); \
> $(if $(findstring tar-src,$@),, \
> --
> 2.25.1
>
>

2020-10-12 18:46:44

by Ujjwal Kumar

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] kbuild: use interpreters to invoke scripts

On 12/10/20 11:50 pm, Lukas Bulwahn wrote:
>
>
> On Mon, 12 Oct 2020, Ujjwal Kumar wrote:
>
>> We cannot rely on execute bits to be set on files in the repository.
>> The build script should use the explicit interpreter when invoking any
>> script from the repository.
>>
>> Link: https://lore.kernel.org/lkml/[email protected]/
>> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
>>
>> Suggested-by: Andrew Morton <[email protected]>
>> Suggested-by: Kees Cook <[email protected]>
>> Suggested-by: Lukas Bulwahn <[email protected]>
>> Signed-off-by: Ujjwal Kumar <[email protected]>
>> ---
>> Makefile | 4 ++--
>> arch/arm64/kernel/vdso/Makefile | 2 +-
>> arch/arm64/kernel/vdso32/Makefile | 2 +-
>> arch/ia64/Makefile | 4 ++--
>> arch/nds32/kernel/vdso/Makefile | 2 +-
>> scripts/Makefile.build | 2 +-
>> scripts/Makefile.package | 4 ++--
>> 7 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 0af7945caa61..df20e71dd7c8 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1256,7 +1256,7 @@ include/generated/utsrelease.h: include/config/kernel.release FORCE
>> PHONY += headerdep
>> headerdep:
>> $(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
>> - $(srctree)/scripts/headerdep.pl -I$(srctree)/include
>> + $(PERL) $(srctree)/scripts/headerdep.pl -I$(srctree)/include
>>
>> # ---------------------------------------------------------------------------
>> # Kernel headers
>> @@ -1312,7 +1312,7 @@ PHONY += kselftest-merge
>> kselftest-merge:
>> $(if $(wildcard $(objtree)/.config),, $(error No .config exists, config your kernel first!))
>> $(Q)find $(srctree)/tools/testing/selftests -name config | \
>> - xargs $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
>> + xargs $(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
>> $(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig
>>
>> # ---------------------------------------------------------------------------
>> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
>> index edccdb77c53e..fb07804b7fc1 100644
>> --- a/arch/arm64/kernel/vdso/Makefile
>> +++ b/arch/arm64/kernel/vdso/Makefile
>> @@ -65,7 +65,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
>> # Generate VDSO offsets using helper script
>> gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
>> quiet_cmd_vdsosym = VDSOSYM $@
>> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
>> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
>>
>> include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
>> $(call if_changed,vdsosym)
>> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
>> index 7f96a1a9f68c..617c9ac58156 100644
>> --- a/arch/arm64/kernel/vdso32/Makefile
>> +++ b/arch/arm64/kernel/vdso32/Makefile
>> @@ -205,7 +205,7 @@ quiet_cmd_vdsomunge = MUNGE $@
>> gen-vdsosym := $(srctree)/$(src)/../vdso/gen_vdso_offsets.sh
>> quiet_cmd_vdsosym = VDSOSYM $@
>> # The AArch64 nm should be able to read an AArch32 binary
>> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
>> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
>>
>> # Install commands for the unstripped file
>> quiet_cmd_vdso_install = INSTALL32 $@
>> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
>> index 703b1c4f6d12..86d42a2d09cb 100644
>> --- a/arch/ia64/Makefile
>> +++ b/arch/ia64/Makefile
>> @@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
>> -falign-functions=32 -frename-registers -fno-optimize-sibling-calls
>> KBUILD_CFLAGS_KERNEL := -mconstant-gp
>>
>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>> +GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>> +KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>
> Here is an instance of what Masahiro-san pointed out being wrong.
>
> Ujjwal, will you send a v3?

Following is the quoted text from the reply mail from Masahiro

>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>> +GAS_STATUS = $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>> +KBUILD_CPPFLAGS += $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>
>
>
> These changes look wrong to me.
>
> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>

From the above text, I understand as follows:

That my proposed change:
$(shell $(src...) -> $($(CONFIG_SHELL) $(src...)

is WRONG

and in the next line he suggested the required correction.
That being:
$($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)

Which is in v2 of the patch series.

Lukas, please correct me if I'm wrong so that I can work on v3
if required.

Also, Nathan reviewed both the patches in v1 of this series. So,
should I be the one who adds his tag in next iterations?

Thanks
Ujjwal Kumar

2020-10-12 18:55:06

by Lukas Bulwahn

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] kbuild: use interpreters to invoke scripts

On Tue, 13 Oct 2020, Ujjwal Kumar wrote:

> On 12/10/20 11:50 pm, Lukas Bulwahn wrote:
> >
> >
> > On Mon, 12 Oct 2020, Ujjwal Kumar wrote:
> >
> >> We cannot rely on execute bits to be set on files in the repository.
> >> The build script should use the explicit interpreter when invoking any
> >> script from the repository.
> >>
> >> Link: https://lore.kernel.org/lkml/[email protected]/
> >> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
> >>
> >> Suggested-by: Andrew Morton <[email protected]>
> >> Suggested-by: Kees Cook <[email protected]>
> >> Suggested-by: Lukas Bulwahn <[email protected]>
> >> Signed-off-by: Ujjwal Kumar <[email protected]>
> >> ---
> >> Makefile | 4 ++--
> >> arch/arm64/kernel/vdso/Makefile | 2 +-
> >> arch/arm64/kernel/vdso32/Makefile | 2 +-
> >> arch/ia64/Makefile | 4 ++--
> >> arch/nds32/kernel/vdso/Makefile | 2 +-
> >> scripts/Makefile.build | 2 +-
> >> scripts/Makefile.package | 4 ++--
> >> 7 files changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index 0af7945caa61..df20e71dd7c8 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -1256,7 +1256,7 @@ include/generated/utsrelease.h: include/config/kernel.release FORCE
> >> PHONY += headerdep
> >> headerdep:
> >> $(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
> >> - $(srctree)/scripts/headerdep.pl -I$(srctree)/include
> >> + $(PERL) $(srctree)/scripts/headerdep.pl -I$(srctree)/include
> >>
> >> # ---------------------------------------------------------------------------
> >> # Kernel headers
> >> @@ -1312,7 +1312,7 @@ PHONY += kselftest-merge
> >> kselftest-merge:
> >> $(if $(wildcard $(objtree)/.config),, $(error No .config exists, config your kernel first!))
> >> $(Q)find $(srctree)/tools/testing/selftests -name config | \
> >> - xargs $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
> >> + xargs $(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
> >> $(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig
> >>
> >> # ---------------------------------------------------------------------------
> >> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
> >> index edccdb77c53e..fb07804b7fc1 100644
> >> --- a/arch/arm64/kernel/vdso/Makefile
> >> +++ b/arch/arm64/kernel/vdso/Makefile
> >> @@ -65,7 +65,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
> >> # Generate VDSO offsets using helper script
> >> gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
> >> quiet_cmd_vdsosym = VDSOSYM $@
> >> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> >> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
> >>
> >> include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
> >> $(call if_changed,vdsosym)
> >> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
> >> index 7f96a1a9f68c..617c9ac58156 100644
> >> --- a/arch/arm64/kernel/vdso32/Makefile
> >> +++ b/arch/arm64/kernel/vdso32/Makefile
> >> @@ -205,7 +205,7 @@ quiet_cmd_vdsomunge = MUNGE $@
> >> gen-vdsosym := $(srctree)/$(src)/../vdso/gen_vdso_offsets.sh
> >> quiet_cmd_vdsosym = VDSOSYM $@
> >> # The AArch64 nm should be able to read an AArch32 binary
> >> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> >> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
> >>
> >> # Install commands for the unstripped file
> >> quiet_cmd_vdso_install = INSTALL32 $@
> >> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
> >> index 703b1c4f6d12..86d42a2d09cb 100644
> >> --- a/arch/ia64/Makefile
> >> +++ b/arch/ia64/Makefile
> >> @@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
> >> -falign-functions=32 -frename-registers -fno-optimize-sibling-calls
> >> KBUILD_CFLAGS_KERNEL := -mconstant-gp
> >>
> >> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >> +GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >> +KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >
> > Here is an instance of what Masahiro-san pointed out being wrong.
> >
> > Ujjwal, will you send a v3?
>
> Following is the quoted text from the reply mail from Masahiro
>
> >> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >> +GAS_STATUS = $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >> +KBUILD_CPPFLAGS += $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >
> >
> >
> > These changes look wrong to me.
> >
> > $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
> >
>
> From the above text, I understand as follows:
>
> That my proposed change:
> $(shell $(src...) -> $($(CONFIG_SHELL) $(src...)
>
> is WRONG
>
> and in the next line he suggested the required correction.
> That being:
> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>
> Which is in v2 of the patch series.
>
> Lukas, please correct me if I'm wrong so that I can work on v3
> if required.
>

Sorry, my memory tricked me; I got it confused. Your patch looks good.

> Also, Nathan reviewed both the patches in v1 of this series. So,
> should I be the one who adds his tag in next iterations?
>

Masahiro-san will probably just add them when he picks the patches.

Lukas

2020-10-12 19:10:22

by Bernd Petrovitsch

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] kbuild: use interpreters to invoke scripts

Hi all!

On 12/10/2020 18:42, Ujjwal Kumar wrote:
> On 12/10/20 11:50 pm, Lukas Bulwahn wrote:
>>
>>
>> On Mon, 12 Oct 2020, Ujjwal Kumar wrote:
>>
>>> We cannot rely on execute bits to be set on files in the repository.
>>> The build script should use the explicit interpreter when invoking any
>>> script from the repository.
>>>
>>> Link: https://lore.kernel.org/lkml/[email protected]/
>>> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
>>>
>>> Suggested-by: Andrew Morton <[email protected]>
>>> Suggested-by: Kees Cook <[email protected]>
>>> Suggested-by: Lukas Bulwahn <[email protected]>
>>> Signed-off-by: Ujjwal Kumar <[email protected]>
>>> ---
>>> Makefile | 4 ++--
>>> arch/arm64/kernel/vdso/Makefile | 2 +-
>>> arch/arm64/kernel/vdso32/Makefile | 2 +-
>>> arch/ia64/Makefile | 4 ++--
>>> arch/nds32/kernel/vdso/Makefile | 2 +-
>>> scripts/Makefile.build | 2 +-
>>> scripts/Makefile.package | 4 ++--
>>> 7 files changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 0af7945caa61..df20e71dd7c8 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -1256,7 +1256,7 @@ include/generated/utsrelease.h: include/config/kernel.release FORCE
>>> PHONY += headerdep
>>> headerdep:
>>> $(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
>>> - $(srctree)/scripts/headerdep.pl -I$(srctree)/include
>>> + $(PERL) $(srctree)/scripts/headerdep.pl -I$(srctree)/include
>>>
>>> # ---------------------------------------------------------------------------
>>> # Kernel headers
>>> @@ -1312,7 +1312,7 @@ PHONY += kselftest-merge
>>> kselftest-merge:
>>> $(if $(wildcard $(objtree)/.config),, $(error No .config exists, config your kernel first!))
>>> $(Q)find $(srctree)/tools/testing/selftests -name config | \
>>> - xargs $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
>>> + xargs $(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
>>> $(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig
>>>
>>> # ---------------------------------------------------------------------------
>>> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
>>> index edccdb77c53e..fb07804b7fc1 100644
>>> --- a/arch/arm64/kernel/vdso/Makefile
>>> +++ b/arch/arm64/kernel/vdso/Makefile
>>> @@ -65,7 +65,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
>>> # Generate VDSO offsets using helper script
>>> gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
>>> quiet_cmd_vdsosym = VDSOSYM $@
>>> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
>>> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
>>>
>>> include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
>>> $(call if_changed,vdsosym)
>>> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
>>> index 7f96a1a9f68c..617c9ac58156 100644
>>> --- a/arch/arm64/kernel/vdso32/Makefile
>>> +++ b/arch/arm64/kernel/vdso32/Makefile
>>> @@ -205,7 +205,7 @@ quiet_cmd_vdsomunge = MUNGE $@
>>> gen-vdsosym := $(srctree)/$(src)/../vdso/gen_vdso_offsets.sh
>>> quiet_cmd_vdsosym = VDSOSYM $@
>>> # The AArch64 nm should be able to read an AArch32 binary
>>> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
>>> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
>>>
>>> # Install commands for the unstripped file
>>> quiet_cmd_vdso_install = INSTALL32 $@
>>> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
>>> index 703b1c4f6d12..86d42a2d09cb 100644
>>> --- a/arch/ia64/Makefile
>>> +++ b/arch/ia64/Makefile
>>> @@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
>>> -falign-functions=32 -frename-registers -fno-optimize-sibling-calls
>>> KBUILD_CFLAGS_KERNEL := -mconstant-gp
>>>
>>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>> +GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>> +KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>
>> Here is an instance of what Masahiro-san pointed out being wrong.
>>
>> Ujjwal, will you send a v3?
>
> Following is the quoted text from the reply mail from Masahiro
>
>>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>> +GAS_STATUS = $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>> +KBUILD_CPPFLAGS += $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>
>>
>>
>> These changes look wrong to me.
>>
>> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>>
>
> From the above text, I understand as follows:

Did you actually *test* that (expecially) these lines work
afterwards as good as before?

> That my proposed change:
> $(shell $(src...) -> $($(CONFIG_SHELL) $(src...)
>
> is WRONG

Yup, as it's in a Makefile and that's a Makefile construct.

> and in the next line he suggested the required correction.
> That being:
> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)

Such stuff should generally not be needed as the to-be-used
shell can be set in Makefiles via a "SHELL = " assignment
(defaulting to /bin/sh - what else;-).
Flags for the shell can BTW set with ".SHELLFLAGS = ".

So please
-) learn basic "Makefile" + "make" before brainlessly patching
a Makefile.
-) actually testy your changes to make sure the patch didn't
broke anything
-) and - last but not least - check if there isn't a shell
already set (and which).

MfG,
Bernd
--
There is no cloud, just other people computers.
-- https://static.fsf.org/nosvn/stickers/thereisnocloud.svg

Attachments:

pEpkey.asc (3.09 kB)

2020-10-13 14:19:23

by Ujjwal Kumar

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] kbuild: use interpreters to invoke scripts

On 13/10/20 12:24 am, Bernd Petrovitsch wrote:
> Hi all!
>
>>>> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
>>>> index 703b1c4f6d12..86d42a2d09cb 100644
>>>> --- a/arch/ia64/Makefile
>>>> +++ b/arch/ia64/Makefile
>>>> @@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
>>>> -falign-functions=32 -frename-registers -fno-optimize-sibling-calls
>>>> KBUILD_CFLAGS_KERNEL := -mconstant-gp
>>>>
>>>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>> +GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> +KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>
>>> Here is an instance of what Masahiro-san pointed out being wrong.
>>>
>>> Ujjwal, will you send a v3?
>>
>> Following is the quoted text from the reply mail from Masahiro
>>
>>>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>> +GAS_STATUS = $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
>>>> +KBUILD_CPPFLAGS += $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
>>>
>>>
>>>
>>> These changes look wrong to me.
>>>
>>> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>>>
>>
>> From the above text, I understand as follows:
>
> Did you actually *test* that (expecially) these lines work
> afterwards as good as before?

Yes, I did check my changes. TBH, I spent a considerable
amount of time in doing so (given that I'm new to the
community). And I explicitly mentioned the ones I couldn't
test in the cover letter.

But I'm afraid this particular change that Masahiro pointed
must have been overlooked by me (and possibly by others
involved in the process). Being the author of the patch I
accept my mistake.

Because this construct was new to me I read about it
thoroughly in the docs.
As soon as it was pointed out to me, I at once realised
that the change proposed by me was wrong (i didn't
have to look at the docs).

>
>> That my proposed change:
>> $(shell $(src...) -> $($(CONFIG_SHELL) $(src...)
>>
>> is WRONG
>
> Yup, as it's in a Makefile and that's a Makefile construct>
>> and in the next line he suggested the required correction.
>> That being:
>> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>
> Such stuff should generally not be needed as the to-be-used
> shell can be set in Makefiles via a "SHELL = " assignment

It's not about setting shell but rather using it at required
place. The 'shell function' is meant to execute provided
commands in an environment outside of make; and executing
commands in that environment is somewhat similar to running
commands on a terminal.
Invoking a script file without setting the x bits will give
a permission denied error.
Similar thing happens when 'shell function' tries to invoke
the provided script. So the task was simply to prepend the
$CONFIG_SHELL (or $SHELL whichever is configured; simple sh
would also suffice) with the script file in 'shell function'.

> (defaulting to /bin/sh - what else;-).
> Flags for the shell can BTW set with ".SHELLFLAGS = ".

setting flags might not be the solution either.

>
> So please
> -) learn basic "Makefile" + "make" before brainlessly patching
> a Makefile.
> -) actually testy your changes to make sure the patch didn't
> broke anything
> -) and - last but not least - check if there isn't a shell
> already set (and which).

btw, I do agree with your points.

>
> MfG,
> Bernd
>

If I said anything incorrect please correct me.

Thanks
Ujjwal Kumar

2020-10-14 00:21:38

by Masahiro Yamada

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] kbuild: use interpreters to invoke scripts

On Tue, Oct 13, 2020 at 4:03 AM Bernd Petrovitsch
<[email protected]> wrote:
>
> Hi all!
>
> On 12/10/2020 18:42, Ujjwal Kumar wrote:
> > On 12/10/20 11:50 pm, Lukas Bulwahn wrote:
> >>
> >>
> >> On Mon, 12 Oct 2020, Ujjwal Kumar wrote:
> >>
> >>> We cannot rely on execute bits to be set on files in the repository.
> >>> The build script should use the explicit interpreter when invoking any
> >>> script from the repository.
> >>>
> >>> Link: https://lore.kernel.org/lkml/[email protected]/
> >>> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
> >>>
> >>> Suggested-by: Andrew Morton <[email protected]>
> >>> Suggested-by: Kees Cook <[email protected]>
> >>> Suggested-by: Lukas Bulwahn <[email protected]>
> >>> Signed-off-by: Ujjwal Kumar <[email protected]>
> >>> ---
> >>> Makefile | 4 ++--
> >>> arch/arm64/kernel/vdso/Makefile | 2 +-
> >>> arch/arm64/kernel/vdso32/Makefile | 2 +-
> >>> arch/ia64/Makefile | 4 ++--
> >>> arch/nds32/kernel/vdso/Makefile | 2 +-
> >>> scripts/Makefile.build | 2 +-
> >>> scripts/Makefile.package | 4 ++--
> >>> 7 files changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/Makefile b/Makefile
> >>> index 0af7945caa61..df20e71dd7c8 100644
> >>> --- a/Makefile
> >>> +++ b/Makefile
> >>> @@ -1256,7 +1256,7 @@ include/generated/utsrelease.h: include/config/kernel.release FORCE
> >>> PHONY += headerdep
> >>> headerdep:
> >>> $(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
> >>> - $(srctree)/scripts/headerdep.pl -I$(srctree)/include
> >>> + $(PERL) $(srctree)/scripts/headerdep.pl -I$(srctree)/include
> >>>
> >>> # ---------------------------------------------------------------------------
> >>> # Kernel headers
> >>> @@ -1312,7 +1312,7 @@ PHONY += kselftest-merge
> >>> kselftest-merge:
> >>> $(if $(wildcard $(objtree)/.config),, $(error No .config exists, config your kernel first!))
> >>> $(Q)find $(srctree)/tools/testing/selftests -name config | \
> >>> - xargs $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
> >>> + xargs $(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m $(objtree)/.config
> >>> $(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig
> >>>
> >>> # ---------------------------------------------------------------------------
> >>> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
> >>> index edccdb77c53e..fb07804b7fc1 100644
> >>> --- a/arch/arm64/kernel/vdso/Makefile
> >>> +++ b/arch/arm64/kernel/vdso/Makefile
> >>> @@ -65,7 +65,7 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
> >>> # Generate VDSO offsets using helper script
> >>> gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
> >>> quiet_cmd_vdsosym = VDSOSYM $@
> >>> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> >>> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
> >>>
> >>> include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
> >>> $(call if_changed,vdsosym)
> >>> diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
> >>> index 7f96a1a9f68c..617c9ac58156 100644
> >>> --- a/arch/arm64/kernel/vdso32/Makefile
> >>> +++ b/arch/arm64/kernel/vdso32/Makefile
> >>> @@ -205,7 +205,7 @@ quiet_cmd_vdsomunge = MUNGE $@
> >>> gen-vdsosym := $(srctree)/$(src)/../vdso/gen_vdso_offsets.sh
> >>> quiet_cmd_vdsosym = VDSOSYM $@
> >>> # The AArch64 nm should be able to read an AArch32 binary
> >>> - cmd_vdsosym = $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
> >>> + cmd_vdsosym = $(NM) $< | $(CONFIG_SHELL) $(gen-vdsosym) | LC_ALL=C sort > $@
> >>>
> >>> # Install commands for the unstripped file
> >>> quiet_cmd_vdso_install = INSTALL32 $@
> >>> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
> >>> index 703b1c4f6d12..86d42a2d09cb 100644
> >>> --- a/arch/ia64/Makefile
> >>> +++ b/arch/ia64/Makefile
> >>> @@ -27,8 +27,8 @@ cflags-y := -pipe $(EXTRA) -ffixed-r13 -mfixed-range=f12-f15,f32-f127 \
> >>> -falign-functions=32 -frename-registers -fno-optimize-sibling-calls
> >>> KBUILD_CFLAGS_KERNEL := -mconstant-gp
> >>>
> >>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >>> +GAS_STATUS = $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >>> +KBUILD_CPPFLAGS += $(shell $(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >>
> >> Here is an instance of what Masahiro-san pointed out being wrong.
> >>
> >> Ujjwal, will you send a v3?
> >
> > Following is the quoted text from the reply mail from Masahiro
> >
> >>> -GAS_STATUS = $(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >>> -KBUILD_CPPFLAGS += $(shell $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >>> +GAS_STATUS = $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")
> >>> +KBUILD_CPPFLAGS += $($(CONFIG_SHELL) $(srctree)/arch/ia64/scripts/toolchain-flags "$(CC)" "$(OBJDUMP)" "$(READELF)")
> >>
> >>
> >>
> >> These changes look wrong to me.
> >>
> >> $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
> >>
> >
> > From the above text, I understand as follows:
>
> Did you actually *test* that (expecially) these lines work
> afterwards as good as before?
>
> > That my proposed change:
> > $(shell $(src...) -> $($(CONFIG_SHELL) $(src...)
> >
> > is WRONG
>
> Yup, as it's in a Makefile and that's a Makefile construct.
>
> > and in the next line he suggested the required correction.
> > That being:
> > $($(CONFIG_SHELL) -> $(shell $(CONFIG_SHELL)
>
> Such stuff should generally not be needed as the to-be-used
> shell can be set in Makefiles via a "SHELL = " assignment
> (defaulting to /bin/sh - what else;-).
> Flags for the shell can BTW set with ".SHELLFLAGS = ".

You are talking about a different thing.

Take the current code as an example:

$(shell $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")

Here are two shell invocations.

[1]
The command
$(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)"
is run in /bin/sh because the default value of SHELL is /bin/sh.

[2]
The script, arch/ia64/scripts/check-gas, is run in /bin/sh
because the hash-bang (the first line of check-gas)
specifies #!/bin/sh

Bernd is talking about [1].

In contrast, this patch is addressing [2] because
Andrew Morton suggested to run scripts without relying
on the executable bit.
(and, after this patch, we run scripts without relying
on the hash-bang because we now specify the interpreter.)

Of course, [1] and [2] can be different.

I always want to use /bin/sh for [1],
so please do not use bash-extension inside $(shell ...)

You have more choices for [2].

If arch/ia64/scripts/check-gas had been written with bash-extension,
the code would have been changed into:

$(shell $(BASH) $(srctree)/arch/ia64/scripts/check-gas "$(CC)" "$(OBJDUMP)")

I hope this will be clearer.

> So please
> -) learn basic "Makefile" + "make" before brainlessly patching
> a Makefile.
> -) actually testy your changes to make sure the patch didn't
> broke anything
> -) and - last but not least - check if there isn't a shell
> already set (and which).
>
> MfG,
> Bernd
> --
> There is no cloud, just other people computers.
> -- https://static.fsf.org/nosvn/stickers/thereisnocloud.svg

--
Best Regards
Masahiro Yamada

2020-10-14 03:04:22

by Masahiro Yamada

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] kconfig: use interpreters to invoke scripts

On Tue, Oct 13, 2020 at 2:08 AM Ujjwal Kumar <[email protected]> wrote:
>
> We cannot rely on execute bits to be set on files in the repository.
> The build script should use the explicit interpreter when invoking any
> script from the repository.
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
>
> Suggested-by: Andrew Morton <[email protected]>
> Suggested-by: Kees Cook <[email protected]>
> Suggested-by: Lukas Bulwahn <[email protected]>
> Signed-off-by: Ujjwal Kumar <[email protected]>
> ---

The patch prefix 'kconfig:' should be used for changes
under scripts/kconfig/.

I want to see both prefixed with "kbuild:".

1/2: kbuild: use interpreters in Kconfig files to invoke scripts
2/2: kbuild: use interpreters in Makefiles to invoke scripts

More preferably, those two patches should be squashed into a
single patch titled "kbuild: use interpreters to invoke scripts"

BTW, I notice some code left unconverted.

For example,
https://github.com/torvalds/linux/blob/master/init/Kconfig#L68
https://github.com/torvalds/linux/blob/v5.9/certs/Makefile#L25

Maybe more...

I know it is difficult to cover everything, but please
re-check the remaining code.

> init/Kconfig | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index c9446911cf41..8adf3194d26f 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -30,12 +30,12 @@ config CC_IS_GCC
>
> config GCC_VERSION
> int
> - default $(shell,$(srctree)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
> + default $(shell,$(CONFIG_SHELL) $(srctree)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
> default 0
>
> config LD_VERSION
> int
> - default $(shell,$(LD) --version | $(srctree)/scripts/ld-version.sh)
> + default $(shell,$(LD) --version | $(AWK) -f $(srctree)/scripts/ld-version.sh)
>
> config CC_IS_CLANG
> def_bool $(success,echo "$(CC_VERSION_TEXT)" | grep -q clang)
> @@ -45,20 +45,20 @@ config LD_IS_LLD
>
> config CLANG_VERSION
> int
> - default $(shell,$(srctree)/scripts/clang-version.sh $(CC))
> + default $(shell,$(CONFIG_SHELL) $(srctree)/scripts/clang-version.sh $(CC))
>
> config CC_CAN_LINK
> bool
> - default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag)) if 64BIT
> - default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag))
> + default $(success,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag)) if 64BIT
> + default $(success,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag))
>
> config CC_CAN_LINK_STATIC
> bool
> - default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag) -static) if 64BIT
> - default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag) -static)
> + default $(success,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m64-flag) -static) if 64BIT
> + default $(success,$(CONFIG_SHELL) $(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(m32-flag) -static)
>
> config CC_HAS_ASM_GOTO
> - def_bool $(success,$(srctree)/scripts/gcc-goto.sh $(CC))
> + def_bool $(success,$(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC))
>
> config CC_HAS_ASM_GOTO_OUTPUT
> depends on CC_HAS_ASM_GOTO
> --
> 2.25.1
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201012170631.1241502-2-ujjwalkumar0501%40gmail.com.

--
Best Regards
Masahiro Yamada