[PATCH bpf 1/2] xdp: check prog type before updating BPF link

[PATCH bpf 1/2] xdp: check prog type before updating BPF link

[Toke Høiland-Jørgensen]

The bpf_xdp_link_update() function didn't check the program type before
updating the program, which made it possible to install any program type as
an XDP program, which is obviously not good. Syzbot managed to trigger this
by swapping in an LWT program on the XDP hook which would crash in a helper
call.

Fix this by adding a check and bailing out if the types don't match.

Fixes: 026a4c28e1db ("bpf, xdp: Implement LINK_UPDATE for BPF XDP link")
Reported-by: syzbot+983941aa85af6ded1fd9@syzkaller.appspotmail.com
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/core/dev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index c4708e2487fb..2078d04c6482 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9656,6 +9656,12 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
 		goto out_unlock;
 	}
 	old_prog = link->prog;
+	if (old_prog->type != new_prog->type ||
+	    old_prog->expected_attach_type != new_prog->expected_attach_type) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
 	if (old_prog == new_prog) {
 		/* no-op, don't disturb drivers */
 		bpf_prog_put(new_prog);
-- 
2.34.1

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

[Toke Høiland-Jørgensen]

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 <toke@redhat.com>
---
 .../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);
+	/* updating program under active BPF link with different type fails */
+	err = bpf_link__update_program(link, skel1->progs.tc_handler);
+	if (!ASSERT_ERR(err, "link_upd_invalid"))
+		goto cleanup;
 
 	err = bpf_link__detach(link);
-	if (CHECK(err, "link_detach", "failed %d\n", err))
+	if (!ASSERT_OK(err, "link_detach"))
 		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") ||
+	    !ASSERT_EQ(link_info.prog_id, id1, "link_prog_id") ||
+	    /* ifindex should be zeroed out */
+	    !ASSERT_EQ(link_info.xdp.ifindex, 0, "link_ifindex"))
 		goto cleanup;
-	CHECK(link_info.prog_id != id1, "link_prog_id",
-	      "got %u != exp %u\n", link_info.prog_id, id1);
-	/* ifindex should be zeroed out */
-	CHECK(link_info.xdp.ifindex != 0, "link_ifindex",
-	      "got %u != exp %u\n", link_info.xdp.ifindex, 0);
 
 cleanup:
 	test_xdp_link__destroy(skel1);
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_link.c b/tools/testing/selftests/bpf/progs/test_xdp_link.c
index ee7d6ac0f615..64ff32eaae92 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_link.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_link.c
@@ -10,3 +10,9 @@ int xdp_handler(struct xdp_md *xdp)
 {
 	return 0;
 }
+
+SEC("tc")
+int tc_handler(struct __sk_buff *skb)
+{
+	return 0;
+}
-- 
2.34.1

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

[Andrii Nakryiko]

On Fri, Jan 7, 2022 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> 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 <toke@redhat.com>
> ---
>  .../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;

> +       /* updating program under active BPF link with different type fails */
> +       err = bpf_link__update_program(link, skel1->progs.tc_handler);
> +       if (!ASSERT_ERR(err, "link_upd_invalid"))
> +               goto cleanup;
>
>         err = bpf_link__detach(link);
> -       if (CHECK(err, "link_detach", "failed %d\n", err))
> +       if (!ASSERT_OK(err, "link_detach"))
>                 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") ||
> +           !ASSERT_EQ(link_info.prog_id, id1, "link_prog_id") ||
> +           /* ifindex should be zeroed out */
> +           !ASSERT_EQ(link_info.xdp.ifindex, 0, "link_ifindex"))
>                 goto cleanup;
> -       CHECK(link_info.prog_id != id1, "link_prog_id",
> -             "got %u != exp %u\n", link_info.prog_id, id1);
> -       /* ifindex should be zeroed out */
> -       CHECK(link_info.xdp.ifindex != 0, "link_ifindex",
> -             "got %u != exp %u\n", link_info.xdp.ifindex, 0);

same here, CHECK() with no if is just ASSERT_xxx() with no if

>
>  cleanup:
>         test_xdp_link__destroy(skel1);
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_link.c b/tools/testing/selftests/bpf/progs/test_xdp_link.c
> index ee7d6ac0f615..64ff32eaae92 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_link.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_link.c
> @@ -10,3 +10,9 @@ int xdp_handler(struct xdp_md *xdp)
>  {
>         return 0;
>  }
> +
> +SEC("tc")
> +int tc_handler(struct __sk_buff *skb)
> +{
> +       return 0;
> +}
> --
> 2.34.1
>

Re: [PATCH bpf 1/2] xdp: check prog type before updating BPF link

[Andrii Nakryiko]

On Fri, Jan 7, 2022 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> The bpf_xdp_link_update() function didn't check the program type before
> updating the program, which made it possible to install any program type as
> an XDP program, which is obviously not good. Syzbot managed to trigger this
> by swapping in an LWT program on the XDP hook which would crash in a helper
> call.
>
> Fix this by adding a check and bailing out if the types don't match.
>
> Fixes: 026a4c28e1db ("bpf, xdp: Implement LINK_UPDATE for BPF XDP link")
> Reported-by: syzbot+983941aa85af6ded1fd9@syzkaller.appspotmail.com
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

The fix looks good to me, thanks. I'd love it if this was done
generically in link_update, but each link type has its own locking
schema for link->prog, so I didn't figure out a way to do this in a
centralized way.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  net/core/dev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c4708e2487fb..2078d04c6482 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9656,6 +9656,12 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
>                 goto out_unlock;
>         }
>         old_prog = link->prog;
> +       if (old_prog->type != new_prog->type ||
> +           old_prog->expected_attach_type != new_prog->expected_attach_type) {
> +               err = -EINVAL;
> +               goto out_unlock;
> +       }
> +
>         if (old_prog == new_prog) {
>                 /* no-op, don't disturb drivers */
>                 bpf_prog_put(new_prog);
> --
> 2.34.1
>

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

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Jan 7, 2022 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> 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 <toke@redhat.com>
>> ---
>>  .../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

Re: [PATCH bpf 1/2] xdp: check prog type before updating BPF link

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Jan 7, 2022 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> The bpf_xdp_link_update() function didn't check the program type before
>> updating the program, which made it possible to install any program type as
>> an XDP program, which is obviously not good. Syzbot managed to trigger this
>> by swapping in an LWT program on the XDP hook which would crash in a helper
>> call.
>>
>> Fix this by adding a check and bailing out if the types don't match.
>>
>> Fixes: 026a4c28e1db ("bpf, xdp: Implement LINK_UPDATE for BPF XDP link")
>> Reported-by: syzbot+983941aa85af6ded1fd9@syzkaller.appspotmail.com
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> The fix looks good to me, thanks. I'd love it if this was done
> generically in link_update, but each link type has its own locking
> schema for link->prog, so I didn't figure out a way to do this in a
> centralized way.

Yeah, moving it to be a generic check was my first thought as well, but
I came to the same conclusion :)

> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks!

-Toke

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

[Andrii Nakryiko]

On Fri, Jan 7, 2022 at 1:23 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Fri, Jan 7, 2022 at 10:31 AM Toke Høiland-Jørgensen <toke@redhat.com> 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 <toke@redhat.com>
> >> ---
> >>  .../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 :)

Nope, completely intentional. There are cases when we need to goto
(e.g., we got invalid pointer or something, so it will crash if we
continue testing). But for cases when we have a bunch of different
properties to validate, I think it's cleaner to validate all of them
unconditionally with no ifs. If any of them is wrong, the test will be
marked failed anyways. But we'll also see all invalid values, not just
the first one, which helps with testing. It's also easier to follow
sequential code. We do this in multiple tests, it's not a coincidence.

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

Yes, please do. It would be also nice if you could split selftest
change into two: one adding new test step (checking that invalid
update fails) and another that converts CHECKs to ASSERTs. Thanks!


>
> -Toke
>