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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e8b88d27-4e85-46f1-8ed4-df883abc8a51@oracle.com>
Date: Thu, 1 Aug 2024 09:35:37 +0100
From: Alan Maguire <alan.maguire@...cle.com>
To: Alexis Lothoré (eBPF Foundation)
 <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 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>

> ---
>  .../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"
>