Re: [PATCH bpf-next 3/4] selftests/bpf: add proper section name to bpf prog and rename it

[Alan Maguire <alan.maguire@oracle.com>]

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/