Re: [PATCH bpf-next v2 7/7] libbpf: add selftest for bpf_link based TC-BPF management API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 3, 2021 at 11:32 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> 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@xxxxxxxxxx>.
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
>  .../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
>

[...]

> +static int test_tc_bpf_link_netlink_interaction(struct bpf_tc_hook *hook,
> +                                               struct bpf_program *prog)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_link_update_opts, lopts,
> +                           .old_prog_fd = bpf_program__fd(prog));
> +       DECLARE_LIBBPF_OPTS(bpf_tc_link_opts, opts, .handle = 1, .priority = 1);
> +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, nopts, .handle = 1, .priority = 1);
> +       DECLARE_LIBBPF_OPTS(bpf_tc_opts, dopts, .handle = 1, .priority = 1);
> +       struct bpf_link *link;
> +       int ret;
> +
> +       /* We need to test the following cases:
> +        *      1. BPF link owned filter cannot be replaced by netlink
> +        *      2. Netlink owned filter cannot be replaced by BPF link
> +        *      3. Netlink cannot do targeted delete of BPF link owned filter
> +        *      4. Filter is actually deleted (with chain cleanup)
> +        *         We actually (ab)use the kernel behavior of returning EINVAL when
> +        *         target chain doesn't exist on tc_get_tfilter (which maps to
> +        *         bpf_tc_query) here, to know if the chain was really cleaned
> +        *         up on tcf_proto destruction. Our setup is so that there is
> +        *         only one reference to the chain.
> +        *
> +        *         So on query, chain ? (filter ?: ENOENT) : EINVAL
> +        */
> +
> +       link = bpf_program__attach_tc(prog, hook, &opts);
> +       if (!ASSERT_OK_PTR(link, "bpf_program__attach_tc"))
> +               return PTR_ERR(link);
> +
> +       nopts.prog_fd = bpf_program__fd(prog);
> +       ret = bpf_tc_attach(hook, &nopts);
> +       if (!ASSERT_EQ(ret, -EEXIST, "bpf_tc_attach without replace"))
> +               goto end;
> +
> +       nopts.flags = BPF_TC_F_REPLACE;
> +       ret = bpf_tc_attach(hook, &nopts);
> +       if (!ASSERT_EQ(ret, -EPERM, "bpf_tc_attach with replace"))
> +               goto end;
> +
> +       ret = bpf_tc_detach(hook, &dopts);
> +       if (!ASSERT_EQ(ret, -EPERM, "bpf_tc_detach"))
> +               goto end;
> +
> +       lopts.flags = BPF_F_REPLACE;
> +       ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog),
> +                             &lopts);
> +       ASSERT_OK(ret, "bpf_link_update");
> +       ret = ret < 0 ? -errno : ret;

all selftests run in libbpf 1.0 mode, so you get actual error
directly, so no need to deal with -errno here.

> +
> +end:
> +       bpf_link__destroy(link);
> +       if (!ret && !ASSERT_EQ(bpf_tc_query(hook, &dopts), -EINVAL,
> +                              "chain empty delete"))
> +               ret = -EINVAL;
> +       return ret;
> +}
> +
> +static int test_tc_bpf_link_update_ways(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_link_update_opts, uopts, 0);
> +       struct test_tc_bpf *skel;
> +       struct bpf_link *link;
> +       int ret;
> +
> +       skel = test_tc_bpf__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
> +               return PTR_ERR(skel);
> +
> +       link = bpf_program__attach_tc(prog, hook, &opts);
> +       if (!ASSERT_OK_PTR(link, "bpf_program__attach_tc")) {
> +               ret = PTR_ERR(link);
> +               goto end;
> +       }
> +
> +       ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog),
> +                             &uopts);
> +       if (!ASSERT_OK(ret, "bpf_link_update no old prog"))
> +               goto end;
> +
> +       uopts.old_prog_fd = bpf_program__fd(prog);
> +       ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog),
> +                             &uopts);

please keep all such calls single-line, they aren't excessively long at all

> +       if (!ASSERT_TRUE(ret < 0 && errno == EINVAL,
> +                        "bpf_link_update with old prog without BPF_F_REPLACE")) {

same as above, ret should already be -EINVAL, so just check directly

> +               ret = -EINVAL;
> +               goto end;
> +       }
> +
> +       uopts.flags = BPF_F_REPLACE;
> +       ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog),
> +                             &uopts);
> +       if (!ASSERT_OK(ret, "bpf_link_update with old prog with BPF_F_REPLACE"))
> +               goto end;
> +
> +       uopts.old_prog_fd = bpf_program__fd(skel->progs.cls);
> +       ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog),
> +                             &uopts);
> +       if (!ASSERT_TRUE(ret < 0 && errno == EINVAL,
> +                        "bpf_link_update with wrong old prog")) {

and here

> +               ret = -EINVAL;
> +               goto end;
> +       }
> +       ret = 0;
> +
> +end:
> +       test_tc_bpf__destroy(skel);
> +       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;

ASSERT_TRUE is very generic, it's better to do ASSERT_LT(ret,
sizeof(path), "snprintf") here

not sure why `!ret` is allowed?..

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

This seems to be a bit too convoluted and over-engineered. Just
validate all the equalities unconditionally.

ASSERT_EQ(info.tc.ifindex, 1, "info.tc.ifindex");
ASSERT_EQ(ifindex, 1, "fdinfo.ifindex");
ASSERT_EQ(info.tc.parent, TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS),
"info.tc.parent");

and so on.

Then, you don't really need to propagate errors from
test_tc_bpf_link_info_api, because each ASSERT_EQ() marks the test (or
subtest) as failed, so you don't have to do that below in
test_tc_bpf_link.

> +               goto end_file;
> +
> +       ret = 0;
> +
> +end_file:
> +       close(fdinfo);
> +end:
> +       bpf_link__destroy(link);
> +       return ret;
> +}
> +
> +void test_tc_bpf_link(void)
> +{
> +       DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = LO_IFINDEX,
> +                           .attach_point = BPF_TC_INGRESS);
> +       struct test_tc_bpf *skel = NULL;
> +       bool hook_created = false;
> +       int ret;
> +
> +       skel = test_tc_bpf__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "test_tc_bpf__open_and_load"))
> +               return;
> +
> +       ret = bpf_tc_hook_create(&hook);
> +       if (ret == 0)
> +               hook_created = true;
> +
> +       ret = ret == -EEXIST ? 0 : ret;
> +       if (!ASSERT_OK(ret, "bpf_tc_hook_create(BPF_TC_INGRESS)"))
> +               goto end;
> +
> +       ret = test_tc_bpf_link_basic(&hook, skel->progs.cls);
> +       if (!ASSERT_OK(ret, "test_tc_bpf_link_basic"))
> +               goto end;
> +
> +       bpf_tc_hook_destroy(&hook);
> +
> +       hook.attach_point = BPF_TC_EGRESS;
> +       ret = test_tc_bpf_link_basic(&hook, skel->progs.cls);
> +       if (!ASSERT_OK(ret, "test_tc_bpf_link_basic"))
> +               goto end;
> +
> +       bpf_tc_hook_destroy(&hook);
> +
> +       ret = test_tc_bpf_link_netlink_interaction(&hook, skel->progs.cls);
> +       if (!ASSERT_OK(ret, "test_tc_bpf_link_netlink_interaction"))
> +               goto end;
> +
> +       bpf_tc_hook_destroy(&hook);
> +
> +       ret = test_tc_bpf_link_update_ways(&hook, skel->progs.cls);
> +       if (!ASSERT_OK(ret, "test_tc_bpf_link_update_ways"))
> +               goto end;
> +
> +       bpf_tc_hook_destroy(&hook);
> +
> +       ret = test_tc_bpf_link_info_api(&hook, skel->progs.cls);
> +       if (!ASSERT_OK(ret, "test_tc_bpf_link_info_api"))

I was talking about this above, it's completely unnecessary and
redundant. Just complicates everything.

> +               goto end;
> +
> +end:
> +       if (hook_created) {
> +               hook.attach_point = BPF_TC_INGRESS | BPF_TC_EGRESS;
> +               bpf_tc_hook_destroy(&hook);
> +       }
> +       test_tc_bpf__destroy(skel);
> +}
> --
> 2.31.1
>




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux