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

[Martin KaFai Lau <martin.lau@linux.dev>]

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@bootlin.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;
>   }