Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach - Toke Høiland-Jørgensen

From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Jakub Sitnicki <jakub@cloudflare.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Jiri Olsa <jolsa@redhat.com>,
	Eelco Chaudron <echaudro@redhat.com>,
	KP Singh <kpsingh@chromium.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach
Date: Fri, 25 Sep 2020 00:20:01 +0200	[thread overview]
Message-ID: <87r1qqbywe.fsf@toke.dk> (raw)
In-Reply-To: <CAEf4BzZxfzQabDCdmby1XMQV7qQ_C=rATWOb=cN-Q1rfxR+nVA@mail.gmail.com>

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

> On Thu, Sep 24, 2020 at 2:24 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>> >>
>> >> > On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke HÃƒÂ¸iland-JÃƒÂ¸rgensen wrote:
>> >> >> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
>> >> >>      u32 max_rdonly_access;
>> >> >>      u32 max_rdwr_access;
>> >> >>      const struct bpf_ctx_arg_aux *ctx_arg_info;
>> >> >> -    struct bpf_prog *linked_prog;
>> >> >
>> >> > This change breaks bpf_preload and selftests test_bpffs.
>> >> > There is really no excuse not to run the selftests.
>> >>
>> >> I did run the tests, and saw no more breakages after applying my patches
>> >> than before. Which didn't catch this, because this is the current state
>> >> of bpf-next selftests:
>> >>
>> >> # ./test_progs  | grep FAIL
>> >> test_lookup_update:FAIL:map1_leak inner_map1 leaked!
>> >> #10/1 lookup_update:FAIL
>> >> #10 btf_map_in_map:FAIL
>> >
>> > this failure suggests you are not running the latest kernel, btw
>>
>> I did see that discussion (about the reverted patch), and figured that
>> was the case. So I did a 'git pull' just before testing, and still got
>> this.
>>
>> $ git describe HEAD
>> v5.9-rc3-2681-g182bf3f3ddb6
>>
>> so any other ideas? :)
>
> That memory leak was fixed in 1d4e1eab456e ("bpf: Fix map leak in
> HASH_OF_MAPS map") at the end of July. So while your git repo might be
> checked out on a recent enough commit, could it be that the kernel
> that you are running is not what you think you are running?

Nah, I'm running these in a one-shot virtual machine with virtme-run.

> I specifically built kernel from the same commit and double-checked:
>
> [vmuser@archvm bpf]$ uname -r
> 5.9.0-rc6-01779-g182bf3f3ddb6
> [vmuser@archvm bpf]$ sudo ./test_progs -t map_in_map
> #10/1 lookup_update:OK
> #10/2 diff_size:OK
> #10 btf_map_in_map:OK
> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED

Trying the same, while manually entering the VM:

[root@(none) bpf]# uname -r
5.9.0-rc6-02685-g64363ff12e8f
[root@(none) bpf]# ./test_progs -t map_in_map
test_lookup_update:PASS:skel_open 0 nsec
test_lookup_update:PASS:skel_attach 0 nsec
test_lookup_update:PASS:inner1 0 nsec
test_lookup_update:PASS:inner2 0 nsec
test_lookup_update:PASS:inner1 0 nsec
test_lookup_update:PASS:inner2 0 nsec
test_lookup_update:PASS:map1_id 0 nsec
test_lookup_update:PASS:map2_id 0 nsec
kern_sync_rcu:PASS:inner_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_update 0 nsec
test_lookup_update:PASS:sync_rcu 0 nsec
kern_sync_rcu:PASS:inner_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_create 0 nsec
kern_sync_rcu:PASS:outer_map_update 0 nsec
test_lookup_update:PASS:sync_rcu 0 nsec
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10/2 diff_size:OK
#10 btf_map_in_map:FAIL
Summary: 0/1 PASSED, 0 SKIPPED, 2 FAILED

>> >> configure_stack:FAIL:BPF load failed; run with -vv for more info
>> >> #72 sk_assign:FAIL
>>
>> (and what about this one, now that I'm asking?)
>
> Did you run with -vv? Jakub Sitnicki (cc'd) might probably help, if
> you provide a bit more details.

No, I didn't, silly me. Turned out that was also just a missing config
option - thanks! :)

>> One thing that would be really useful would be to have a 'reference
>> config' or something like that. Missing config options are a common
>> reason for test failures (as we have just seen above), and it's not
>> always obvious which option is missing for each test. Even something
>> like grepping .config for BPF doesn't catch everything. If you already
>> have a CI running, just pointing to that config would be a good start
>> (especially if it has history). In an ideal world I think it would be
>> great if each test could detect whether the kernel has the right config
>> set for its features and abort with a clear error message if it isn't...
>
> so tools/testing/selftests/bpf/config is intended to list all the
> config values necessary, but given we don't update them often we
> forget to update them when selftests requiring extra kernel config are
> added, unfortunately.

Ah, that's useful! I wonder how difficult it would be to turn this into
a 'make bpfconfig' top-level make target (similar to 'make defconfig')?

That way, it could be run automatically, and we would also catch
anything missing?

> As for CI's config, check [0], that's what we use to build kernels.
> Kernel config is intentionally pretty minimal and is running in a
> single-user mode in pretty stripped down environment, so might not
> work as is for full-blown VM. But you can still take a look.
>
>   [0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/latest.config

Well that's how I'm running my own tests (as mentioned above), so that
might be useful, actually! I'll go take a look, thanks :)

-Toke