* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
@ 2022-01-15 2:34 zhudi (E)
2022-01-15 2:48 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: zhudi (E) @ 2022-01-15 2:34 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
john fastabend, KP Singh, Jakub Sitnicki, bpf, open list,
Luzhihao (luzhihao, Euler), Chenxiang (EulerOS)
> On Thu, Jan 13, 2022 at 1:01 AM Di Zhu <zhudi2@huawei.com> wrote:
> >
> > Add test for querying progs attached to sockmap. we use an existing
> > libbpf query interface to query prog cnt before and after progs
> > attaching to sockmap and check whether the queried prog id is right.
> >
> > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > Acked-by: Yonghong Song <yhs@fb.com>
> > ---
> > .../selftests/bpf/prog_tests/sockmap_basic.c | 70 +++++++++++++++++++
> > .../bpf/progs/test_sockmap_progs_query.c | 24 +++++++
> > 2 files changed, 94 insertions(+)
> > create mode 100644
> tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > index 85db0f4cdd95..06923ea44bad 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -8,6 +8,7 @@
> > #include "test_sockmap_update.skel.h"
> > #include "test_sockmap_invalid_update.skel.h"
> > #include "test_sockmap_skb_verdict_attach.skel.h"
> > +#include "test_sockmap_progs_query.skel.h"
> > #include "bpf_iter_sockmap.skel.h"
> >
> > #define TCP_REPAIR 19 /* TCP sock is under repair
> right now */
> > @@ -315,6 +316,69 @@ static void test_sockmap_skb_verdict_attach(enum
> bpf_attach_type first,
> > test_sockmap_skb_verdict_attach__destroy(skel);
> > }
> >
> > +static __u32 query_prog_id(int prog_fd)
> > +{
> > + struct bpf_prog_info info = {};
> > + __u32 info_len = sizeof(info);
> > + int err;
> > +
> > + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> > + if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
> > + !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
> > + return 0;
> > +
> > + return info.id;
> > +}
> > +
> > +static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
> > +{
> > + struct test_sockmap_progs_query *skel;
> > + int err, map_fd, verdict_fd, duration = 0;
> > + __u32 attach_flags = 0;
> > + __u32 prog_ids[3] = {};
> > + __u32 prog_cnt = 3;
> > +
> > + skel = test_sockmap_progs_query__open_and_load();
> > + if (!ASSERT_OK_PTR(skel,
> "test_sockmap_progs_query__open_and_load"))
> > + return;
> > +
> > + map_fd = bpf_map__fd(skel->maps.sock_map);
> > +
> > + if (attach_type == BPF_SK_MSG_VERDICT)
> > + verdict_fd =
> bpf_program__fd(skel->progs.prog_skmsg_verdict);
> > + else
> > + verdict_fd =
> bpf_program__fd(skel->progs.prog_skb_verdict);
> > +
> > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > + &attach_flags, prog_ids, &prog_cnt);
> > + if (!ASSERT_OK(err, "bpf_prog_query failed"))
> > + goto out;
> > +
> > + if (!ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query"))
> > + goto out;
> > +
> > + if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
> > + goto out;
> > +
> > + err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> > + if (!ASSERT_OK(err, "bpf_prog_attach failed"))
> > + goto out;
> > +
> > + prog_cnt = 1;
> > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > + &attach_flags, prog_ids, &prog_cnt);
> > +
> > + ASSERT_OK(err, "bpf_prog_query failed");
> > + ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
> > + ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
> > + ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
> > + "wrong prog_ids on query");
>
> See how much easier it is to follow these tests, why didn't you do the
> same with err, attach_flags and prog above?
It is recommended by Yonghong Song to increase the test coverage.
>
>
> > +
> > + bpf_prog_detach2(verdict_fd, map_fd, attach_type);
> > +out:
> > + test_sockmap_progs_query__destroy(skel);
> > +}
> > +
> > void test_sockmap_basic(void)
> > {
> > if (test__start_subtest("sockmap create_update_free"))
> > @@ -341,4 +405,10 @@ void test_sockmap_basic(void)
> >
> test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
> >
> BPF_SK_SKB_VERDICT);
> > }
> > + if (test__start_subtest("sockmap progs query")) {
> > + test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
> > +
> test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
> > +
> test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
> > + test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
>
> Why are these not separate subtests? What's the benefit of bundling
> them into one subtest?
>
These are essentially doing the same thing, just for different program attach types.
> > + }
> > }
> > diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > new file mode 100644
> > index 000000000000..9d58d61c0dee
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > @@ -0,0 +1,24 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_SOCKMAP);
> > + __uint(max_entries, 1);
> > + __type(key, __u32);
> > + __type(value, __u64);
> > +} sock_map SEC(".maps");
> > +
> > +SEC("sk_skb")
> > +int prog_skb_verdict(struct __sk_buff *skb)
> > +{
> > + return SK_PASS;
> > +}
> > +
> > +SEC("sk_msg")
> > +int prog_skmsg_verdict(struct sk_msg_md *msg)
> > +{
> > + return SK_PASS;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > --
> > 2.27.0
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
2022-01-15 2:34 [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap zhudi (E)
@ 2022-01-15 2:48 ` Andrii Nakryiko
0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2022-01-15 2:48 UTC (permalink / raw)
To: zhudi (E)
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
john fastabend, KP Singh, Jakub Sitnicki, bpf, open list,
Luzhihao (luzhihao, Euler), Chenxiang (EulerOS)
On Fri, Jan 14, 2022 at 6:34 PM zhudi (E) <zhudi2@huawei.com> wrote:
>
> > On Thu, Jan 13, 2022 at 1:01 AM Di Zhu <zhudi2@huawei.com> wrote:
> > >
> > > Add test for querying progs attached to sockmap. we use an existing
> > > libbpf query interface to query prog cnt before and after progs
> > > attaching to sockmap and check whether the queried prog id is right.
> > >
> > > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > > Acked-by: Yonghong Song <yhs@fb.com>
> > > ---
> > > .../selftests/bpf/prog_tests/sockmap_basic.c | 70 +++++++++++++++++++
> > > .../bpf/progs/test_sockmap_progs_query.c | 24 +++++++
> > > 2 files changed, 94 insertions(+)
> > > create mode 100644
> > tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > > index 85db0f4cdd95..06923ea44bad 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > > @@ -8,6 +8,7 @@
> > > #include "test_sockmap_update.skel.h"
> > > #include "test_sockmap_invalid_update.skel.h"
> > > #include "test_sockmap_skb_verdict_attach.skel.h"
> > > +#include "test_sockmap_progs_query.skel.h"
> > > #include "bpf_iter_sockmap.skel.h"
> > >
> > > #define TCP_REPAIR 19 /* TCP sock is under repair
> > right now */
> > > @@ -315,6 +316,69 @@ static void test_sockmap_skb_verdict_attach(enum
> > bpf_attach_type first,
> > > test_sockmap_skb_verdict_attach__destroy(skel);
> > > }
> > >
> > > +static __u32 query_prog_id(int prog_fd)
> > > +{
> > > + struct bpf_prog_info info = {};
> > > + __u32 info_len = sizeof(info);
> > > + int err;
> > > +
> > > + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> > > + if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
> > > + !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
> > > + return 0;
> > > +
> > > + return info.id;
> > > +}
> > > +
> > > +static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
> > > +{
> > > + struct test_sockmap_progs_query *skel;
> > > + int err, map_fd, verdict_fd, duration = 0;
> > > + __u32 attach_flags = 0;
> > > + __u32 prog_ids[3] = {};
> > > + __u32 prog_cnt = 3;
> > > +
> > > + skel = test_sockmap_progs_query__open_and_load();
> > > + if (!ASSERT_OK_PTR(skel,
> > "test_sockmap_progs_query__open_and_load"))
> > > + return;
> > > +
> > > + map_fd = bpf_map__fd(skel->maps.sock_map);
> > > +
> > > + if (attach_type == BPF_SK_MSG_VERDICT)
> > > + verdict_fd =
> > bpf_program__fd(skel->progs.prog_skmsg_verdict);
> > > + else
> > > + verdict_fd =
> > bpf_program__fd(skel->progs.prog_skb_verdict);
> > > +
> > > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > > + &attach_flags, prog_ids, &prog_cnt);
> > > + if (!ASSERT_OK(err, "bpf_prog_query failed"))
> > > + goto out;
> > > +
> > > + if (!ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query"))
> > > + goto out;
> > > +
> > > + if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
> > > + goto out;
I mean here that you can do just
ASSERT_OK(err, ...);
ASSERT_EQ(attach_flags, ...);
ASSERT_EQ(prog_cnt, ...);
No if + goto necessary.
> > > +
> > > + err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> > > + if (!ASSERT_OK(err, "bpf_prog_attach failed"))
> > > + goto out;
> > > +
> > > + prog_cnt = 1;
> > > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > > + &attach_flags, prog_ids, &prog_cnt);
> > > +
> > > + ASSERT_OK(err, "bpf_prog_query failed");
> > > + ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
> > > + ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
> > > + ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
> > > + "wrong prog_ids on query");
> >
> > See how much easier it is to follow these tests, why didn't you do the
> > same with err, attach_flags and prog above?
>
> It is recommended by Yonghong Song to increase the test coverage.
see above
>
> >
> >
> > > +
> > > + bpf_prog_detach2(verdict_fd, map_fd, attach_type);
> > > +out:
> > > + test_sockmap_progs_query__destroy(skel);
> > > +}
> > > +
> > > void test_sockmap_basic(void)
> > > {
> > > if (test__start_subtest("sockmap create_update_free"))
> > > @@ -341,4 +405,10 @@ void test_sockmap_basic(void)
> > >
> > test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
> > >
> > BPF_SK_SKB_VERDICT);
> > > }
> > > + if (test__start_subtest("sockmap progs query")) {
> > > + test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
> > > +
> > test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
> > > +
> > test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
> > > + test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
> >
> > Why are these not separate subtests? What's the benefit of bundling
> > them into one subtest?
> >
>
> These are essentially doing the same thing, just for different program attach types.
Right, so they are independent subtests, no? Not separate tests, but
not one subtest either.
>
> > > + }
> > > }
> > > diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > > new file mode 100644
> > > index 000000000000..9d58d61c0dee
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > > @@ -0,0 +1,24 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include "vmlinux.h"
> > > +#include <bpf/bpf_helpers.h>
> > > +
> > > +struct {
> > > + __uint(type, BPF_MAP_TYPE_SOCKMAP);
> > > + __uint(max_entries, 1);
> > > + __type(key, __u32);
> > > + __type(value, __u64);
> > > +} sock_map SEC(".maps");
> > > +
> > > +SEC("sk_skb")
> > > +int prog_skb_verdict(struct __sk_buff *skb)
> > > +{
> > > + return SK_PASS;
> > > +}
> > > +
> > > +SEC("sk_msg")
> > > +int prog_skmsg_verdict(struct sk_msg_md *msg)
> > > +{
> > > + return SK_PASS;
> > > +}
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > --
> > > 2.27.0
> > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
@ 2022-01-17 2:59 zhudi (E)
0 siblings, 0 replies; 10+ messages in thread
From: zhudi (E) @ 2022-01-17 2:59 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
john fastabend, KP Singh, Jakub Sitnicki, bpf, open list,
Luzhihao (luzhihao, Euler), Chenxiang (EulerOS)
> On Fri, Jan 14, 2022 at 6:34 PM zhudi (E) <zhudi2@huawei.com> wrote:
> >
> > > On Thu, Jan 13, 2022 at 1:01 AM Di Zhu <zhudi2@huawei.com> wrote:
> > > >
> > > > Add test for querying progs attached to sockmap. we use an existing
> > > > libbpf query interface to query prog cnt before and after progs
> > > > attaching to sockmap and check whether the queried prog id is right.
> > > >
> > > > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > > > Acked-by: Yonghong Song <yhs@fb.com>
> > > > ---
> > > > .../selftests/bpf/prog_tests/sockmap_basic.c | 70
> +++++++++++++++++++
> > > > .../bpf/progs/test_sockmap_progs_query.c | 24 +++++++
> > > > 2 files changed, 94 insertions(+)
> > > > create mode 100644
> > > tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > > b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > > > index 85db0f4cdd95..06923ea44bad 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > > > @@ -8,6 +8,7 @@
> > > > #include "test_sockmap_update.skel.h"
> > > > #include "test_sockmap_invalid_update.skel.h"
> > > > #include "test_sockmap_skb_verdict_attach.skel.h"
> > > > +#include "test_sockmap_progs_query.skel.h"
> > > > #include "bpf_iter_sockmap.skel.h"
> > > >
> > > > #define TCP_REPAIR 19 /* TCP sock is under repair
> > > right now */
> > > > @@ -315,6 +316,69 @@ static void
> test_sockmap_skb_verdict_attach(enum
> > > bpf_attach_type first,
> > > > test_sockmap_skb_verdict_attach__destroy(skel);
> > > > }
> > > >
> > > > +static __u32 query_prog_id(int prog_fd)
> > > > +{
> > > > + struct bpf_prog_info info = {};
> > > > + __u32 info_len = sizeof(info);
> > > > + int err;
> > > > +
> > > > + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> > > > + if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
> > > > + !ASSERT_EQ(info_len, sizeof(info),
> "bpf_obj_get_info_by_fd"))
> > > > + return 0;
> > > > +
> > > > + return info.id;
> > > > +}
> > > > +
> > > > +static void test_sockmap_progs_query(enum bpf_attach_type
> attach_type)
> > > > +{
> > > > + struct test_sockmap_progs_query *skel;
> > > > + int err, map_fd, verdict_fd, duration = 0;
> > > > + __u32 attach_flags = 0;
> > > > + __u32 prog_ids[3] = {};
> > > > + __u32 prog_cnt = 3;
> > > > +
> > > > + skel = test_sockmap_progs_query__open_and_load();
> > > > + if (!ASSERT_OK_PTR(skel,
> > > "test_sockmap_progs_query__open_and_load"))
> > > > + return;
> > > > +
> > > > + map_fd = bpf_map__fd(skel->maps.sock_map);
> > > > +
> > > > + if (attach_type == BPF_SK_MSG_VERDICT)
> > > > + verdict_fd =
> > > bpf_program__fd(skel->progs.prog_skmsg_verdict);
> > > > + else
> > > > + verdict_fd =
> > > bpf_program__fd(skel->progs.prog_skb_verdict);
> > > > +
> > > > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > > > + &attach_flags, prog_ids, &prog_cnt);
> > > > + if (!ASSERT_OK(err, "bpf_prog_query failed"))
> > > > + goto out;
> > > > +
> > > > + if (!ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query"))
> > > > + goto out;
> > > > +
> > > > + if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
> > > > + goto out;
>
> I mean here that you can do just
>
> ASSERT_OK(err, ...);
> ASSERT_EQ(attach_flags, ...);
> ASSERT_EQ(prog_cnt, ...);
>
> No if + goto necessary.
I see what you mean. I'll modify the code, thanks
>
> > > > +
> > > > + err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> > > > + if (!ASSERT_OK(err, "bpf_prog_attach failed"))
> > > > + goto out;
> > > > +
> > > > + prog_cnt = 1;
> > > > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > > > + &attach_flags, prog_ids, &prog_cnt);
> > > > +
> > > > + ASSERT_OK(err, "bpf_prog_query failed");
> > > > + ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
> > > > + ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
> > > > + ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
> > > > + "wrong prog_ids on query");
> > >
> > > See how much easier it is to follow these tests, why didn't you do the
> > > same with err, attach_flags and prog above?
> >
> > It is recommended by Yonghong Song to increase the test coverage.
>
> see above
>
> >
> > >
> > >
> > > > +
> > > > + bpf_prog_detach2(verdict_fd, map_fd, attach_type);
> > > > +out:
> > > > + test_sockmap_progs_query__destroy(skel);
> > > > +}
> > > > +
> > > > void test_sockmap_basic(void)
> > > > {
> > > > if (test__start_subtest("sockmap create_update_free"))
> > > > @@ -341,4 +405,10 @@ void test_sockmap_basic(void)
> > > >
> > > test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
> > > >
> > > BPF_SK_SKB_VERDICT);
> > > > }
> > > > + if (test__start_subtest("sockmap progs query")) {
> > > > + test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
> > > > +
> > > test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
> > > > +
> > > test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
> > > > + test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
> > >
> > > Why are these not separate subtests? What's the benefit of bundling
> > > them into one subtest?
> > >
> >
> > These are essentially doing the same thing, just for different program attach
> types.
>
> Right, so they are independent subtests, no? Not separate tests, but
> not one subtest either.
Ok, I'll split it into several subsets.
> >
> > > > + }
> > > > }
> > > > diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > > b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > > > new file mode 100644
> > > > index 000000000000..9d58d61c0dee
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> > > > @@ -0,0 +1,24 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +#include "vmlinux.h"
> > > > +#include <bpf/bpf_helpers.h>
> > > > +
> > > > +struct {
> > > > + __uint(type, BPF_MAP_TYPE_SOCKMAP);
> > > > + __uint(max_entries, 1);
> > > > + __type(key, __u32);
> > > > + __type(value, __u64);
> > > > +} sock_map SEC(".maps");
> > > > +
> > > > +SEC("sk_skb")
> > > > +int prog_skb_verdict(struct __sk_buff *skb)
> > > > +{
> > > > + return SK_PASS;
> > > > +}
> > > > +
> > > > +SEC("sk_msg")
> > > > +int prog_skmsg_verdict(struct sk_msg_md *msg)
> > > > +{
> > > > + return SK_PASS;
> > > > +}
> > > > +
> > > > +char _license[] SEC("license") = "GPL";
> > > > --
> > > > 2.27.0
> > > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
2022-01-13 9:00 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
@ 2022-01-15 1:18 ` Andrii Nakryiko
0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2022-01-15 1:18 UTC (permalink / raw)
To: Di Zhu
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
john fastabend, KP Singh, Jakub Sitnicki, bpf, open list,
luzhihao, rose.chen
On Thu, Jan 13, 2022 at 1:01 AM Di Zhu <zhudi2@huawei.com> wrote:
>
> Add test for querying progs attached to sockmap. we use an existing
> libbpf query interface to query prog cnt before and after progs
> attaching to sockmap and check whether the queried prog id is right.
>
> Signed-off-by: Di Zhu <zhudi2@huawei.com>
> Acked-by: Yonghong Song <yhs@fb.com>
> ---
> .../selftests/bpf/prog_tests/sockmap_basic.c | 70 +++++++++++++++++++
> .../bpf/progs/test_sockmap_progs_query.c | 24 +++++++
> 2 files changed, 94 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index 85db0f4cdd95..06923ea44bad 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -8,6 +8,7 @@
> #include "test_sockmap_update.skel.h"
> #include "test_sockmap_invalid_update.skel.h"
> #include "test_sockmap_skb_verdict_attach.skel.h"
> +#include "test_sockmap_progs_query.skel.h"
> #include "bpf_iter_sockmap.skel.h"
>
> #define TCP_REPAIR 19 /* TCP sock is under repair right now */
> @@ -315,6 +316,69 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
> test_sockmap_skb_verdict_attach__destroy(skel);
> }
>
> +static __u32 query_prog_id(int prog_fd)
> +{
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> + int err;
> +
> + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> + if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
> + !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
> + return 0;
> +
> + return info.id;
> +}
> +
> +static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
> +{
> + struct test_sockmap_progs_query *skel;
> + int err, map_fd, verdict_fd, duration = 0;
> + __u32 attach_flags = 0;
> + __u32 prog_ids[3] = {};
> + __u32 prog_cnt = 3;
> +
> + skel = test_sockmap_progs_query__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "test_sockmap_progs_query__open_and_load"))
> + return;
> +
> + map_fd = bpf_map__fd(skel->maps.sock_map);
> +
> + if (attach_type == BPF_SK_MSG_VERDICT)
> + verdict_fd = bpf_program__fd(skel->progs.prog_skmsg_verdict);
> + else
> + verdict_fd = bpf_program__fd(skel->progs.prog_skb_verdict);
> +
> + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> + &attach_flags, prog_ids, &prog_cnt);
> + if (!ASSERT_OK(err, "bpf_prog_query failed"))
> + goto out;
> +
> + if (!ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query"))
> + goto out;
> +
> + if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
> + goto out;
> +
> + err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> + if (!ASSERT_OK(err, "bpf_prog_attach failed"))
> + goto out;
> +
> + prog_cnt = 1;
> + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> + &attach_flags, prog_ids, &prog_cnt);
> +
> + ASSERT_OK(err, "bpf_prog_query failed");
> + ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
> + ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
> + ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
> + "wrong prog_ids on query");
See how much easier it is to follow these tests, why didn't you do the
same with err, attach_flags and prog above?
> +
> + bpf_prog_detach2(verdict_fd, map_fd, attach_type);
> +out:
> + test_sockmap_progs_query__destroy(skel);
> +}
> +
> void test_sockmap_basic(void)
> {
> if (test__start_subtest("sockmap create_update_free"))
> @@ -341,4 +405,10 @@ void test_sockmap_basic(void)
> test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
> BPF_SK_SKB_VERDICT);
> }
> + if (test__start_subtest("sockmap progs query")) {
> + test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
> + test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
> + test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
> + test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
Why are these not separate subtests? What's the benefit of bundling
them into one subtest?
> + }
> }
> diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> new file mode 100644
> index 000000000000..9d58d61c0dee
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_SOCKMAP);
> + __uint(max_entries, 1);
> + __type(key, __u32);
> + __type(value, __u64);
> +} sock_map SEC(".maps");
> +
> +SEC("sk_skb")
> +int prog_skb_verdict(struct __sk_buff *skb)
> +{
> + return SK_PASS;
> +}
> +
> +SEC("sk_msg")
> +int prog_skmsg_verdict(struct sk_msg_md *msg)
> +{
> + return SK_PASS;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
2022-01-13 9:00 [PATCH bpf-next v5 1/2] bpf: support " Di Zhu
@ 2022-01-13 9:00 ` Di Zhu
2022-01-15 1:18 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Di Zhu @ 2022-01-13 9:00 UTC (permalink / raw)
To: ast, davem, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh, jakub
Cc: bpf, linux-kernel, zhudi2, luzhihao, rose.chen
Add test for querying progs attached to sockmap. we use an existing
libbpf query interface to query prog cnt before and after progs
attaching to sockmap and check whether the queried prog id is right.
Signed-off-by: Di Zhu <zhudi2@huawei.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
.../selftests/bpf/prog_tests/sockmap_basic.c | 70 +++++++++++++++++++
.../bpf/progs/test_sockmap_progs_query.c | 24 +++++++
2 files changed, 94 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 85db0f4cdd95..06923ea44bad 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -8,6 +8,7 @@
#include "test_sockmap_update.skel.h"
#include "test_sockmap_invalid_update.skel.h"
#include "test_sockmap_skb_verdict_attach.skel.h"
+#include "test_sockmap_progs_query.skel.h"
#include "bpf_iter_sockmap.skel.h"
#define TCP_REPAIR 19 /* TCP sock is under repair right now */
@@ -315,6 +316,69 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
test_sockmap_skb_verdict_attach__destroy(skel);
}
+static __u32 query_prog_id(int prog_fd)
+{
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ int err;
+
+ err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+ if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
+ !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
+ return 0;
+
+ return info.id;
+}
+
+static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
+{
+ struct test_sockmap_progs_query *skel;
+ int err, map_fd, verdict_fd, duration = 0;
+ __u32 attach_flags = 0;
+ __u32 prog_ids[3] = {};
+ __u32 prog_cnt = 3;
+
+ skel = test_sockmap_progs_query__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_sockmap_progs_query__open_and_load"))
+ return;
+
+ map_fd = bpf_map__fd(skel->maps.sock_map);
+
+ if (attach_type == BPF_SK_MSG_VERDICT)
+ verdict_fd = bpf_program__fd(skel->progs.prog_skmsg_verdict);
+ else
+ verdict_fd = bpf_program__fd(skel->progs.prog_skb_verdict);
+
+ err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+ &attach_flags, prog_ids, &prog_cnt);
+ if (!ASSERT_OK(err, "bpf_prog_query failed"))
+ goto out;
+
+ if (!ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query"))
+ goto out;
+
+ if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
+ goto out;
+
+ err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
+ if (!ASSERT_OK(err, "bpf_prog_attach failed"))
+ goto out;
+
+ prog_cnt = 1;
+ err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+ &attach_flags, prog_ids, &prog_cnt);
+
+ ASSERT_OK(err, "bpf_prog_query failed");
+ ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
+ ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
+ ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
+ "wrong prog_ids on query");
+
+ bpf_prog_detach2(verdict_fd, map_fd, attach_type);
+out:
+ test_sockmap_progs_query__destroy(skel);
+}
+
void test_sockmap_basic(void)
{
if (test__start_subtest("sockmap create_update_free"))
@@ -341,4 +405,10 @@ void test_sockmap_basic(void)
test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
BPF_SK_SKB_VERDICT);
}
+ if (test__start_subtest("sockmap progs query")) {
+ test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
+ test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
+ test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
+ test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
+ }
}
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
new file mode 100644
index 000000000000..9d58d61c0dee
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+struct {
+ __uint(type, BPF_MAP_TYPE_SOCKMAP);
+ __uint(max_entries, 1);
+ __type(key, __u32);
+ __type(value, __u64);
+} sock_map SEC(".maps");
+
+SEC("sk_skb")
+int prog_skb_verdict(struct __sk_buff *skb)
+{
+ return SK_PASS;
+}
+
+SEC("sk_msg")
+int prog_skmsg_verdict(struct sk_msg_md *msg)
+{
+ return SK_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
2021-11-05 1:57 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
@ 2021-11-05 4:24 ` Yonghong Song
0 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2021-11-05 4:24 UTC (permalink / raw)
To: Di Zhu, davem, ast, daniel, andrii, kafai, songliubraving,
john.fastabend, kpsingh, jakub
Cc: bpf, linux-kernel
On 11/4/21 6:57 PM, Di Zhu wrote:
> Add test for querying progs attached to sockmap. we use an existing
> libbpf query interface to query prog cnt before and after progs
> attaching to sockmap and check whether the queried prog id is right.
>
> Signed-off-by: Di Zhu <zhudi2@huawei.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
2021-11-05 1:57 [PATCH bpf-next v5 1/2] bpf: support " Di Zhu
@ 2021-11-05 1:57 ` Di Zhu
2021-11-05 4:24 ` Yonghong Song
0 siblings, 1 reply; 10+ messages in thread
From: Di Zhu @ 2021-11-05 1:57 UTC (permalink / raw)
To: davem, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh, jakub
Cc: bpf, linux-kernel, zhudi2
Add test for querying progs attached to sockmap. we use an existing
libbpf query interface to query prog cnt before and after progs
attaching to sockmap and check whether the queried prog id is right.
Signed-off-by: Di Zhu <zhudi2@huawei.com>
---
.../selftests/bpf/prog_tests/sockmap_basic.c | 70 +++++++++++++++++++
.../bpf/progs/test_sockmap_progs_query.c | 24 +++++++
2 files changed, 94 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 1352ec104149..4d94b72f2d8d 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -8,6 +8,7 @@
#include "test_sockmap_update.skel.h"
#include "test_sockmap_invalid_update.skel.h"
#include "test_sockmap_skb_verdict_attach.skel.h"
+#include "test_sockmap_progs_query.skel.h"
#include "bpf_iter_sockmap.skel.h"
#define TCP_REPAIR 19 /* TCP sock is under repair right now */
@@ -315,6 +316,69 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
test_sockmap_skb_verdict_attach__destroy(skel);
}
+static __u32 query_prog_id(int prog_fd)
+{
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ int err;
+
+ err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+ if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
+ !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
+ return 0;
+
+ return info.id;
+}
+
+static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
+{
+ struct test_sockmap_progs_query *skel;
+ int err, map_fd, verdict_fd, duration = 0;
+ __u32 attach_flags = 0;
+ __u32 prog_ids[3] = {};
+ __u32 prog_cnt = 3;
+
+ skel = test_sockmap_progs_query__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_sockmap_progs_query__open_and_load"))
+ return;
+
+ map_fd = bpf_map__fd(skel->maps.sock_map);
+
+ if (attach_type == BPF_SK_MSG_VERDICT)
+ verdict_fd = bpf_program__fd(skel->progs.prog_skmsg_verdict);
+ else
+ verdict_fd = bpf_program__fd(skel->progs.prog_skb_verdict);
+
+ err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+ &attach_flags, prog_ids, &prog_cnt);
+ if (!ASSERT_OK(err, "bpf_prog_query failed"))
+ goto out;
+
+ if (!ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query"))
+ goto out;
+
+ if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
+ goto out;
+
+ err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
+ if (!ASSERT_OK(err, "bpf_prog_attach failed"))
+ goto out;
+
+ prog_cnt = 1;
+ err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+ &attach_flags, prog_ids, &prog_cnt);
+
+ ASSERT_OK(err, "bpf_prog_query failed");
+ ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
+ ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
+ ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
+ "wrong prog_ids on query");
+
+ bpf_prog_detach2(verdict_fd, map_fd, attach_type);
+out:
+ test_sockmap_progs_query__destroy(skel);
+}
+
void test_sockmap_basic(void)
{
if (test__start_subtest("sockmap create_update_free"))
@@ -341,4 +405,10 @@ void test_sockmap_basic(void)
test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
BPF_SK_SKB_VERDICT);
}
+ if (test__start_subtest("sockmap progs query")) {
+ test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
+ test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
+ test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
+ test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
+ }
}
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
new file mode 100644
index 000000000000..9d58d61c0dee
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+struct {
+ __uint(type, BPF_MAP_TYPE_SOCKMAP);
+ __uint(max_entries, 1);
+ __type(key, __u32);
+ __type(value, __u64);
+} sock_map SEC(".maps");
+
+SEC("sk_skb")
+int prog_skb_verdict(struct __sk_buff *skb)
+{
+ return SK_PASS;
+}
+
+SEC("sk_msg")
+int prog_skmsg_verdict(struct sk_msg_md *msg)
+{
+ return SK_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
@ 2021-11-04 6:09 zhudi (E)
0 siblings, 0 replies; 10+ messages in thread
From: zhudi (E) @ 2021-11-04 6:09 UTC (permalink / raw)
To: Yonghong Song, davem, ast, daniel, andrii, kafai, songliubraving,
john.fastabend, kpsingh, jakub
Cc: bpf, linux-kernel
> On 11/3/21 6:07 PM, Di Zhu wrote:
> > Add test for querying progs attached to sockmap. we use an existing
> > libbpf query interface to query prog cnt before and after progs
> > attaching to sockmap and check whether the queried prog id is right.
> >
> > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > ---
> > .../selftests/bpf/prog_tests/sockmap_basic.c | 75 +++++++++++++++++++
> > .../bpf/progs/test_sockmap_progs_query.c | 24 ++++++
> > 2 files changed, 99 insertions(+)
> > create mode 100644
> tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > index 1352ec104149..de8f91d91240 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -8,6 +8,7 @@
> > #include "test_sockmap_update.skel.h"
> > #include "test_sockmap_invalid_update.skel.h"
> > #include "test_sockmap_skb_verdict_attach.skel.h"
> > +#include "test_sockmap_progs_query.skel.h"
> > #include "bpf_iter_sockmap.skel.h"
> >
> > #define TCP_REPAIR 19 /* TCP sock is under repair right now */
> > @@ -315,6 +316,74 @@ static void test_sockmap_skb_verdict_attach(enum
> bpf_attach_type first,
> > test_sockmap_skb_verdict_attach__destroy(skel);
> > }
> >
> > +static __u32 query_prog_id(int prog_fd)
> > +{
> > + struct bpf_prog_info info = {};
> > + __u32 info_len = sizeof(info);
> > + int err;
> > +
> > + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> > + if (CHECK_FAIL(err || info_len != sizeof(info))) {
> > + perror("bpf_obj_get_info_by_fd");
>
> Please use ASSERT_* macros. These macros are defined in test_progs.h.
> We may have some old files still using CHECK* which are not converted
> to ASSERT* yet. But for new contributions, we would like to use
> ASSERT* from start. You can check other prog_tests/*.c files for
> examples.
>
> For the above example, you can do
> if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
> !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
> return 0;
I refer to the implementation of the old codes, I will modify it
Thanks,
> > + return 0;
> > + }
> > +
> > + return info.id;
> [...]
> > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > + &attach_flags, prog_ids, &prog_cnt);
> > + if (CHECK(err, "bpf_prog_query", "failed\n"))
> > + goto out;
>
> In this case, you can use
> if (!ASSERT_OK(err, "bpf_prog_query"))
> goto out;
>
> Please also change below other CHECK usages.
>
> > +
> > + if (CHECK(attach_flags != 0, "bpf_prog_query",
> > + "wrong attach_flags on query: %u", attach_flags))
> > + goto out;
> > +
> > + if (CHECK(prog_cnt != 0, "bpf_prog_query",
> > + "wrong program count on query: %u", prog_cnt))
> > + goto out;
> > +
> > + err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> > + if (CHECK(err, "bpf_prog_attach", "failed\n"))
> > + goto out;
> > +
> > + prog_cnt = 1;
> > + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> > + &attach_flags, prog_ids, &prog_cnt);
> > +
> > + CHECK(err, "bpf_prog_query", "failed\n");
> > + CHECK(attach_flags != 0, "bpf_prog_query attach_flags",
> > + "wrong attach_flags on query: %u", attach_flags);
> > + CHECK(prog_cnt != 1, "bpf_prog_query prog_cnt",
> > + "wrong program count on query: %u", prog_cnt);
> > + CHECK(prog_ids[0] != query_prog_id(verdict_fd), "bpf_prog_query",
> > + "wrong prog_ids on query: %u", prog_ids[0]);
> [...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
2021-11-04 1:07 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
@ 2021-11-04 5:51 ` Yonghong Song
0 siblings, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2021-11-04 5:51 UTC (permalink / raw)
To: Di Zhu, davem, ast, daniel, andrii, kafai, songliubraving,
john.fastabend, kpsingh, jakub
Cc: bpf, linux-kernel
On 11/3/21 6:07 PM, Di Zhu wrote:
> Add test for querying progs attached to sockmap. we use an existing
> libbpf query interface to query prog cnt before and after progs
> attaching to sockmap and check whether the queried prog id is right.
>
> Signed-off-by: Di Zhu <zhudi2@huawei.com>
> ---
> .../selftests/bpf/prog_tests/sockmap_basic.c | 75 +++++++++++++++++++
> .../bpf/progs/test_sockmap_progs_query.c | 24 ++++++
> 2 files changed, 99 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index 1352ec104149..de8f91d91240 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -8,6 +8,7 @@
> #include "test_sockmap_update.skel.h"
> #include "test_sockmap_invalid_update.skel.h"
> #include "test_sockmap_skb_verdict_attach.skel.h"
> +#include "test_sockmap_progs_query.skel.h"
> #include "bpf_iter_sockmap.skel.h"
>
> #define TCP_REPAIR 19 /* TCP sock is under repair right now */
> @@ -315,6 +316,74 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
> test_sockmap_skb_verdict_attach__destroy(skel);
> }
>
> +static __u32 query_prog_id(int prog_fd)
> +{
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> + int err;
> +
> + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> + if (CHECK_FAIL(err || info_len != sizeof(info))) {
> + perror("bpf_obj_get_info_by_fd");
Please use ASSERT_* macros. These macros are defined in test_progs.h.
We may have some old files still using CHECK* which are not converted
to ASSERT* yet. But for new contributions, we would like to use
ASSERT* from start. You can check other prog_tests/*.c files for
examples.
For the above example, you can do
if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
!ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
return 0;
> + return 0;
> + }
> +
> + return info.id;
[...]
> + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> + &attach_flags, prog_ids, &prog_cnt);
> + if (CHECK(err, "bpf_prog_query", "failed\n"))
> + goto out;
In this case, you can use
if (!ASSERT_OK(err, "bpf_prog_query"))
goto out;
Please also change below other CHECK usages.
> +
> + if (CHECK(attach_flags != 0, "bpf_prog_query",
> + "wrong attach_flags on query: %u", attach_flags))
> + goto out;
> +
> + if (CHECK(prog_cnt != 0, "bpf_prog_query",
> + "wrong program count on query: %u", prog_cnt))
> + goto out;
> +
> + err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> + if (CHECK(err, "bpf_prog_attach", "failed\n"))
> + goto out;
> +
> + prog_cnt = 1;
> + err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> + &attach_flags, prog_ids, &prog_cnt);
> +
> + CHECK(err, "bpf_prog_query", "failed\n");
> + CHECK(attach_flags != 0, "bpf_prog_query attach_flags",
> + "wrong attach_flags on query: %u", attach_flags);
> + CHECK(prog_cnt != 1, "bpf_prog_query prog_cnt",
> + "wrong program count on query: %u", prog_cnt);
> + CHECK(prog_ids[0] != query_prog_id(verdict_fd), "bpf_prog_query",
> + "wrong prog_ids on query: %u", prog_ids[0]);
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
2021-11-04 1:07 [PATCH bpf-next v5 1/2] bpf: support " Di Zhu
@ 2021-11-04 1:07 ` Di Zhu
2021-11-04 5:51 ` Yonghong Song
0 siblings, 1 reply; 10+ messages in thread
From: Di Zhu @ 2021-11-04 1:07 UTC (permalink / raw)
To: davem, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh, jakub
Cc: bpf, linux-kernel, zhudi2
Add test for querying progs attached to sockmap. we use an existing
libbpf query interface to query prog cnt before and after progs
attaching to sockmap and check whether the queried prog id is right.
Signed-off-by: Di Zhu <zhudi2@huawei.com>
---
.../selftests/bpf/prog_tests/sockmap_basic.c | 75 +++++++++++++++++++
.../bpf/progs/test_sockmap_progs_query.c | 24 ++++++
2 files changed, 99 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 1352ec104149..de8f91d91240 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -8,6 +8,7 @@
#include "test_sockmap_update.skel.h"
#include "test_sockmap_invalid_update.skel.h"
#include "test_sockmap_skb_verdict_attach.skel.h"
+#include "test_sockmap_progs_query.skel.h"
#include "bpf_iter_sockmap.skel.h"
#define TCP_REPAIR 19 /* TCP sock is under repair right now */
@@ -315,6 +316,74 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
test_sockmap_skb_verdict_attach__destroy(skel);
}
+static __u32 query_prog_id(int prog_fd)
+{
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ int err;
+
+ err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+ if (CHECK_FAIL(err || info_len != sizeof(info))) {
+ perror("bpf_obj_get_info_by_fd");
+ return 0;
+ }
+
+ return info.id;
+}
+
+static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
+{
+ struct test_sockmap_progs_query *skel;
+ int err, map_fd, verdict_fd, duration = 0;
+ __u32 attach_flags = 0;
+ __u32 prog_ids[3] = {};
+ __u32 prog_cnt = 3;
+
+ skel = test_sockmap_progs_query__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_sockmap_progs_query__open_and_load"))
+ return;
+
+ map_fd = bpf_map__fd(skel->maps.sock_map);
+
+ if (attach_type == BPF_SK_MSG_VERDICT)
+ verdict_fd = bpf_program__fd(skel->progs.prog_skmsg_verdict);
+ else
+ verdict_fd = bpf_program__fd(skel->progs.prog_skb_verdict);
+
+ err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+ &attach_flags, prog_ids, &prog_cnt);
+ if (CHECK(err, "bpf_prog_query", "failed\n"))
+ goto out;
+
+ if (CHECK(attach_flags != 0, "bpf_prog_query",
+ "wrong attach_flags on query: %u", attach_flags))
+ goto out;
+
+ if (CHECK(prog_cnt != 0, "bpf_prog_query",
+ "wrong program count on query: %u", prog_cnt))
+ goto out;
+
+ err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
+ if (CHECK(err, "bpf_prog_attach", "failed\n"))
+ goto out;
+
+ prog_cnt = 1;
+ err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+ &attach_flags, prog_ids, &prog_cnt);
+
+ CHECK(err, "bpf_prog_query", "failed\n");
+ CHECK(attach_flags != 0, "bpf_prog_query attach_flags",
+ "wrong attach_flags on query: %u", attach_flags);
+ CHECK(prog_cnt != 1, "bpf_prog_query prog_cnt",
+ "wrong program count on query: %u", prog_cnt);
+ CHECK(prog_ids[0] != query_prog_id(verdict_fd), "bpf_prog_query",
+ "wrong prog_ids on query: %u", prog_ids[0]);
+
+ bpf_prog_detach2(verdict_fd, map_fd, attach_type);
+out:
+ test_sockmap_progs_query__destroy(skel);
+}
+
void test_sockmap_basic(void)
{
if (test__start_subtest("sockmap create_update_free"))
@@ -341,4 +410,10 @@ void test_sockmap_basic(void)
test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
BPF_SK_SKB_VERDICT);
}
+ if (test__start_subtest("sockmap progs query")) {
+ test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
+ test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
+ test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
+ test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
+ }
}
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
new file mode 100644
index 000000000000..9d58d61c0dee
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+struct {
+ __uint(type, BPF_MAP_TYPE_SOCKMAP);
+ __uint(max_entries, 1);
+ __type(key, __u32);
+ __type(value, __u64);
+} sock_map SEC(".maps");
+
+SEC("sk_skb")
+int prog_skb_verdict(struct __sk_buff *skb)
+{
+ return SK_PASS;
+}
+
+SEC("sk_msg")
+int prog_skmsg_verdict(struct sk_msg_md *msg)
+{
+ return SK_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.27.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-17 3:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-15 2:34 [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap zhudi (E)
2022-01-15 2:48 ` Andrii Nakryiko
-- strict thread matches above, loose matches on Subject: below --
2022-01-17 2:59 zhudi (E)
2022-01-13 9:00 [PATCH bpf-next v5 1/2] bpf: support " Di Zhu
2022-01-13 9:00 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
2022-01-15 1:18 ` Andrii Nakryiko
2021-11-05 1:57 [PATCH bpf-next v5 1/2] bpf: support " Di Zhu
2021-11-05 1:57 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
2021-11-05 4:24 ` Yonghong Song
2021-11-04 6:09 zhudi (E)
2021-11-04 1:07 [PATCH bpf-next v5 1/2] bpf: support " Di Zhu
2021-11-04 1:07 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
2021-11-04 5:51 ` Yonghong Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).