| Message ID | 20240731-convert_cgroup_tests-v1-3-14cbc51b6947@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7b4400a0a69b8255a2afed0bead0ce2c2c457545 |
| Headers | show |
| Series | selftests/bpf: convert three other cgroup tests to test_progs | expand |
On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote: > test_skb_cgroup_id_kern.c is currently involved in a manual test. In its > current form, it can not be used with the auto-generated skeleton APIs, > because the section name is not valid to allow libbpf to deduce the program > type. > > Update section name to allow skeleton APIs usage. Also rename the program > name to make it shorter and more straighforward regarding the API it is > testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get > a working reference before converting it to test_progs > - update the obj name > - fix loading issue (verifier rejecting the program when loaded through tc, > because of map not found), by preloading the whole obj with bpftool > > Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> Reviewed-by: Alan Maguire <alan.maguire@oracle.com> > --- > .../progs/{test_skb_cgroup_id_kern.c => cgroup_ancestor.c} | 2 +- > tools/testing/selftests/bpf/test_skb_cgroup_id.sh | 12 ++++++++---- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c > similarity index 97% > rename from tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c > rename to tools/testing/selftests/bpf/progs/cgroup_ancestor.c > index 37aacc66cd68..4879645f5827 100644 > --- a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c > +++ b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c > @@ -28,7 +28,7 @@ static __always_inline void log_nth_level(struct __sk_buff *skb, __u32 level) > bpf_map_update_elem(&cgroup_ids, &level, &id, 0); > } > > -SEC("cgroup_id_logger") > +SEC("tc") > int log_cgroup_id(struct __sk_buff *skb) > { > /* Loop unroll can't be used here due to [1]. Unrolling manually. > diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh > index 515c2eafc97f..d7dad49175c2 100755 > --- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh > +++ b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh > @@ -30,8 +30,10 @@ setup() > wait_for_ip > > tc qdisc add dev ${TEST_IF} clsact > - tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \ > - sec ${BPF_PROG_SECTION} da > + mkdir -p /sys/fs/bpf/${BPF_PROG_PIN} > + bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc > + tc filter add dev ${TEST_IF} egress bpf da object-pinned \ > + /sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME} > Just out of curiosity; did you find that the pin is needed? I would have thought tc attach would be enough to keep the program aroud. > BPF_PROG_ID=$(tc filter show dev ${TEST_IF} egress | \ > awk '/ id / {sub(/.* id /, "", $0); print($1)}') > @@ -41,6 +43,7 @@ cleanup() > { > ip link del ${TEST_IF} 2>/dev/null || : > ip link del ${TEST_IF_PEER} 2>/dev/null || : > + rm -rf /sys/fs/bpf/${BPF_PROG_PIN} > } > > main() > @@ -54,8 +57,9 @@ DIR=$(dirname $0) > TEST_IF="test_cgid_1" > TEST_IF_PEER="test_cgid_2" > MAX_PING_TRIES=5 > -BPF_PROG_OBJ="${DIR}/test_skb_cgroup_id_kern.bpf.o" > -BPF_PROG_SECTION="cgroup_id_logger" > +BPF_PROG_PIN="cgroup_ancestor" > +BPF_PROG_OBJ="${DIR}/${BPF_PROG_PIN}.bpf.o" > +BPF_PROG_NAME="log_cgroup_id" > BPF_PROG_ID=0 > PROG="${DIR}/test_skb_cgroup_id_user" > type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6" >
On 8/1/24 10:35, Alan Maguire wrote: > On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote: >> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its >> current form, it can not be used with the auto-generated skeleton APIs, >> because the section name is not valid to allow libbpf to deduce the program >> type. >> >> Update section name to allow skeleton APIs usage. Also rename the program >> name to make it shorter and more straighforward regarding the API it is >> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get >> a working reference before converting it to test_progs >> - update the obj name >> - fix loading issue (verifier rejecting the program when loaded through tc, >> because of map not found), by preloading the whole obj with bpftool >> >> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> > > Reviewed-by: Alan Maguire <alan.maguire@oracle.com> [...] >> tc qdisc add dev ${TEST_IF} clsact >> - tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \ >> - sec ${BPF_PROG_SECTION} da >> + mkdir -p /sys/fs/bpf/${BPF_PROG_PIN} >> + bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc >> + tc filter add dev ${TEST_IF} egress bpf da object-pinned \ >> + /sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME} >> > > Just out of curiosity; did you find that the pin is needed? I would have > thought tc attach would be enough to keep the program aroud. That's more because that's the only way I found to make the filter addition work. With the current command, the tc command fails because of the verifier rejecting the program: Verifier analysis: func#0 @0 0: R1=ctx() R10=fp0 0: (bf) r6 = r1 ; R1=ctx() R6_w=ctx() 1: (b4) w1 = 0 ; R1_w=0 2: (63) *(u32 *)(r10 -4) = r1 mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1 mark_precise: frame0: regs=r1 stack= before 1: (b4) w1 = 0 3: R1_w=0 R10=fp0 fp-8=0000???? 3: (bf) r1 = r6 ; R1_w=ctx() R6_w=ctx() 4: (b4) w2 = 0 ; R2_w=0 5: (85) call bpf_skb_ancestor_cgroup_id#83 ; R0_w=scalar() 6: (7b) *(u64 *)(r10 -16) = r0 ; R0_w=scalar(id=1) R10=fp0 fp-16_w=scalar(id=1) 7: (bf) r2 = r10 ; R2_w=fp0 R10=fp0 8: (07) r2 += -4 ; R2_w=fp-4 9: (bf) r3 = r10 ; R3_w=fp0 R10=fp0 10: (07) r3 += -16 ; R3_w=fp-16 11: (18) r1 = 0x0 ; R1_w=0 13: (b7) r4 = 0 ; R4_w=0 14: (85) call bpf_map_update_elem#2 R1 type=scalar expected=map_ptr processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0 (I also tried to remove the sec argument from the tc commandline, in case it could allow tc to load everything, but the issue remains the same) IIUC the verifier output, there's an issue with the variable representing the map. When stracing the tc filter add command, I indeed see the BPF_PROG_LOAD syscall but no BPF_MAP_CREATE at all, so I assumed tc did not read/create the map correctly. That's why I used bpftool to make sure everything is loaded, but as a consequence I need to provide a pin path when using load/loadall. But maybe I am missing something ?
On 01/08/2024 11:00, Alexis Lothoré wrote: > On 8/1/24 10:35, Alan Maguire wrote: >> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote: >>> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its >>> current form, it can not be used with the auto-generated skeleton APIs, >>> because the section name is not valid to allow libbpf to deduce the program >>> type. >>> >>> Update section name to allow skeleton APIs usage. Also rename the program >>> name to make it shorter and more straighforward regarding the API it is >>> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get >>> a working reference before converting it to test_progs >>> - update the obj name >>> - fix loading issue (verifier rejecting the program when loaded through tc, >>> because of map not found), by preloading the whole obj with bpftool >>> >>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> >> >> Reviewed-by: Alan Maguire <alan.maguire@oracle.com> > > [...] > >>> tc qdisc add dev ${TEST_IF} clsact >>> - tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \ >>> - sec ${BPF_PROG_SECTION} da >>> + mkdir -p /sys/fs/bpf/${BPF_PROG_PIN} >>> + bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc >>> + tc filter add dev ${TEST_IF} egress bpf da object-pinned \ >>> + /sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME} >>> >> >> Just out of curiosity; did you find that the pin is needed? I would have >> thought tc attach would be enough to keep the program aroud. > > That's more because that's the only way I found to make the filter addition > work. With the current command, the tc command fails because of the verifier > rejecting the program: > > Verifier analysis: > > func#0 @0 > 0: R1=ctx() R10=fp0 > 0: (bf) r6 = r1 ; R1=ctx() R6_w=ctx() > 1: (b4) w1 = 0 ; R1_w=0 > 2: (63) *(u32 *)(r10 -4) = r1 > mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1 > mark_precise: frame0: regs=r1 stack= before 1: (b4) w1 = 0 > 3: R1_w=0 R10=fp0 fp-8=0000???? > 3: (bf) r1 = r6 ; R1_w=ctx() R6_w=ctx() > 4: (b4) w2 = 0 ; R2_w=0 > 5: (85) call bpf_skb_ancestor_cgroup_id#83 ; R0_w=scalar() > 6: (7b) *(u64 *)(r10 -16) = r0 ; R0_w=scalar(id=1) R10=fp0 > fp-16_w=scalar(id=1) > 7: (bf) r2 = r10 ; R2_w=fp0 R10=fp0 > 8: (07) r2 += -4 ; R2_w=fp-4 > 9: (bf) r3 = r10 ; R3_w=fp0 R10=fp0 > 10: (07) r3 += -16 ; R3_w=fp-16 > 11: (18) r1 = 0x0 ; R1_w=0 > 13: (b7) r4 = 0 ; R4_w=0 > 14: (85) call bpf_map_update_elem#2 > R1 type=scalar expected=map_ptr > processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 0 > peak_states 0 mark_read 0 > > (I also tried to remove the sec argument from the tc commandline, in case it > could allow tc to load everything, but the issue remains the same) > > IIUC the verifier output, there's an issue with the variable representing the map. > When stracing the tc filter add command, I indeed see the BPF_PROG_LOAD syscall > but no BPF_MAP_CREATE at all, so I assumed tc did not read/create the map > correctly. That's why I used bpftool to make sure everything is loaded, but as a > consequence I need to provide a pin path when using load/loadall. But maybe I am > missing something ? > No I think you're absolutely right. It seems like there have been problems with BTF-defined maps in the past with tc filter loading [1], but more recent tc fixes this since it uses libbpf under the hood. I tried with the section name update only and the test passes, so it might just be a tc version issue. As in [1] I'd suggest compiling an up-to-date iproute2/tc and re-testing. Thanks! [1] https://lore.kernel.org/bpf/87zgkx9l6y.fsf@toke.dk/
On 8/6/24 14:33, Alan Maguire wrote: > On 01/08/2024 11:00, Alexis Lothoré wrote: >> On 8/1/24 10:35, Alan Maguire wrote: >>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote: >>>> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its >>>> current form, it can not be used with the auto-generated skeleton APIs, >>>> because the section name is not valid to allow libbpf to deduce the program >>>> type. >>>> >>>> Update section name to allow skeleton APIs usage. Also rename the program >>>> name to make it shorter and more straighforward regarding the API it is >>>> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get >>>> a working reference before converting it to test_progs >>>> - update the obj name >>>> - fix loading issue (verifier rejecting the program when loaded through tc, >>>> because of map not found), by preloading the whole obj with bpftool >>>> >>>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> >>> >>> Reviewed-by: Alan Maguire <alan.maguire@oracle.com> >> >> [...] >> >>>> tc qdisc add dev ${TEST_IF} clsact >>>> - tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \ >>>> - sec ${BPF_PROG_SECTION} da >>>> + mkdir -p /sys/fs/bpf/${BPF_PROG_PIN} >>>> + bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc >>>> + tc filter add dev ${TEST_IF} egress bpf da object-pinned \ >>>> + /sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME} >>>> >>> >>> Just out of curiosity; did you find that the pin is needed? I would have >>> thought tc attach would be enough to keep the program aroud. >> >> That's more because that's the only way I found to make the filter addition >> work. With the current command, the tc command fails because of the verifier >> rejecting the program: >> >> Verifier analysis: >> >> func#0 @0 >> 0: R1=ctx() R10=fp0 >> 0: (bf) r6 = r1 ; R1=ctx() R6_w=ctx() >> 1: (b4) w1 = 0 ; R1_w=0 >> 2: (63) *(u32 *)(r10 -4) = r1 >> mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1 >> mark_precise: frame0: regs=r1 stack= before 1: (b4) w1 = 0 >> 3: R1_w=0 R10=fp0 fp-8=0000???? >> 3: (bf) r1 = r6 ; R1_w=ctx() R6_w=ctx() >> 4: (b4) w2 = 0 ; R2_w=0 >> 5: (85) call bpf_skb_ancestor_cgroup_id#83 ; R0_w=scalar() >> 6: (7b) *(u64 *)(r10 -16) = r0 ; R0_w=scalar(id=1) R10=fp0 >> fp-16_w=scalar(id=1) >> 7: (bf) r2 = r10 ; R2_w=fp0 R10=fp0 >> 8: (07) r2 += -4 ; R2_w=fp-4 >> 9: (bf) r3 = r10 ; R3_w=fp0 R10=fp0 >> 10: (07) r3 += -16 ; R3_w=fp-16 >> 11: (18) r1 = 0x0 ; R1_w=0 >> 13: (b7) r4 = 0 ; R4_w=0 >> 14: (85) call bpf_map_update_elem#2 >> R1 type=scalar expected=map_ptr >> processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 0 >> peak_states 0 mark_read 0 >> >> (I also tried to remove the sec argument from the tc commandline, in case it >> could allow tc to load everything, but the issue remains the same) >> >> IIUC the verifier output, there's an issue with the variable representing the map. >> When stracing the tc filter add command, I indeed see the BPF_PROG_LOAD syscall >> but no BPF_MAP_CREATE at all, so I assumed tc did not read/create the map >> correctly. That's why I used bpftool to make sure everything is loaded, but as a >> consequence I need to provide a pin path when using load/loadall. But maybe I am >> missing something ? >> > > No I think you're absolutely right. It seems like there have been > problems with BTF-defined maps in the past with tc filter loading [1], > but more recent tc fixes this since it uses libbpf under the hood. I > tried with the section name update only and the test passes, so it might > just be a tc version issue. As in [1] I'd suggest compiling an > up-to-date iproute2/tc and re-testing. Thanks! > > [1] https://lore.kernel.org/bpf/87zgkx9l6y.fsf@toke.dk/ I have checked my tc binary and indeed it was not built with libbpf support. So applying what is suggested in the thread you have attached, I rebuilt tc with proper libbpf support, and now the original tc command works properly (bpf program *and* map properly loaded through tc). So in the end, the issue was on my testing setup. I have already sent a v2 updating the other topics you have suggested, if it raises new comments I will use the opportunity to put back the previous tc filter command. Thanks for the review and the helpful comments ! Alexis
diff --git a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c similarity index 97% rename from tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c rename to tools/testing/selftests/bpf/progs/cgroup_ancestor.c index 37aacc66cd68..4879645f5827 100644 --- a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c +++ b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c @@ -28,7 +28,7 @@ static __always_inline void log_nth_level(struct __sk_buff *skb, __u32 level) bpf_map_update_elem(&cgroup_ids, &level, &id, 0); } -SEC("cgroup_id_logger") +SEC("tc") int log_cgroup_id(struct __sk_buff *skb) { /* Loop unroll can't be used here due to [1]. Unrolling manually. diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh index 515c2eafc97f..d7dad49175c2 100755 --- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh +++ b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh @@ -30,8 +30,10 @@ setup() wait_for_ip tc qdisc add dev ${TEST_IF} clsact - tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \ - sec ${BPF_PROG_SECTION} da + mkdir -p /sys/fs/bpf/${BPF_PROG_PIN} + bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc + tc filter add dev ${TEST_IF} egress bpf da object-pinned \ + /sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME} BPF_PROG_ID=$(tc filter show dev ${TEST_IF} egress | \ awk '/ id / {sub(/.* id /, "", $0); print($1)}') @@ -41,6 +43,7 @@ cleanup() { ip link del ${TEST_IF} 2>/dev/null || : ip link del ${TEST_IF_PEER} 2>/dev/null || : + rm -rf /sys/fs/bpf/${BPF_PROG_PIN} } main() @@ -54,8 +57,9 @@ DIR=$(dirname $0) TEST_IF="test_cgid_1" TEST_IF_PEER="test_cgid_2" MAX_PING_TRIES=5 -BPF_PROG_OBJ="${DIR}/test_skb_cgroup_id_kern.bpf.o" -BPF_PROG_SECTION="cgroup_id_logger" +BPF_PROG_PIN="cgroup_ancestor" +BPF_PROG_OBJ="${DIR}/${BPF_PROG_PIN}.bpf.o" +BPF_PROG_NAME="log_cgroup_id" BPF_PROG_ID=0 PROG="${DIR}/test_skb_cgroup_id_user" type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6"
test_skb_cgroup_id_kern.c is currently involved in a manual test. In its current form, it can not be used with the auto-generated skeleton APIs, because the section name is not valid to allow libbpf to deduce the program type. Update section name to allow skeleton APIs usage. Also rename the program name to make it shorter and more straighforward regarding the API it is testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get a working reference before converting it to test_progs - update the obj name - fix loading issue (verifier rejecting the program when loaded through tc, because of map not found), by preloading the whole obj with bpftool Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> --- .../progs/{test_skb_cgroup_id_kern.c => cgroup_ancestor.c} | 2 +- tools/testing/selftests/bpf/test_skb_cgroup_id.sh | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-)