On Thu, Sep 24, 2020 at 3:20 PM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

On Thu, Sep 24, 2020 at 2:24 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Andrii Nakryiko [off-list ref] writes:

quoted

On Thu, Sep 24, 2020 at 7:36 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Alexei Starovoitov [off-list ref] writes:

quoted

On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:

quoted

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

quoted

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

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

quoted

quoted

quoted

quoted

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

quoted

quoted

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

quoted

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