Message-ID: <CAEf4Bza38tR1GvyLzzzzv6zT8B-_gM_jhTqK_c7+e1ciU3ZA1w@mail.gmail.com>
Date:   Thu, 24 Sep 2020 15:37:04 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Jakub Sitnicki <jakub@...udflare.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and
 trampoline into bpf_link on attach

On Thu, Sep 24, 2020 at 3:20 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@...il.com> writes:
>
> > On Thu, Sep 24, 2020 at 2:24 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@...il.com> writes:
> >>
> >> > On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >> >>
> >> >> Alexei Starovoitov <alexei.starovoitov@...il.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@...hvm bpf]$ uname -r
> > 5.9.0-rc6-01779-g182bf3f3ddb6
> > [vmuser@...hvm 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

I don't see 64363ff12e8f sha in my repo, so I still don't know what
commit your kernel is built off of. But I believe that you have the
latest kernel, you'll just need to debug this on your own, though,
because this test was never flaky for me, I can't repro the failure.

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

try adding sleep(few seconds, enough for RCU grace period to pass)
here and see if that helps

if not, please printk() around to see why the inner_map1 wasn't freed

> 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! :)

ok, cool

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

no idea, might be worth trying

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

glad I could help

>
> -Toke
>

Powered by blists - more mailing lists