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

[Ujjwal Kumar]

On 01/10/20 7:39 pm, Lukas Bulwahn wrote:
> 
> 
> 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.

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

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