netdev - Re: [PATCH bpf-next v2 7/7] libbpf: add selftest for bpf

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a2e9d489-7fe1-f185-3443-565601624269@fb.com>
Date:   Sat, 5 Jun 2021 10:26:43 -0700
From:   Yonghong Song <yhs@...com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>, <bpf@...r.kernel.org>
CC:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Vlad Buslov <vladbu@...dia.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 7/7] libbpf: add selftest for bpf_link based
 TC-BPF management API



On 6/3/21 11:31 PM, Kumar Kartikeya Dwivedi wrote:
> This covers basic attach/detach/update, and tests interaction with the
> netlink API. It also exercises the bpf_link_info and fdinfo codepaths.
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>.
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> ---
>   .../selftests/bpf/prog_tests/tc_bpf_link.c    | 285 ++++++++++++++++++
>   1 file changed, 285 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/tc_bpf_link.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_bpf_link.c b/tools/testing/selftests/bpf/prog_tests/tc_bpf_link.c
> new file mode 100644
> index 000000000000..beaf06e0557c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_bpf_link.c
> @@ -0,0 +1,285 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include <linux/pkt_cls.h>
> +
> +#include "test_tc_bpf.skel.h"
> +
> +#define LO_IFINDEX 1
> +
> +static int test_tc_bpf_link_basic(struct bpf_tc_hook *hook,
> +				  struct bpf_program *prog)
> +{
> +	DECLARE_LIBBPF_OPTS(bpf_tc_link_opts, opts, .handle = 1, .priority = 1);
> +	DECLARE_LIBBPF_OPTS(bpf_tc_opts, qopts, .handle = 1, .priority = 1);
> +	struct bpf_prog_info info = {};
> +	__u32 info_len = sizeof(info);
> +	struct bpf_link *link, *invl;
> +	int ret;
> +
> +	link = bpf_program__attach_tc(prog, hook, &opts);
> +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_tc"))
> +		return PTR_ERR(link);

If we changed the bpf_program__attach_tc return semantics such that
the return "link" value can only be NULL or a valid pointer, you can do
	if (!ASSERT_OK_PTR(link, "bpf_program__attach_tc"))
		return -EINVAL;

The true error code should have been printed inside ASSERT_OK_PTR.

The same for a few other cases below.

> +
> +	ret = bpf_obj_get_info_by_fd(bpf_program__fd(prog), &info, &info_len);
> +	if (!ASSERT_OK(ret, "bpf_obj_get_info_by_fd"))
> +		goto end;
> +
> +	ret = bpf_tc_query(hook, &qopts);
> +	if (!ASSERT_OK(ret, "bpf_tc_query"))
> +		goto end;
> +
> +	if (!ASSERT_EQ(qopts.prog_id, info.id, "prog_id match"))
> +		goto end;
> +
> +	opts.gen_flags = ~0u;
> +	invl = bpf_program__attach_tc(prog, hook, &opts);
> +	if (!ASSERT_ERR_PTR(invl, "bpf_program__attach_tc with invalid flags")) {
> +		bpf_link__destroy(invl);
> +		ret = -EINVAL;
> +	}
> +
> +end:
> +	bpf_link__destroy(link);
> +	return ret;
> +}
> +
[...]
> +
> +static int test_tc_bpf_link_info_api(struct bpf_tc_hook *hook,
> +				     struct bpf_program *prog)
> +{
> +	DECLARE_LIBBPF_OPTS(bpf_tc_link_opts, opts, .handle = 1, .priority = 1);
> +	__u32 ifindex, parent, handle, gen_flags, priority;
> +	char buf[4096], path[256], *begin;
> +	struct bpf_link_info info = {};
> +	__u32 info_len = sizeof(info);
> +	struct bpf_link *link;
> +	int ret, fdinfo;
> +
> +	link = bpf_program__attach_tc(prog, hook, &opts);
> +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_tc"))
> +		return PTR_ERR(link);
> +
> +	ret = bpf_obj_get_info_by_fd(bpf_link__fd(link), &info, &info_len);
> +	if (!ASSERT_OK(ret, "bpf_obj_get_info_by_fd"))
> +		goto end;
> +
> +	ret = snprintf(path, sizeof(path), "/proc/self/fdinfo/%d",
> +		       bpf_link__fd(link));
> +	if (!ASSERT_TRUE(!ret || ret < sizeof(path), "snprintf pathname"))
> +		goto end;
> +
> +	fdinfo = open(path, O_RDONLY);
> +	if (!ASSERT_GT(fdinfo, -1, "open fdinfo"))
> +		goto end;
> +
> +	ret = read(fdinfo, buf, sizeof(buf));
> +	if (!ASSERT_GT(ret, 0, "read fdinfo")) {
> +		ret = -EINVAL;
> +		goto end_file;
> +	}
> +
> +	begin = strstr(buf, "ifindex");
> +	if (!ASSERT_OK_PTR(begin, "find beginning of fdinfo info")) {
> +		ret = -EINVAL;
> +		goto end_file;
> +	}
> +
> +	ret = sscanf(begin, "ifindex:\t%u\n"
> +			    "parent:\t%u\n"
> +			    "handle:\t%u\n"
> +			    "priority:\t%u\n"
> +			    "gen_flags:\t%u\n",
> +			    &ifindex, &parent, &handle, &priority, &gen_flags);
> +	if (!ASSERT_EQ(ret, 5, "sscanf fdinfo")) {
> +		ret = -EINVAL;
> +		goto end_file;
> +	}
> +
> +	ret = -EINVAL;
> +
> +#define X(a, b, c) (!ASSERT_EQ(a, b, #a " == " #b) || !ASSERT_EQ(b, c, #b " == " #c))
> +	if (X(info.tc.ifindex, ifindex, 1) ||
> +	    X(info.tc.parent, parent,
> +	      TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS)) ||
> +	    X(info.tc.handle, handle, 1) ||
> +	    X(info.tc.gen_flags, gen_flags, TCA_CLS_FLAGS_NOT_IN_HW) ||
> +	    X(info.tc.priority, priority, 1))
> +#undef X
> +		goto end_file;

Maybe put "#undef X" after "goto end_file" is a little bit better?

> +
> +	ret = 0;
> +
> +end_file:
> +	close(fdinfo);
> +end:
> +	bpf_link__destroy(link);
> +	return ret;
> +}
> +
[...]