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 16:37:08 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
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 Tue, Jul 16, 2019 at 3:57 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
>
> 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.

Oh, -qp is cool, noted :)

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

Well, it's "File is an intermediate prerequisite." in both cases, so I
don't know if that's it.
Makefiles is not my strong suit, honestly, and definitely not an area
of passion, so no idea

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

Some of those _TEST_DIR's are dependencies of multiple targets, so
you'd need to replicate that `mkdir -p $@` in multiple places, which
is annoying.

But I also don't think we need to worry about creating them, because
there is always at least one test (otherwise those tests are useless
anyways) in those directories, so we might as well just remove those
dependencies, I guess.

TBH, those explicit dependencies are good to specify anyways, so I
think this fix is good. Second patch just makes the layout of
dependencies similar, so it's easier to spot differences like this
one.

As usual, I'll let Alexei and Daniel decide which one to apply (or if
we need to take some other approach to fixing this).


>
> 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'; \