Subject: Re: [PATCH bpf 2/2] bpf/selftests: Add check for updating XDP bpf_link with wrong program type

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Fri, 07 Jan 2022 22:23:14 +0100

Andrii Nakryiko <[email-masked]> writes:

> On Fri, Jan 7, 2022 at 10:31 AM Toke Høiland-Jørgensen <[email-masked]> wrote:
>>
>> Add a check to the xdp_link selftest that the kernel rejects replacing an
>> XDP program with a different program type on link update. Convert the
>> selftest to use the preferred ASSERT_* macros while we're at it.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <[email-masked]>
>> ---
>>  .../selftests/bpf/prog_tests/xdp_link.c       | 62 +++++++++----------
>>  .../selftests/bpf/progs/test_xdp_link.c       |  6 ++
>>  2 files changed, 37 insertions(+), 31 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_link.c b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
>> index 983ab0b47d30..8660e68383ea 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_link.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
>> @@ -8,46 +8,47 @@
>>
>>  void serial_test_xdp_link(void)
>>  {
>> -       __u32 duration = 0, id1, id2, id0 = 0, prog_fd1, prog_fd2, err;
>>         DECLARE_LIBBPF_OPTS(bpf_xdp_set_link_opts, opts, .old_fd = -1);
>>         struct test_xdp_link *skel1 = NULL, *skel2 = NULL;
>> +       __u32 id1, id2, id0 = 0, prog_fd1, prog_fd2;
>>         struct bpf_link_info link_info;
>>         struct bpf_prog_info prog_info;
>>         struct bpf_link *link;
>> +       int err;
>>         __u32 link_info_len = sizeof(link_info);
>>         __u32 prog_info_len = sizeof(prog_info);
>>
>>         skel1 = test_xdp_link__open_and_load();
>> -       if (CHECK(!skel1, "skel_load", "skeleton open and load failed\n"))
>> +       if (!ASSERT_OK_PTR(skel1, "skel_load"))
>>                 goto cleanup;
>>         prog_fd1 = bpf_program__fd(skel1->progs.xdp_handler);
>>
>>         skel2 = test_xdp_link__open_and_load();
>> -       if (CHECK(!skel2, "skel_load", "skeleton open and load failed\n"))
>> +       if (!ASSERT_OK_PTR(skel2, "skel_load"))
>>                 goto cleanup;
>>         prog_fd2 = bpf_program__fd(skel2->progs.xdp_handler);
>>
>>         memset(&prog_info, 0, sizeof(prog_info));
>>         err = bpf_obj_get_info_by_fd(prog_fd1, &prog_info, &prog_info_len);
>> -       if (CHECK(err, "fd_info1", "failed %d\n", -errno))
>> +       if (!ASSERT_OK(err, "fd_info1"))
>>                 goto cleanup;
>>         id1 = prog_info.id;
>>
>>         memset(&prog_info, 0, sizeof(prog_info));
>>         err = bpf_obj_get_info_by_fd(prog_fd2, &prog_info, &prog_info_len);
>> -       if (CHECK(err, "fd_info2", "failed %d\n", -errno))
>> +       if (!ASSERT_OK(err, "fd_info2"))
>>                 goto cleanup;
>>         id2 = prog_info.id;
>>
>>         /* set initial prog attachment */
>>         err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, prog_fd1, XDP_FLAGS_REPLACE, &opts);
>> -       if (CHECK(err, "fd_attach", "initial prog attach failed: %d\n", err))
>> +       if (!ASSERT_OK(err, "fd_attach"))
>>                 goto cleanup;
>>
>>         /* validate prog ID */
>>         err = bpf_get_link_xdp_id(IFINDEX_LO, &id0, 0);
>> -       CHECK(err || id0 != id1, "id1_check",
>> -             "loaded prog id %u != id1 %u, err %d", id0, id1, err);
>> +       if (!ASSERT_OK(err, "id1_check_err") || !ASSERT_EQ(id0, id1, "id1_check_val"))
>> +               goto cleanup;
>>
>>         /* BPF link is not allowed to replace prog attachment */
>>         link = bpf_program__attach_xdp(skel1->progs.xdp_handler, IFINDEX_LO);
>> @@ -62,7 +63,7 @@ void serial_test_xdp_link(void)
>>         /* detach BPF program */
>>         opts.old_fd = prog_fd1;
>>         err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, -1, XDP_FLAGS_REPLACE, &opts);
>> -       if (CHECK(err, "prog_detach", "failed %d\n", err))
>> +       if (!ASSERT_OK(err, "prog_detach"))
>>                 goto cleanup;
>>
>>         /* now BPF link should attach successfully */
>> @@ -73,24 +74,23 @@ void serial_test_xdp_link(void)
>>
>>         /* validate prog ID */
>>         err = bpf_get_link_xdp_id(IFINDEX_LO, &id0, 0);
>> -       if (CHECK(err || id0 != id1, "id1_check",
>> -                 "loaded prog id %u != id1 %u, err %d", id0, id1, err))
>> +       if (!ASSERT_OK(err, "id1_check_err") || !ASSERT_EQ(id0, id1, "id1_check_val"))
>>                 goto cleanup;
>>
>>         /* BPF prog attach is not allowed to replace BPF link */
>>         opts.old_fd = prog_fd1;
>>         err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, prog_fd2, XDP_FLAGS_REPLACE, &opts);
>> -       if (CHECK(!err, "prog_attach_fail", "unexpected success\n"))
>> +       if (!ASSERT_ERR(err, "prog_attach_fail"))
>>                 goto cleanup;
>>
>>         /* Can't force-update when BPF link is active */
>>         err = bpf_set_link_xdp_fd(IFINDEX_LO, prog_fd2, 0);
>> -       if (CHECK(!err, "prog_update_fail", "unexpected success\n"))
>> +       if (!ASSERT_ERR(err, "prog_update_fail"))
>>                 goto cleanup;
>>
>>         /* Can't force-detach when BPF link is active */
>>         err = bpf_set_link_xdp_fd(IFINDEX_LO, -1, 0);
>> -       if (CHECK(!err, "prog_detach_fail", "unexpected success\n"))
>> +       if (!ASSERT_ERR(err, "prog_detach_fail"))
>>                 goto cleanup;
>>
>>         /* BPF link is not allowed to replace another BPF link */
>> @@ -110,40 +110,40 @@ void serial_test_xdp_link(void)
>>         skel2->links.xdp_handler = link;
>>
>>         err = bpf_get_link_xdp_id(IFINDEX_LO, &id0, 0);
>> -       if (CHECK(err || id0 != id2, "id2_check",
>> -                 "loaded prog id %u != id2 %u, err %d", id0, id1, err))
>> +       if (!ASSERT_OK(err, "id2_check_err") || !ASSERT_EQ(id0, id2, "id2_check_val"))
>>                 goto cleanup;
>>
>>         /* updating program under active BPF link works as expected */
>>         err = bpf_link__update_program(link, skel1->progs.xdp_handler);
>> -       if (CHECK(err, "link_upd", "failed: %d\n", err))
>> +       if (!ASSERT_OK(err, "link_upd"))
>>                 goto cleanup;
>>
>>         memset(&link_info, 0, sizeof(link_info));
>>         err = bpf_obj_get_info_by_fd(bpf_link__fd(link), &link_info, &link_info_len);
>> -       if (CHECK(err, "link_info", "failed: %d\n", err))
>> +       if (!ASSERT_OK(err, "link_info"))
>> +               goto cleanup;
>> +
>> +       if (!ASSERT_EQ(link_info.type, BPF_LINK_TYPE_XDP, "link_type") ||
>> +           !ASSERT_EQ(link_info.prog_id, id1, "link_prog_id") ||
>> +           !ASSERT_EQ(link_info.xdp.ifindex, IFINDEX_LO, "link_ifindex"))
>>                 goto cleanup;
>>
>> -       CHECK(link_info.type != BPF_LINK_TYPE_XDP, "link_type",
>> -             "got %u != exp %u\n", link_info.type, BPF_LINK_TYPE_XDP);
>> -       CHECK(link_info.prog_id != id1, "link_prog_id",
>> -             "got %u != exp %u\n", link_info.prog_id, id1);
>> -       CHECK(link_info.xdp.ifindex != IFINDEX_LO, "link_ifindex",
>> -             "got %u != exp %u\n", link_info.xdp.ifindex, IFINDEX_LO);
>
> these checks were done unconditionally (and all of them), even if one
> of the fails. Why did you do goto cleanup for those. Similarly below.
> It's much cleaner to just have three ASSERT_EQ() statements one after
> the other with no if() goto cleanup;

Because I figured the absence of a 'goto cleanup' was an oversight :)

Not sure why you think it's cleaner, but I don't have any strong opinion
about it either way, so I can respin get rid of the containing ifs if
you prefer...

-Toke