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

[Lukas Bulwahn]

On Thu, 1 Oct 2020, Ujjwal Kumar wrote:

> On 01/10/20 4:38 pm, Lukas Bulwahn wrote:
> > 
> > 
> > On Thu, 1 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/20200830174409.c24c3f67addcce0cea9a9d4c@linux-foundation.org/
> >> Link: https://lore.kernel.org/lkml/202008271102.FEB906C88@keescook/
> >>
> >> Suggested-by: Andrew Morton <akpm at linux-foundation.org>
> >> Suggested-by: Kees Cook <keescook at chromium.org>
> >> Suggested-by: Lukas Bulwahn <lukas.bulwahn at gmail.com>
> >> Signed-off-by: Ujjwal Kumar <ujjwalkumar0501 at gmail.com>
> >> ---
> >>  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.

> 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 +-
> 
> 
> > 
> > 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.
 
> >> --- 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