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

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

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 ?

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com