[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <05d39d25-d132-4194-a2a1-fbae8917e70b@oracle.com>
Date: Tue, 6 Aug 2024 13:33:01 +0100
From: Alan Maguire <alan.maguire@...cle.com>
To: Alexis Lothoré <alexis.lothore@...tlin.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 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/
Powered by blists - more mailing lists