netdev - Re: [PATCH bpf-next v4 2/3] selftests/bpf: Make install target copy test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87wmrruynb.fsf@all.your.base.are.belong.to.us>
Date: Tue, 30 Jan 2024 07:24:24 +0100
From: Björn Töpel <bjorn@...nel.org>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
 <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Mykola
 Lysenko <mykolal@...com>, bpf@...r.kernel.org, netdev@...r.kernel.org,
 Björn Töpel <bjorn@...osinc.com>,
 linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v4 2/3] selftests/bpf: Make install target copy
 test_progs extra files

Andrii Nakryiko <andrii.nakryiko@...il.com> writes:

> On Sun, Jan 28, 2024 at 11:09 PM Björn Töpel <bjorn@...nel.org> wrote:
>>
>> From: Björn Töpel <bjorn@...osinc.com>
>>
>> Currently, "make install" does not install the required test_progs
>> "extra files" (e.g. kernel modules, helper shell scripts, etc.) for
>> the BPF machine flavors (e.g. cpuv4).
>>
>> Add the missing "extra files" dependencies to rsync, called from the
>> install target.
>>
>> Unfortunately, kselftest does not use bash as the default shell, so
>> the globbering is limited. Blindly enabling "SHELL:=/bin/bash" for the
>> Makefile breaks in other places. Workaround by explicitly call
>> "/bin/bash" to expand the file globbing.
>>
>> Signed-off-by: Björn Töpel <bjorn@...osinc.com>
>> ---
>>  tools/testing/selftests/bpf/Makefile | 29 +++++++++++++++++-----------
>>  1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index 830a34f0aa37..c3c5b85f7dae 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -605,14 +605,15 @@ TRUNNER_EXTRA_SOURCES := test_progs.c             \
>>                          json_writer.c          \
>>                          flow_dissector_load.h  \
>>                          ip_check_defrag_frags.h
>> -TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
>> -                      $(OUTPUT)/liburandom_read.so                     \
>> -                      $(OUTPUT)/xdp_synproxy                           \
>> -                      $(OUTPUT)/sign-file                              \
>> -                      $(OUTPUT)/uprobe_multi                           \
>> -                      ima_setup.sh                                     \
>> -                      verify_sig_setup.sh                              \
>> -                      $(wildcard progs/btf_dump_test_case_*.c)
>> +TRUNNER_PROGS_EXTRA_FILES:= $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko    \
>> +                           $(OUTPUT)/liburandom_read.so                        \
>> +                           $(OUTPUT)/xdp_synproxy                              \
>> +                           $(OUTPUT)/sign-file                                 \
>> +                           $(OUTPUT)/uprobe_multi                              \
>> +                           ima_setup.sh                                        \
>> +                           verify_sig_setup.sh                                 \
>> +                           $(wildcard progs/btf_dump_test_case_*.c)
>> +TRUNNER_EXTRA_FILES := $(TRUNNER_PROGS_EXTRA_FILES)
>>  TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
>>  TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
>>  $(eval $(call DEFINE_TEST_RUNNER,test_progs))
>> @@ -740,11 +741,17 @@ EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)    \
>>  # Delete partially updated (corrupted) files on error
>>  .DELETE_ON_ERROR:
>>
>> +space := $(subst ,, )
>> +comma := ,
>> +EXTRA_FILES_GLOB := {$(subst $(space),$(comma),$(notdir $(TRUNNER_PROGS_EXTRA_FILES)))}
>>  DEFAULT_INSTALL_RULE := $(INSTALL_RULE)
>>  override define INSTALL_RULE
>>         $(DEFAULT_INSTALL_RULE)
>> -       @for DIR in $(TEST_INST_SUBDIRS); do              \
>> -               mkdir -p $(INSTALL_PATH)/$$DIR;   \
>> -               rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;\
>> +       @for DIR in $(TEST_INST_SUBDIRS); do                                            \
>> +               mkdir -p $(INSTALL_PATH)/$$DIR;                                         \
>> +               rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;                 \
>> +               rsync -a --copy-unsafe-links                                            \
>> +                       $$(/bin/bash -c "echo $(OUTPUT)/$$DIR/$(EXTRA_FILES_GLOB)")     \
>
> this feels quite hacky... have you tried using $(foreach) to go over
> each element of TRUNNER_PROGS_EXTRA_FILES and append $(OUTPUT)/$$DIR/
> to each one? Hopefully that will allow us to get rid of space and
> comma hacks?

I haven't -- I'll try that out. Another ugliness is that *.bpf.o and
bpftool has to be explicitly specified, which hints of unrobustness.

> I'm also wondering if it would be ok to just combine two rsync calls
> into one, so that $(INSTALL_PATH)/$$DIR is specified once?

FWIW, I fold the two calls in patch 3. Regardless; I'll try to make the
whole series more palatable.


Björn