netdev - Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and creating bpf qdisc as default qdisc

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: <83c8f387-c4a9-4293-9996-fec285d34c94@linux.dev>
Date: Thu, 1 May 2025 16:58:09 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org,
 alexei.starovoitov@...il.com, andrii@...nel.org, daniel@...earbox.net,
 martin.lau@...nel.org, xiyou.wangcong@...il.com, kernel-team@...a.com
Subject: Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and
 creating bpf qdisc as default qdisc

On 5/1/25 3:30 PM, Amery Hung wrote:
> First, test that bpf qdisc can be set as default qdisc. Then, attach
> an mq qdisc to see if bpf qdisc can be successfully created and grafted.
> 
> The test is a sequential test as net.core.default_qdisc is global.
> 
> Signed-off-by: Amery Hung <ameryhung@...il.com>
> ---
>   .../selftests/bpf/prog_tests/bpf_qdisc.c      | 78 +++++++++++++++++++
>   1 file changed, 78 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> index c9a54177c84e..c954cc2ae64f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> @@ -159,6 +159,79 @@ static void test_qdisc_attach_to_non_root(void)
>   	bpf_qdisc_fifo__destroy(fifo_skel);
>   }
>   
> +static int get_default_qdisc(char *qdisc_name)
> +{
> +	FILE *f;
> +	int num;
> +
> +	f = fopen("/proc/sys/net/core/default_qdisc", "r");
> +	if (!f)
> +		return -errno;
> +
> +	num = fscanf(f, "%s", qdisc_name);
> +	fclose(f);
> +
> +	return num == 1 ? 0 : -EFAULT;
> +}
> +
> +static void test_default_qdisc_attach_to_mq(void)
> +{
> +	struct bpf_qdisc_fifo *fifo_skel;
> +	char default_qdisc[IFNAMSIZ];
> +	struct netns_obj *netns;
> +	char tc_qdisc_show[64];
> +	struct bpf_link *link;
> +	char *str_ret;
> +	FILE *tc;
> +	int err;
> +
> +	fifo_skel = bpf_qdisc_fifo__open_and_load();
> +	if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(fifo_skel->maps.fifo);

	fifo_skel->links.fifo = bpf_map__attach_struct_ops(....);

Then no need to bpf_link__destroy(link). bpf_qdisc_fifo__destroy() should do.

> +	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
> +		bpf_qdisc_fifo__destroy(fifo_skel);
> +		return;
> +	}
> +
> +	err = get_default_qdisc(default_qdisc);
> +	if (!ASSERT_OK(err, "read sysctl net.core.default_qdisc"))
> +		goto out;
> +
> +	err = write_sysctl("/proc/sys/net/core/default_qdisc", "bpf_fifo");
> +	if (!ASSERT_OK(err, "write sysctl net.core.default_qdisc"))
> +		goto out;
> +
> +	netns = netns_new("bpf_qdisc_ns", true);
> +	if (!ASSERT_OK_PTR(netns, "netns_new"))
> +		goto out;

This should be 'goto out_restore_dflt_qdisc'.

I would stay with minimum number of cleanup labels if possible. Initialize the 
variables that need to be cleaned up instead. There is no need to optimize each 
cleanup case for a test,

e.g. "struct netns_obj netns = NULL; char default_qdisc[IFNAMSIZ] = {};..."

> +
> +	SYS(out_restore_dflt_qdisc, "ip link add veth0 type veth peer veth1");
> +	SYS(out_delete_netns, "tc qdisc add dev veth0 root handle 1: mq");
> +
> +	tc = popen("tc qdisc show dev veth0 parent 1:1", "r");
> +	if (!ASSERT_OK_PTR(tc, "tc qdisc show dev veth0 parent 1:1"))
> +		goto out_delete_netns;
> +
> +	str_ret = fgets(tc_qdisc_show, sizeof(tc_qdisc_show), tc);
> +	if (!ASSERT_OK_PTR(str_ret, "tc qdisc show dev veth0 parent 1:1"))
> +		goto out_delete_netns;
> +
> +	str_ret = strstr(tc_qdisc_show, "qdisc bpf_fifo");
> +	if (!ASSERT_OK_PTR(str_ret, "check if bpf_fifo is created"))
> +		goto out_delete_netns;

Instead of pipe and grep, how about having the bpf_fifo_init bpf prog to set a 
global variable when called and then check the "fifo_skel->bss->init_called == 
true" here?

> +
> +	SYS(out_delete_netns, "tc qdisc delete dev veth0 root mq");
> +out_delete_netns:
> +	netns_free(netns);
> +out_restore_dflt_qdisc:
> +	write_sysctl("/proc/sys/net/core/default_qdisc", default_qdisc);
> +out:
> +	bpf_link__destroy(link);
> +	bpf_qdisc_fifo__destroy(fifo_skel);
> +}
> +
>   void test_bpf_qdisc(void)
>   {
>   	struct netns_obj *netns;
> @@ -178,3 +251,8 @@ void test_bpf_qdisc(void)
>   
>   	netns_free(netns);
>   }
> +
> +void serial_test_bpf_qdisc_default(void)
> +{
> +	test_default_qdisc_attach_to_mq();
> +}