netdev - Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Jul 2019 15:57:35 -0700
From:   Stanislav Fomichev <sdf@...ichev.me>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>,
        Ilya Leoshkevich <iii@...ux.ibm.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make
 dependencies

On 07/16, Andrii Nakryiko wrote:
> On Tue, Jul 16, 2019 at 12:55 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
> >
> > On 07/16, Andrii Nakryiko wrote:
> > > e46fc22e60a4 ("selftests/bpf: make directory prerequisites order-only")
> > > exposed existing problem in Makefile for test_verifier and test_maps tests:
> > > their dependency on auto-generated header file with a list of all tests wasn't
> > > recorded explicitly. This patch fixes these issues.
> > Why adding it explicitly fixes it? At least for test_verifier, we have
> > the following rule:
> >
> >         test_verifier.c: $(VERIFIER_TESTS_H)
> >
> > And there should be implicit/builtin test_verifier -> test_verifier.c
> > dependency rule.
> >
> > Same for maps, I guess:
> >
> >         $(OUTPUT)/test_maps: map_tests/*.c
> >         test_maps.c: $(MAP_TESTS_H)
> >
> > So why is it not working as is? What I'm I missing?
> 
> I don't know exactly why it's not working, but it's clearly because of
> that. It's the only difference between how test_progs are set up,
> which didn't break, and test_maps/test_verifier, which did.
> 
> Feel free to figure it out through a maze of Makefiles why it didn't
> work as expected, but this definitely fixed a breakage (at least for
> me).
Agreed on not wasting time. I took a brief look (with make -qp) and I
don't have any clue.

By default implicit matching doesn't work:
# makefile (from 'Makefile', line 261)
/linux/tools/testing/selftests/bpf/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
/linux/tools/testing/selftests/bpf/test_maps: map_tests/sk_storage_map.c /linux/tools/testing/selftests/bpf/test_stub.o /linux/tools/testing/selftests/bpf/libbpf.a
#  Implicit rule search has not been done.
#  File is an intermediate prerequisite.
#  Modification time never checked.
#  File has not been updated.
# variable set hash-table stats:
# Load=1/32=3%, Rehash=0, Collisions=0/2=0%

If I comment out the following line:
$(TEST_GEN_PROGS): $(OUTPUT)/test_stub.o $(BPFOBJ)

Then it works:
# makefile (from 'Makefile', line 261)
/linux/tools/testing/selftests/bpf/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
/linux/tools/testing/selftests/bpf/test_maps: test_maps.c map_tests/sk_storage_map.c
#  Implicit rule search has been done.
#  Implicit/static pattern stem: 'test_maps'
#  File is an intermediate prerequisite.
#  File does not exist.
#  File has not been updated.
# variable set hash-table stats:
# Load=1/32=3%, Rehash=0, Collisions=0/2=0%
#  recipe to execute (from '../lib.mk', line 138):
        $(LINK.c) $^ $(LDLIBS) -o $@

It's because "File is an intermediate prerequisite.", but I
don't see how it's is a intermediate prerequisite for anything...


One other optional suggestion I have to your second patch: maybe drop all
those dependencies on the directories altogether? Why not do the following
instead, for example (same for test_progs/test_maps)?

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 1296253b3422..c2d087ce6d4b 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -277,12 +277,9 @@ VERIFIER_TESTS_H := $(OUTPUT)/verifier/tests.h
 test_verifier.c: $(VERIFIER_TESTS_H)
 $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
 
-VERIFIER_TESTS_DIR = $(OUTPUT)/verifier
-$(VERIFIER_TESTS_DIR):
-       mkdir -p $@
-
 VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
-$(OUTPUT)/verifier/tests.h: $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
+$(OUTPUT)/verifier/tests.h: $(VERIFIER_TEST_FILES)
+       mkdir -p $(dir $@)
        $(shell ( cd verifier/; \
                  echo '/* Generated header, do not edit */'; \
                  echo '#ifdef FILL_ARRAY'; \