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

Commit Message

Alexis Lothoré (eBPF Foundation) Oct. 16, 2024, 6:35 p.m. UTC

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) <[e-mail]>
---
 .../selftests/bpf/prog_tests/btf_skc_cls_ingress.c | 31 ++++++++++++++--------
 1 file changed, 20 insertions(+), 11 deletions(-)

Comments

Martin KaFai Lau Oct. 18, 2024, 11:57 p.m. UTC | #1

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) <[e-mail]>
> ---
>   .../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;
>   }

Alexis Lothoré (eBPF Foundation) Oct. 19, 2024, 12:13 p.m. UTC | #2

Hi Martin, thanks for the review !

On 10/19/24 01:57, Martin KaFai Lau wrote:
> 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) <[e-mail]>

[...]

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

Ah, indeed, I missed it in make_netns. Thanks, I'll remove this part from the
test then.
> 
> 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.

Patch

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"))
-		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;
 }
 
 static void reset_test(void)
@@ -169,6 +175,7 @@  void test_btf_skc_cls_ingress(void)
 	int i;
 
 	skel = test_btf_skc_cls_ingress__open_and_load();
+	struct netns_obj *ns;
 	if (CHECK(!skel, "test_btf_skc_cls_ingress__open_and_load", "failed\n"))
 		return;
 
@@ -176,13 +183,15 @@  void test_btf_skc_cls_ingress(void)
 		if (!test__start_subtest(tests[i].desc))
 			continue;
 
-		if (prepare_netns())
+		ns = prepare_netns();
+		if (!ns)
 			break;
 
 		tests[i].run();
 
 		print_err_line();
 		reset_test();
+		netns_free(ns);
 	}
 
 	test_btf_skc_cls_ingress__destroy(skel);