| Message ID | 160079991808.8301.6462172487971110332.stgit@toke.dk |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Support multi-attach for freplace programs | expand |
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 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. > + struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */ > + struct bpf_prog *tgt_prog; > + struct bpf_trampoline *tgt_trampoline; > bool verifier_zext; /* Zero extensions has been inserted by verifier. */ > bool offload_requested; > bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */ ... > struct bpf_tracing_link { > struct bpf_link link; > enum bpf_attach_type attach_type; > + struct bpf_trampoline *trampoline; > + struct bpf_prog *tgt_prog; imo it's confusing to have 'tgt_prog' to mean two different things. In prog->aux->tgt_prog it means target prog to attach to in the future. Whereas here it means the existing prog that was used to attached to. They kinda both 'target progs' but would be good to disambiguate. May be keep it as 'tgt_prog' here and rename to 'dest_prog' and 'dest_trampoline' in prog->aux ? > }; > > static void bpf_tracing_link_release(struct bpf_link *link) > { > - WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog)); > + struct bpf_tracing_link *tr_link = > + container_of(link, struct bpf_tracing_link, link); > + > + WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog, > + tr_link->trampoline)); > + > + bpf_trampoline_put(tr_link->trampoline); > + > + if (tr_link->tgt_prog) > + bpf_prog_put(tr_link->tgt_prog); I had to scratch my head quite a bit before I understood this NULL check. Could you add a comment saying that tr_link->tgt_prog can be NULL when trampoline is for kernel function ?
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
On Thu, Sep 24, 2020 at 7:34 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > ...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... :) Ignoring failures is the same as not running them. I expect all developers to confirm that they see "0 FAILED" before sending any patches. > > > 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. Please see Documentation/bpf/bpf_devel_QA.rst. patchwork status is the way we communicate the intent. If the patch is not in the queue it won't be acted upon.
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 > 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: yeah, seems like selftests/bpf/config needs to be updated to mention UMH-related config values: CONFIG_BPF_PRELOAD=y CONFIG_BPF_PRELOAD_UMD=m|y with that test_bpffs shouldn't fail on master > > [..] > CC [M] kernel/bpf/preload/bpf_preload_kern.o > > Auto-detecting system features: > ... libelf: [ OFF ] > ... zlib: [ OFF ] > ... bpf: [ OFF ] > > No libelf found might be worthwhile to look into why detection fails, might be something with Makefiles or your environment > > ...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... :) Don't just assume some tests are always broken. Either ask or investigate on your own. Such cases do happen from time to time while we wait for a fix in bpf to get merged into bpf-next or vice versa, but it's rare. We now have two different CI systems running selftests all the time, in addition to running them locally as well, so any permanent test failure is very apparent and annoying, so we fix them quickly. So, when in doubt - ask or fix. > > > 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 They are and hopefully moving forward that would be your assumption. > 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 >
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? :) >> 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?) >> 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: > > yeah, seems like selftests/bpf/config needs to be updated to mention > UMH-related config values: > > CONFIG_BPF_PRELOAD=y > CONFIG_BPF_PRELOAD_UMD=m|y > > with that test_bpffs shouldn't fail on master Yup, did get that far, and got the below... >> >> [..] >> CC [M] kernel/bpf/preload/bpf_preload_kern.o >> >> Auto-detecting system features: >> ... libelf: [ OFF ] >> ... zlib: [ OFF ] >> ... bpf: [ OFF ] >> >> No libelf found > > might be worthwhile to look into why detection fails, might be > something with Makefiles or your environment I think it's actually another instance of the bug I fixed with this commit: 1eb832ac2dee ("tools/bpf: build: Make sure resolve_btfids cleans up after itself") which I finally remembered after being tickled by the error message seeming familiar. And indeed, manually removing the 'feature' directory in kernel/bpf/preload seems to fix the issue, so I'm planning to go fix that Makefile as well... >> ...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... :) > > Don't just assume some tests are always broken. Either ask or > investigate on your own. Such cases do happen from time to time while > we wait for a fix in bpf to get merged into bpf-next or vice versa, > but it's rare. We now have two different CI systems running selftests > all the time, in addition to running them locally as well, so any > permanent test failure is very apparent and annoying, so we fix them > quickly. So, when in doubt - ask or fix. That's good to know; and I do think the situation has improved immensely. There was a time when the selftests broke every other week (or so it felt, at least), and I guess I'm still a bit scarred from that. 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... >> > 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 > > They are and hopefully moving forward that would be your assumption. Sure, with the exception of the two tests still failing that I mentioned above. Which I'm hoping you can help figure out the reason for :) -Toke
>> > 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. > > Please see Documentation/bpf/bpf_devel_QA.rst. > patchwork status is the way we communicate the intent. > If the patch is not in the queue it won't be acted upon. I do realise that you guys use patchwork as the status tracker, but from a submitter PoV, in practice a change there is coupled with an email either requesting something change, or notifying of merge. Which is fine, and I'm not asking you to do anything differently. I'm just suggesting that if you start silently marking patches as 'changes requested' without emailing the submitter explaining why, that will just going to end up creating confusion, and you'll get questions and/or identical resubmissions. So it won't actually solve anything... (And to be clear, I'm not saying this because I plan to deliberately submit patches with broken selftests in the future!) -Toke
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: >> + struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */ >> + struct bpf_prog *tgt_prog; >> + struct bpf_trampoline *tgt_trampoline; >> bool verifier_zext; /* Zero extensions has been inserted by verifier. */ >> bool offload_requested; >> bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */ > ... >> struct bpf_tracing_link { >> struct bpf_link link; >> enum bpf_attach_type attach_type; >> + struct bpf_trampoline *trampoline; >> + struct bpf_prog *tgt_prog; > > imo it's confusing to have 'tgt_prog' to mean two different things. > In prog->aux->tgt_prog it means target prog to attach to in the future. > Whereas here it means the existing prog that was used to attached to. > They kinda both 'target progs' but would be good to disambiguate. > May be keep it as 'tgt_prog' here and > rename to 'dest_prog' and 'dest_trampoline' in prog->aux ? I started changing this as you suggested, but I think it actually makes the code weirder. We'll end up with a lot of 'tgt_prog = prog->aux->dest_prog' assignments in the verifier, unless we also rename all of the local variables, which I think is just code churn for very little gain (the existing 'target' meaning is quite clear, I think). I also think it's quite natural that the target moves; I mean, it's literally the same pointer being re-assigned from prog->aux to the link. We could rename the link member to 'attached_tgt_prog' or something like that, but I'm not sure it helps (and I don't see much of a problem in the first place). WDYT? -Toke
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? 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 > > >> 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. > > >> 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: > > > > yeah, seems like selftests/bpf/config needs to be updated to mention > > UMH-related config values: > > > > CONFIG_BPF_PRELOAD=y > > CONFIG_BPF_PRELOAD_UMD=m|y > > > > with that test_bpffs shouldn't fail on master > > Yup, did get that far, and got the below... > > >> > >> [..] > >> CC [M] kernel/bpf/preload/bpf_preload_kern.o > >> > >> Auto-detecting system features: > >> ... libelf: [ OFF ] > >> ... zlib: [ OFF ] > >> ... bpf: [ OFF ] > >> > >> No libelf found > > > > might be worthwhile to look into why detection fails, might be > > something with Makefiles or your environment > > I think it's actually another instance of the bug I fixed with this > commit: > > 1eb832ac2dee ("tools/bpf: build: Make sure resolve_btfids cleans up after itself") > > which I finally remembered after being tickled by the error message > seeming familiar. And indeed, manually removing the 'feature' directory > in kernel/bpf/preload seems to fix the issue, so I'm planning to go fix > that Makefile as well... > glad we got to the bottom of it > >> ...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... :) > > > > Don't just assume some tests are always broken. Either ask or > > investigate on your own. Such cases do happen from time to time while > > we wait for a fix in bpf to get merged into bpf-next or vice versa, > > but it's rare. We now have two different CI systems running selftests > > all the time, in addition to running them locally as well, so any > > permanent test failure is very apparent and annoying, so we fix them > > quickly. So, when in doubt - ask or fix. > > That's good to know; and I do think the situation has improved > immensely. There was a time when the selftests broke every other week > (or so it felt, at least), and I guess I'm still a bit scarred from > that. > > 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. 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 > > >> > 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 > > > > They are and hopefully moving forward that would be your assumption. > > Sure, with the exception of the two tests still failing that I mentioned > above. Which I'm hoping you can help figure out the reason for :) > > -Toke >
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
On Thu, Sep 24, 2020 at 3:20 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > 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 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 >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> [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 Aha, found it! It happened because my kernel was built with PREEMPT_VOLUNTARY. Changing that to PREEMPT fixed the test, and got me to: Summary: 116/853 PASSED, 14 SKIPPED, 0 FAILED So yay! Thanks for your help with debugging :) -Toke
On Thu, Sep 24, 2020 at 3:00 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > > >> + struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */ > >> + struct bpf_prog *tgt_prog; > >> + struct bpf_trampoline *tgt_trampoline; > >> bool verifier_zext; /* Zero extensions has been inserted by verifier. */ > >> bool offload_requested; > >> bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */ > > ... > >> struct bpf_tracing_link { > >> struct bpf_link link; > >> enum bpf_attach_type attach_type; > >> + struct bpf_trampoline *trampoline; > >> + struct bpf_prog *tgt_prog; > > > > imo it's confusing to have 'tgt_prog' to mean two different things. > > In prog->aux->tgt_prog it means target prog to attach to in the future. > > Whereas here it means the existing prog that was used to attached to. > > They kinda both 'target progs' but would be good to disambiguate. > > May be keep it as 'tgt_prog' here and > > rename to 'dest_prog' and 'dest_trampoline' in prog->aux ? > > I started changing this as you suggested, but I think it actually makes > the code weirder. We'll end up with a lot of 'tgt_prog = > prog->aux->dest_prog' assignments in the verifier, unless we also rename > all of the local variables, which I think is just code churn for very > little gain (the existing 'target' meaning is quite clear, I think). you mean "churn" just for this patch. that's fine. But it will make names more accurate for everyone reading it afterwards. Hence I prefer distinct and specific names where possible. > I also think it's quite natural that the target moves; I mean, it's > literally the same pointer being re-assigned from prog->aux to the link. > We could rename the link member to 'attached_tgt_prog' or something like > that, but I'm not sure it helps (and I don't see much of a problem in > the first place). 'attached_tgt_prog' will not be the correct name. There is 'prog' inside the link already. That's 'attached' prog. Not this one. This one is the 'attached_to' prog. But such name would be too long. imo calling it 'dest_prog' in aux is shorter and more obvious.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Thu, Sep 24, 2020 at 3:00 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: >> >> >> + struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */ >> >> + struct bpf_prog *tgt_prog; >> >> + struct bpf_trampoline *tgt_trampoline; >> >> bool verifier_zext; /* Zero extensions has been inserted by verifier. */ >> >> bool offload_requested; >> >> bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */ >> > ... >> >> struct bpf_tracing_link { >> >> struct bpf_link link; >> >> enum bpf_attach_type attach_type; >> >> + struct bpf_trampoline *trampoline; >> >> + struct bpf_prog *tgt_prog; >> > >> > imo it's confusing to have 'tgt_prog' to mean two different things. >> > In prog->aux->tgt_prog it means target prog to attach to in the future. >> > Whereas here it means the existing prog that was used to attached to. >> > They kinda both 'target progs' but would be good to disambiguate. >> > May be keep it as 'tgt_prog' here and >> > rename to 'dest_prog' and 'dest_trampoline' in prog->aux ? >> >> I started changing this as you suggested, but I think it actually makes >> the code weirder. We'll end up with a lot of 'tgt_prog = >> prog->aux->dest_prog' assignments in the verifier, unless we also rename >> all of the local variables, which I think is just code churn for very >> little gain (the existing 'target' meaning is quite clear, I think). > > you mean "churn" just for this patch. that's fine. > But it will make names more accurate for everyone reading it afterwards. > Hence I prefer distinct and specific names where possible. > >> I also think it's quite natural that the target moves; I mean, it's >> literally the same pointer being re-assigned from prog->aux to the link. >> We could rename the link member to 'attached_tgt_prog' or something like >> that, but I'm not sure it helps (and I don't see much of a problem in >> the first place). > > 'attached_tgt_prog' will not be the correct name. > There is 'prog' inside the link already. That's 'attached' prog. > Not this one. This one is the 'attached_to' prog. > But such name would be too long. > imo calling it 'dest_prog' in aux is shorter and more obvious. Meh, don't really see how it helps ('destination' and 'target' are literally synonyms). But I don't care enough to bikeshed about it either, so I'll just do a search/replace... -Toke
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a1760fd87815..f0fc110ac0fb 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -619,8 +619,8 @@ static __always_inline unsigned int bpf_dispatcher_nop_func( } #ifdef CONFIG_BPF_JIT struct bpf_trampoline *bpf_trampoline_lookup(u64 key); -int bpf_trampoline_link_prog(struct bpf_prog *prog); -int bpf_trampoline_unlink_prog(struct bpf_prog *prog); +int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr); +int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr); struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr, struct btf_func_model *fmodel); void bpf_trampoline_put(struct bpf_trampoline *tr); @@ -671,11 +671,13 @@ static inline struct bpf_trampoline *bpf_trampoline_lookup(u64 key) { return NULL; } -static inline int bpf_trampoline_link_prog(struct bpf_prog *prog) +static inline int bpf_trampoline_link_prog(struct bpf_prog *prog, + struct bpf_trampoline *tr) { return -ENOTSUPP; } -static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog) +static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog, + struct bpf_trampoline *tr) { return -ENOTSUPP; } @@ -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; + struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */ + struct bpf_prog *tgt_prog; + struct bpf_trampoline *tgt_trampoline; bool verifier_zext; /* Zero extensions has been inserted by verifier. */ bool offload_requested; bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */ @@ -754,7 +758,6 @@ struct bpf_prog_aux { bool sleepable; bool tail_call_reachable; enum bpf_tramp_prog_type trampoline_prog_type; - struct bpf_trampoline *trampoline; struct hlist_node tramp_hlist; /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */ const struct btf_type *attach_func_proto; diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 868c03a24d0a..76cc6ae46821 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -3706,7 +3706,7 @@ struct btf *btf_parse_vmlinux(void) struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog) { - struct bpf_prog *tgt_prog = prog->aux->linked_prog; + struct bpf_prog *tgt_prog = prog->aux->tgt_prog; if (tgt_prog) { return tgt_prog->aux->btf; @@ -3733,7 +3733,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, struct bpf_insn_access_aux *info) { const struct btf_type *t = prog->aux->attach_func_proto; - struct bpf_prog *tgt_prog = prog->aux->linked_prog; + struct bpf_prog *tgt_prog = prog->aux->tgt_prog; struct btf *btf = bpf_prog_get_target_btf(prog); const char *tname = prog->aux->attach_func_name; struct bpf_verifier_log *log = info->log; @@ -4559,7 +4559,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, return -EFAULT; } if (prog_type == BPF_PROG_TYPE_EXT) - prog_type = prog->aux->linked_prog->type; + prog_type = prog->aux->tgt_prog->type; t = btf_type_by_id(btf, t->type); if (!t || !btf_type_is_func_proto(t)) { diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index c4811b139caa..0eb5f7501e29 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -99,6 +99,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag INIT_LIST_HEAD_RCU(&fp->aux->ksym.lnode); mutex_init(&fp->aux->used_maps_mutex); + mutex_init(&fp->aux->tgt_mutex); return fp; } @@ -255,6 +256,7 @@ void __bpf_prog_free(struct bpf_prog *fp) { if (fp->aux) { mutex_destroy(&fp->aux->used_maps_mutex); + mutex_destroy(&fp->aux->tgt_mutex); free_percpu(fp->aux->stats); kfree(fp->aux->poke_tab); kfree(fp->aux); @@ -2138,7 +2140,8 @@ static void bpf_prog_free_deferred(struct work_struct *work) if (aux->prog->has_callchain_buf) put_callchain_buffers(); #endif - bpf_trampoline_put(aux->trampoline); + if (aux->tgt_trampoline) + bpf_trampoline_put(aux->tgt_trampoline); for (i = 0; i < aux->func_cnt; i++) bpf_jit_free(aux->func[i]); if (aux->func_cnt) { @@ -2154,8 +2157,8 @@ void bpf_prog_free(struct bpf_prog *fp) { struct bpf_prog_aux *aux = fp->aux; - if (aux->linked_prog) - bpf_prog_put(aux->linked_prog); + if (aux->tgt_prog) + bpf_prog_put(aux->tgt_prog); INIT_WORK(&aux->work, bpf_prog_free_deferred); schedule_work(&aux->work); } diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index ec68d3a23a2b..a2db33f4753e 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2161,7 +2161,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr) err = PTR_ERR(tgt_prog); goto free_prog_nouncharge; } - prog->aux->linked_prog = tgt_prog; + prog->aux->tgt_prog = tgt_prog; } prog->aux->offload_requested = !!attr->prog_ifindex; @@ -2494,11 +2494,22 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd) struct bpf_tracing_link { struct bpf_link link; enum bpf_attach_type attach_type; + struct bpf_trampoline *trampoline; + struct bpf_prog *tgt_prog; }; static void bpf_tracing_link_release(struct bpf_link *link) { - WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog)); + struct bpf_tracing_link *tr_link = + container_of(link, struct bpf_tracing_link, link); + + WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog, + tr_link->trampoline)); + + bpf_trampoline_put(tr_link->trampoline); + + if (tr_link->tgt_prog) + bpf_prog_put(tr_link->tgt_prog); } static void bpf_tracing_link_dealloc(struct bpf_link *link) @@ -2541,7 +2552,9 @@ static const struct bpf_link_ops bpf_tracing_link_lops = { static int bpf_tracing_prog_attach(struct bpf_prog *prog) { struct bpf_link_primer link_primer; + struct bpf_prog *tgt_prog = NULL; struct bpf_tracing_link *link; + struct bpf_trampoline *tr; int err; switch (prog->type) { @@ -2579,19 +2592,37 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog) &bpf_tracing_link_lops, prog); link->attach_type = prog->expected_attach_type; - err = bpf_link_prime(&link->link, &link_primer); - if (err) { - kfree(link); - goto out_put_prog; + mutex_lock(&prog->aux->tgt_mutex); + + if (!prog->aux->tgt_trampoline) { + err = -ENOENT; + goto out_unlock; } + tr = prog->aux->tgt_trampoline; + tgt_prog = prog->aux->tgt_prog; + + err = bpf_link_prime(&link->link, &link_primer); + if (err) + goto out_unlock; - err = bpf_trampoline_link_prog(prog); + err = bpf_trampoline_link_prog(prog, tr); if (err) { bpf_link_cleanup(&link_primer); - goto out_put_prog; + link = NULL; + goto out_unlock; } + link->tgt_prog = tgt_prog; + link->trampoline = tr; + + prog->aux->tgt_prog = NULL; + prog->aux->tgt_trampoline = NULL; + mutex_unlock(&prog->aux->tgt_mutex); + return bpf_link_settle(&link_primer); +out_unlock: + mutex_unlock(&prog->aux->tgt_mutex); + kfree(link); out_put_prog: bpf_prog_put(prog); return err; diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index e86d32f7f7dc..3145615647a5 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -261,14 +261,12 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) } } -int bpf_trampoline_link_prog(struct bpf_prog *prog) +int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr) { enum bpf_tramp_prog_type kind; - struct bpf_trampoline *tr; int err = 0; int cnt; - tr = prog->aux->trampoline; kind = bpf_attach_type_to_tramp(prog); mutex_lock(&tr->mutex); if (tr->extension_prog) { @@ -301,7 +299,7 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog) } hlist_add_head(&prog->aux->tramp_hlist, &tr->progs_hlist[kind]); tr->progs_cnt[kind]++; - err = bpf_trampoline_update(prog->aux->trampoline); + err = bpf_trampoline_update(tr); if (err) { hlist_del(&prog->aux->tramp_hlist); tr->progs_cnt[kind]--; @@ -312,13 +310,11 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog) } /* bpf_trampoline_unlink_prog() should never fail. */ -int bpf_trampoline_unlink_prog(struct bpf_prog *prog) +int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr) { enum bpf_tramp_prog_type kind; - struct bpf_trampoline *tr; int err; - tr = prog->aux->trampoline; kind = bpf_attach_type_to_tramp(prog); mutex_lock(&tr->mutex); if (kind == BPF_TRAMP_REPLACE) { @@ -330,7 +326,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog) } hlist_del(&prog->aux->tramp_hlist); tr->progs_cnt[kind]--; - err = bpf_trampoline_update(prog->aux->trampoline); + err = bpf_trampoline_update(tr); out: mutex_unlock(&tr->mutex); return err; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index ad244a606d7a..647fac170f19 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2643,8 +2643,7 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno, static enum bpf_prog_type resolve_prog_type(struct bpf_prog *prog) { - return prog->aux->linked_prog ? prog->aux->linked_prog->type - : prog->type; + return prog->aux->tgt_prog ? prog->aux->tgt_prog->type : prog->type; } static bool may_access_direct_pkt_data(struct bpf_verifier_env *env, @@ -11452,8 +11451,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, static int check_attach_btf_id(struct bpf_verifier_env *env) { struct bpf_prog *prog = env->prog; - struct bpf_prog *tgt_prog = prog->aux->linked_prog; u32 btf_id = prog->aux->attach_btf_id; + struct bpf_prog *tgt_prog = prog->aux->tgt_prog; struct btf_func_model fmodel; struct bpf_trampoline *tr; const struct btf_type *t; @@ -11517,7 +11516,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) if (!tr) return -ENOMEM; - prog->aux->trampoline = tr; + prog->aux->tgt_trampoline = tr; return 0; }