linux-kernel - Re: [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in btf_skc_cls

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: <18cb274a-a214-42c0-bcec-cbda34703893@linux.dev>
Date: Fri, 18 Oct 2024 16:57:20 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Alexis Lothoré (eBPF Foundation)
 <alexis.lothore@...tlin.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 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>, "David S. Miller" <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>, Jesper Dangaard Brouer <hawk@...nel.org>,
 ebpf@...uxfoundation.org, Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Lorenz Bauer <lmb@...udflare.com>, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 2/6] selftests/bpf: add missing ns cleanups in
 btf_skc_cls_ingress

On 10/16/24 11:35 AM, Alexis Lothoré (eBPF Foundation) wrote:
> btf_skc_cls_ingress.c currently runs two subtests, and create a
> dedicated network namespace for each, but never cleans up the created
> namespace once the test has ended.
> 
> Add missing namespace cleanup after each namespace to avoid accumulating
> namespaces for each new subtest. While at it, switch namespace
> management to netns_{new,free}
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@...tlin.com>
> ---
>   .../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 31 ++++++++++++++--------
>   1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> index 5d8d7736edc095b647ca3fbc12cac0440b60140e..8d1fa8806cdda088d264b44104f7c80726b025e2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
> @@ -17,32 +17,34 @@
>   #include "test_progs.h"
>   #include "test_btf_skc_cls_ingress.skel.h"
>   
> +#define TEST_NS "skc_cls_ingress"
> +
>   static struct test_btf_skc_cls_ingress *skel;
>   static struct sockaddr_in6 srv_sa6;
>   static __u32 duration;
>   
> -static int prepare_netns(void)
> +static struct netns_obj *prepare_netns(void)
>   {
>   	LIBBPF_OPTS(bpf_tc_hook, qdisc_lo, .attach_point = BPF_TC_INGRESS);
>   	LIBBPF_OPTS(bpf_tc_opts, tc_attach,
>   		    .prog_fd = bpf_program__fd(skel->progs.cls_ingress));
> +	struct netns_obj *ns = NULL;
>   
> -	if (CHECK(unshare(CLONE_NEWNET), "create netns",
> -		  "unshare(CLONE_NEWNET): %s (%d)",
> -		  strerror(errno), errno))
> -		return -1;
> +	ns = netns_new(TEST_NS, true);
> +	if (!ASSERT_OK_PTR(ns, "create and join netns"))
> +		return ns;
>   
>   	if (CHECK(system("ip link set dev lo up"),
>   		  "ip link set dev lo up", "failed\n"))

nit. netns_new() takes care of "lo up" also, so the above can be removed.

test_progs.c has restore_netns() after each test, so the netns was not cleaned 
up. The second unshare should have freed the earlier netns also.

Using netns_new() removed the boiler plate codes. It is nice to see this change 
here regardless.

> -		return -1;
> +		goto free_ns;
>   
>   	qdisc_lo.ifindex = if_nametoindex("lo");
>   	if (!ASSERT_OK(bpf_tc_hook_create(&qdisc_lo), "qdisc add dev lo clsact"))
> -		return -1;
> +		goto free_ns;
>   
>   	if (!ASSERT_OK(bpf_tc_attach(&qdisc_lo, &tc_attach),
>   		       "filter add dev lo ingress"))
> -		return -1;
> +		goto free_ns;
>   
>   	/* Ensure 20 bytes options (i.e. in total 40 bytes tcp header) for the
>   	 * bpf_tcp_gen_syncookie() helper.
> @@ -50,9 +52,13 @@ static int prepare_netns(void)
>   	if (write_sysctl("/proc/sys/net/ipv4/tcp_window_scaling", "1") ||
>   	    write_sysctl("/proc/sys/net/ipv4/tcp_timestamps", "1") ||
>   	    write_sysctl("/proc/sys/net/ipv4/tcp_sack", "1"))
> -		return -1;
> +		goto free_ns;
> +
> +	return ns;
>   
> -	return 0;
> +free_ns:
> +	netns_free(ns);
> +	return NULL;
>   }