Message-ID: <5aab98d1-949d-4a20-a64c-3187439a7e81@bootlin.com>
Date: Tue, 6 Aug 2024 19:24:46 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: Alan Maguire <alan.maguire@...cle.com>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau
 <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>,
 Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
 Shuah Khan <shuah@...nel.org>
Cc: ebpf@...uxfoundation.org, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, linux-kernel@...r.kernel.org,
 bpf@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next 3/4] selftests/bpf: add proper section name to
 bpf prog and rename it

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@...tlin.com>
>>>
>>> Reviewed-by: Alan Maguire <alan.maguire@...cle.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

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

Powered by blists - more mailing lists