Re: [Linux-kernel-mentees] [PATCH RFC] kbuild: use interpreters to invoke scripts

On 01/10/20 7:39 pm, Lukas Bulwahn wrote:

On Thu, 1 Oct 2020, Ujjwal Kumar wrote:

quoted

On 01/10/20 4:38 pm, Lukas Bulwahn wrote:

quoted

On Thu, 1 Oct 2020, Ujjwal Kumar wrote:

quoted

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/20200830174409.c24c3f67addcce0cea9a9d4c@linux-foundation.org/ (local)
Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/ (local)

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Suggested-by: Kees Cook <redacted>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Ujjwal Kumar <redacted>
---
 Documentation/Makefile                           | 16 ++++++++--------
 Documentation/userspace-api/media/Makefile       |  6 +++---
 Makefile                                         |  4 ++--
 arch/arm64/kernel/vdso/Makefile                  |  2 +-
 arch/arm64/kernel/vdso32/Makefile                |  2 +-
 arch/ia64/Makefile                               |  4 ++--
 arch/nds32/kernel/vdso/Makefile                  |  2 +-
 .../comedi/drivers/ni_routing/tools/Makefile     |  6 +++---
 scripts/Makefile.build                           |  2 +-
 scripts/Makefile.package                         |  4 ++--
 tools/bootconfig/Makefile                        |  2 +-
 tools/bpf/Makefile.helpers                       |  2 +-
 tools/lib/bpf/Makefile                           |  2 +-
 tools/perf/Makefile.perf                         |  2 +-
 tools/power/cpupower/Makefile                    |  2 +-
 tools/testing/selftests/rcutorture/Makefile      |  2 +-
 tools/testing/selftests/rseq/Makefile            |  2 +-
 tools/testing/selftests/vm/Makefile              |  6 +++---
 tools/testing/selftests/wireguard/qemu/Makefile  |  4 ++--
 tools/testing/selftests/x86/Makefile             |  6 +++---
 20 files changed, 39 insertions(+), 39 deletions(-)

You will probably need to split this patch into multiple patches, but the 
discussion will show how to split it best.

Probably:

 - one for Documentation
 - one for general kbuild, maybe including arch
 - one for selftests
 - one for the other tools
 - one for the comedi driver

So, what did you do for testing your change?

First, I had to unset the execute bits on all files.
for i in $(find -executable -type f); do chmod -x $i; done

To test the changes, I invoked the make rule under which my current 
changes lie. And I could see a permission-denied whenever a script 
was invoked. After the patch, the make rule progressed without 
any permissions error.
I couldn't test each and every change, because some rules failed 
before invoking the script and others could not be invoked by me.

Which could you not test? Please share that.

arch/arm64/kernel/vdso/Makefile
arch/arm64/kernel/vdso32/Makefile
arch/ia64/Makefile
arch/nds32/kernel/vdso/Makefile
drivers/staging/comedi/drivers/ni_routing/tools/Makefile
scripts/Makefile.build
scripts/Makefile.package
tools/bootconfig/Makefile
tools/bpf/Makefile.helpers
tools/lib/bpf/Makefile
tools/testing/selftests/rcutorture/Makefile
tools/testing/selftests/rseq/Makefile
tools/testing/selftests/vm/Makefile
tools/testing/selftests/wireguard/qemu/Makefile
tools/testing/selftests/x86/Makefile

quoted

I successfully tested the changes under Documentation/ (and some others).

$ make htmldocs
make[1]: execvp: ./scripts/sphinx-pre-install: Permission denied
make[1]: *** [Documentation/Makefile:81: htmldocs] Error 127
make: *** [Makefile:1661: htmldocs] Error 2

Files that I tested my changes on:
Documentation/Makefile                           | 16 ++++++++--------
Documentation/userspace-api/media/Makefile       |  6 +++---
Makefile                                         |  4 ++--
tools/perf/Makefile.perf                         |  2 +-
tools/power/cpupower/Makefile                    |  2 +-

quoted

Did you check if CONFIG_SHELL is actually available in tools?

Yes, I did check.
To verify, I echo(ed) the variables under a make task. And invoked 
the task from srctree.

That sounds good.
 

quoted

quoted

quoted

--- 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 > $@

I guess it is better to modify gen-vdsosym.

Do you mean something as follows:
gen-vdsosym := $(CONFIG_SHELL) $(srctree)/$(src)/gen_vdso_offsets.sh

If so, I have never seen that style in the build files. Infact, the current
style is followed at many places.
For instance,
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/Makefile.lib?h=next-20201001#n398
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/tools/Makefile?h=next-20201001#n50

I searched around as well. I think your proposal is fine.

How about splitting out a first patch on the generic kbuild files and send 
that out as RFC, first send the RFC patch to linux-kernel-mentees list, 
then if that looks good, we go for Masahiro and the linux-kbuild list?

Lukas

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees