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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: 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>,
	netdev@vger.kernel.org, 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: Thu, 24 Sep 2020 16:34:51 +0200	[thread overview]
Message-ID: <874knn1bw4.fsf@toke.dk> (raw)
In-Reply-To: <20200924001439.qitbu5tmzz55ck4z@ast-mbp.dhcp.thefacebook.com>

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
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
test_test_bpffs:FAIL:bpffs test  failed 255
#96 test_bpffs:FAIL
Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED

The test_bpffs failure happens because the umh is missing from the
.config; and when I tried to fix this I ended up with:

[..]
  CC [M]  kernel/bpf/preload/bpf_preload_kern.o

Auto-detecting system features:
...                        libelf: [ OFF ]
...                          zlib: [ OFF ]
...                           bpf: [ OFF ]

No libelf found

...which I just put down to random breakage, turned off the umh and
continued on my way (ignoring the failed test). Until you wrote this I
did not suspect this would be something I needed to pay attention to.
Now that you did mention it, I'll obviously go investigate some more, my
point is just that in this instance it's not accurate to assume I just
didn't run the tests... :)

> I think I will just start marking patches as changes-requested when I see that
> they break tests without replying and without reviewing.
> Please respect reviewer's time.

That is completely fine if the tests are working in the first place. And
even when they're not (like in this case), pointing it out is fine, and
I'll obviously go investigate. But please at least reply to the email,
not all of us watch patchwork regularly.

(I'll fix all your other comments and respin; thanks!)

-Toke