| Message ID | 20210604220235.6758-4-zeffron@riotgames.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: support input xdp_md context in BPF_PROG_TEST_RUN | expand |
On 6/4/21 3:02 PM, Zvi Effron wrote: > Add a test for using xdp_md as a context to BPF_PROG_TEST_RUN for XDP > programs. > > The test uses a BPF program that takes in a return value from XDP > metadata, then reduces the size of the XDP metadata by 4 bytes. > > Test cases validate the possible failure cases for passing in invalid > xdp_md contexts, that the return value is successfully passed > in, and that the adjusted metadata is successfully copied out. > > Co-developed-by: Cody Haas <chaas@riotgames.com> > Signed-off-by: Cody Haas <chaas@riotgames.com> > Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com> > Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com> > Signed-off-by: Zvi Effron <zeffron@riotgames.com> > --- > .../bpf/prog_tests/xdp_context_test_run.c | 114 ++++++++++++++++++ > .../bpf/progs/test_xdp_context_test_run.c | 20 +++ > 2 files changed, 134 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c > create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c > new file mode 100644 > index 000000000000..0dbdebbc66ce > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c > @@ -0,0 +1,114 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <test_progs.h> > +#include <network_helpers.h> > +#include "test_xdp_context_test_run.skel.h" > + > +void test_xdp_context_test_run(void) > +{ > + struct test_xdp_context_test_run *skel = NULL; > + char data[sizeof(pkt_v4) + sizeof(__u32)]; > + char buf[128]; > + char bad_ctx[sizeof(struct xdp_md)]; > + struct xdp_md ctx_in, ctx_out; > + DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts, > + .data_in = &data, > + .data_out = buf, > + .data_size_in = sizeof(data), > + .data_size_out = sizeof(buf), > + .ctx_out = &ctx_out, > + .ctx_size_out = sizeof(ctx_out), > + .repeat = 1, > + ); > + int err, prog_fd; > + > + skel = test_xdp_context_test_run__open_and_load(); > + if (!ASSERT_OK_PTR(skel, "skel")) > + return; > + prog_fd = bpf_program__fd(skel->progs._xdp_context); > + > + *(__u32 *)data = XDP_PASS; > + *(struct ipv4_packet *)(data + sizeof(__u32)) = pkt_v4; > + > + memset(&ctx_in, 0, sizeof(ctx_in)); > + opts.ctx_in = &ctx_in; > + opts.ctx_size_in = sizeof(ctx_in); > + > + opts.ctx_in = &ctx_in; > + opts.ctx_size_in = sizeof(ctx_in); The above two assignments are redundant. > + ctx_in.data_meta = 0; > + ctx_in.data = sizeof(__u32); > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_OK(err, "bpf_prog_test_run(test1)"); > + ASSERT_EQ(opts.retval, XDP_PASS, "test1-retval"); > + ASSERT_EQ(opts.data_size_out, sizeof(pkt_v4), "test1-datasize"); > + ASSERT_EQ(opts.ctx_size_out, opts.ctx_size_in, "test1-ctxsize"); > + ASSERT_EQ(ctx_out.data_meta, 0, "test1-datameta"); > + ASSERT_EQ(ctx_out.data, ctx_out.data_meta, "test1-data"); I suggest just to test ctx_out.data == 0. It just happens the input data - meta = 4 and bpf program adjuested by 4. If they are not the same, the result won't be equal to data_meta. > + ASSERT_EQ(ctx_out.data_end, sizeof(pkt_v4), "test1-dataend"); > + > + /* Data past the end of the kernel's struct xdp_md must be 0 */ > + bad_ctx[sizeof(bad_ctx) - 1] = 1; > + opts.ctx_in = bad_ctx; > + opts.ctx_size_in = sizeof(bad_ctx); > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test2-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test2)"); I suggest to drop this test. Basically you did here is to have non-zero egress_ifindex which is not allowed. You have a test below. > + > + /* The egress cannot be specified */ > + ctx_in.egress_ifindex = 1; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test3-errno"); Use EINVAL explicitly? The same for below a few other cases. > + ASSERT_ERR(err, "bpf_prog_test_run(test3)"); > + > + /* data_meta must reference the start of data */ > + ctx_in.data_meta = sizeof(__u32); > + ctx_in.data = ctx_in.data_meta; > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > + ctx_in.egress_ifindex = 0; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test4-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test4)"); > + > + /* Metadata must be 32 bytes or smaller */ > + ctx_in.data_meta = 0; > + ctx_in.data = sizeof(__u32)*9; > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test5-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test5)"); This test is not necessary if ctx size should be <= sizeof(struct xdp_md). So far, I think we can require it must be sizeof(struct xdp_md). If in the future, kernel struct xdp_md is extended, it may be changed to accept both old and new xdp_md's similar to other uapi data strcture like struct bpf_prog_info if there is a desire. In my opinion, the kernel should just stick to sizeof(struct xdp_md) size since the functionality is implemented as a *testing* mechanism. > + > + /* Metadata's size must be a multiple of 4 */ > + ctx_in.data = 3; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test6-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test6)"); > + > + /* Total size of data must match data_end - data_meta */ > + ctx_in.data = 0; > + ctx_in.data_end = sizeof(pkt_v4) - 4; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test7-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test7)"); > + > + ctx_in.data_end = sizeof(pkt_v4) + 4; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test8-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test8)"); > + > + /* RX queue cannot be specified without specifying an ingress */ > + ctx_in.data_end = sizeof(pkt_v4); > + ctx_in.ingress_ifindex = 0; > + ctx_in.rx_queue_index = 1; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test9-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test9)"); > + > + ctx_in.ingress_ifindex = 1; > + ctx_in.rx_queue_index = 1; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test10-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test10)"); Why this failure? I guess it is due to device search failure, right? So this test MAY succeed if the underlying host happens with a proper configuration with ingress_ifindex = 1 and rx_queue_index = 1, right? > + > + test_xdp_context_test_run__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c > new file mode 100644 > index 000000000000..56fd0995b67c > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c > @@ -0,0 +1,20 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > + > +SEC("xdp") > +int _xdp_context(struct xdp_md *xdp) Maybe drop prefix "_" from the function name? > +{ > + void *data = (void *)(unsigned long)xdp->data; > + __u32 *metadata = (void *)(unsigned long)xdp->data_meta; The above code is okay as verifier will rewrite correctly with actual address. But I still suggest to use "long" instead of "unsigned long" to be consistent with other bpf programs. > + __u32 ret; > + > + if (metadata + 1 > data) > + return XDP_ABORTED; > + ret = *metadata; > + if (bpf_xdp_adjust_meta(xdp, 4)) > + return XDP_ABORTED; > + return ret; > +} > + > +char _license[] SEC("license") = "GPL"; >
On 6/4/21 3:02 PM, Zvi Effron wrote: > Add a test for using xdp_md as a context to BPF_PROG_TEST_RUN for XDP > programs. > > The test uses a BPF program that takes in a return value from XDP > metadata, then reduces the size of the XDP metadata by 4 bytes. > > Test cases validate the possible failure cases for passing in invalid > xdp_md contexts, that the return value is successfully passed > in, and that the adjusted metadata is successfully copied out. > > Co-developed-by: Cody Haas <chaas@riotgames.com> > Signed-off-by: Cody Haas <chaas@riotgames.com> > Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com> > Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com> > Signed-off-by: Zvi Effron <zeffron@riotgames.com> > --- > .../bpf/prog_tests/xdp_context_test_run.c | 114 ++++++++++++++++++ > .../bpf/progs/test_xdp_context_test_run.c | 20 +++ > 2 files changed, 134 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c > create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c > new file mode 100644 > index 000000000000..0dbdebbc66ce > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c > @@ -0,0 +1,114 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <test_progs.h> > +#include <network_helpers.h> > +#include "test_xdp_context_test_run.skel.h" > + > +void test_xdp_context_test_run(void) > +{ > + struct test_xdp_context_test_run *skel = NULL; > + char data[sizeof(pkt_v4) + sizeof(__u32)]; > + char buf[128]; > + char bad_ctx[sizeof(struct xdp_md)]; > + struct xdp_md ctx_in, ctx_out; > + DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts, > + .data_in = &data, > + .data_out = buf, > + .data_size_in = sizeof(data), > + .data_size_out = sizeof(buf), > + .ctx_out = &ctx_out, > + .ctx_size_out = sizeof(ctx_out), > + .repeat = 1, > + ); > + int err, prog_fd; > + > + skel = test_xdp_context_test_run__open_and_load(); > + if (!ASSERT_OK_PTR(skel, "skel")) > + return; > + prog_fd = bpf_program__fd(skel->progs._xdp_context); > + > + *(__u32 *)data = XDP_PASS; > + *(struct ipv4_packet *)(data + sizeof(__u32)) = pkt_v4; > + > + memset(&ctx_in, 0, sizeof(ctx_in)); > + opts.ctx_in = &ctx_in; > + opts.ctx_size_in = sizeof(ctx_in); > + > + opts.ctx_in = &ctx_in; > + opts.ctx_size_in = sizeof(ctx_in); > + ctx_in.data_meta = 0; > + ctx_in.data = sizeof(__u32); > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_OK(err, "bpf_prog_test_run(test1)"); > + ASSERT_EQ(opts.retval, XDP_PASS, "test1-retval"); > + ASSERT_EQ(opts.data_size_out, sizeof(pkt_v4), "test1-datasize"); > + ASSERT_EQ(opts.ctx_size_out, opts.ctx_size_in, "test1-ctxsize"); > + ASSERT_EQ(ctx_out.data_meta, 0, "test1-datameta"); > + ASSERT_EQ(ctx_out.data, ctx_out.data_meta, "test1-data"); > + ASSERT_EQ(ctx_out.data_end, sizeof(pkt_v4), "test1-dataend"); > + > + /* Data past the end of the kernel's struct xdp_md must be 0 */ > + bad_ctx[sizeof(bad_ctx) - 1] = 1; > + opts.ctx_in = bad_ctx; > + opts.ctx_size_in = sizeof(bad_ctx); > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test2-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test2)"); > + > + /* The egress cannot be specified */ > + ctx_in.egress_ifindex = 1; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test3-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test3)"); > + > + /* data_meta must reference the start of data */ > + ctx_in.data_meta = sizeof(__u32); > + ctx_in.data = ctx_in.data_meta; > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > + ctx_in.egress_ifindex = 0; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test4-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test4)"); > + > + /* Metadata must be 32 bytes or smaller */ > + ctx_in.data_meta = 0; > + ctx_in.data = sizeof(__u32)*9; > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test5-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test5)"); > + > + /* Metadata's size must be a multiple of 4 */ > + ctx_in.data = 3; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test6-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test6)"); > + > + /* Total size of data must match data_end - data_meta */ > + ctx_in.data = 0; > + ctx_in.data_end = sizeof(pkt_v4) - 4; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test7-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test7)"); > + > + ctx_in.data_end = sizeof(pkt_v4) + 4; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test8-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test8)"); > + > + /* RX queue cannot be specified without specifying an ingress */ > + ctx_in.data_end = sizeof(pkt_v4); > + ctx_in.ingress_ifindex = 0; > + ctx_in.rx_queue_index = 1; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test9-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test9)"); Also, these failure tests are very similar. I suggest to have a static function to do the common work. This should simplify the code and also it will be clear for each test what are ctx_in parameters. In current form, one may have to search previous test to get some parameter value. For example, for the above, one has to search previous tests to find ctx_in.data = 0 and ctx_in.data_meta = 0. > + > + ctx_in.ingress_ifindex = 1; > + ctx_in.rx_queue_index = 1; > + err = bpf_prog_test_run_opts(prog_fd, &opts); > + ASSERT_EQ(errno, 22, "test10-errno"); > + ASSERT_ERR(err, "bpf_prog_test_run(test10)"); > + > + test_xdp_context_test_run__destroy(skel); > +} [...]
On Sat, Jun 5, 2021 at 11:19 PM Yonghong Song <yhs@fb.com> wrote: > On 6/4/21 3:02 PM, Zvi Effron wrote: > > + opts.ctx_in = &ctx_in; > > + opts.ctx_size_in = sizeof(ctx_in); > > + > > + opts.ctx_in = &ctx_in; > > + opts.ctx_size_in = sizeof(ctx_in); > > The above two assignments are redundant. > Good catch. > > + ctx_in.data_meta = 0; > > + ctx_in.data = sizeof(__u32); > > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > > + err = bpf_prog_test_run_opts(prog_fd, &opts); > > + ASSERT_OK(err, "bpf_prog_test_run(test1)"); > > + ASSERT_EQ(opts.retval, XDP_PASS, "test1-retval"); > > + ASSERT_EQ(opts.data_size_out, sizeof(pkt_v4), "test1-datasize"); > > + ASSERT_EQ(opts.ctx_size_out, opts.ctx_size_in, "test1-ctxsize"); > > + ASSERT_EQ(ctx_out.data_meta, 0, "test1-datameta"); > > + ASSERT_EQ(ctx_out.data, ctx_out.data_meta, "test1-data"); > > I suggest just to test ctx_out.data == 0. It just happens > the input data - meta = 4 and bpf program adjuested by 4. > If they are not the same, the result won't be equal to data_meta. > Sure. > > + ASSERT_EQ(ctx_out.data_end, sizeof(pkt_v4), "test1-dataend"); > > + > > + /* Data past the end of the kernel's struct xdp_md must be 0 */ > > + bad_ctx[sizeof(bad_ctx) - 1] = 1; > > + opts.ctx_in = bad_ctx; > > + opts.ctx_size_in = sizeof(bad_ctx); > > + err = bpf_prog_test_run_opts(prog_fd, &opts); > > + ASSERT_EQ(errno, 22, "test2-errno"); > > + ASSERT_ERR(err, "bpf_prog_test_run(test2)"); > > I suggest to drop this test. Basically you did here > is to have non-zero egress_ifindex which is not allowed. > You have a test below. > We think the actual correction here is that bad_ctx is supposed to be one byte larger than than struct xdp_md. It is misdeclared. We'll correct that. > > + > > + /* The egress cannot be specified */ > > + ctx_in.egress_ifindex = 1; > > + err = bpf_prog_test_run_opts(prog_fd, &opts); > > + ASSERT_EQ(errno, 22, "test3-errno"); > > Use EINVAL explicitly? The same for below a few other cases. > Good suggestion. > > + ASSERT_ERR(err, "bpf_prog_test_run(test3)"); > > + > > + /* data_meta must reference the start of data */ > > + ctx_in.data_meta = sizeof(__u32); > > + ctx_in.data = ctx_in.data_meta; > > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > > + ctx_in.egress_ifindex = 0; > > + err = bpf_prog_test_run_opts(prog_fd, &opts); > > + ASSERT_EQ(errno, 22, "test4-errno"); > > + ASSERT_ERR(err, "bpf_prog_test_run(test4)"); > > + > > + /* Metadata must be 32 bytes or smaller */ > > + ctx_in.data_meta = 0; > > + ctx_in.data = sizeof(__u32)*9; > > + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); > > + err = bpf_prog_test_run_opts(prog_fd, &opts); > > + ASSERT_EQ(errno, 22, "test5-errno"); > > + ASSERT_ERR(err, "bpf_prog_test_run(test5)"); > > This test is not necessary if ctx size should be > <= sizeof(struct xdp_md). So far, I think we can > require it must be sizeof(struct xdp_md). If > in the future, kernel struct xdp_md is extended, > it may be changed to accept both old and new > xdp_md's similar to other uapi data strcture > like struct bpf_prog_info if there is a desire. > In my opinion, the kernel should just stick > to sizeof(struct xdp_md) size since the functionality > is implemented as a *testing* mechanism. > You might be confusing the context (struct xdp_md) with the XDP metadata (data just before the frame data). XDP allows at most 32 bytes of metadata. This test is verifying that a metadata size >32 bytes is rejected. > > + ctx_in.ingress_ifindex = 1; > > + ctx_in.rx_queue_index = 1; > > + err = bpf_prog_test_run_opts(prog_fd, &opts); > > + ASSERT_EQ(errno, 22, "test10-errno"); > > + ASSERT_ERR(err, "bpf_prog_test_run(test10)"); > > Why this failure? I guess it is due to device search failure, right? > So this test MAY succeed if the underlying host happens with > a proper configuration with ingress_ifindex = 1 and rx_queue_index = 1, > right? > I may be making incorrect assumptions, but my understanding is that interface index 1 is always the loopback interface, and the loopback interface only ever (in current kernels) has one rx queue. If that's not the case, we'll need to adjust (or remove) the test. > > + > > + test_xdp_context_test_run__destroy(skel); > > +} > > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c > > new file mode 100644 > > index 000000000000..56fd0995b67c > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c > > @@ -0,0 +1,20 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include <linux/bpf.h> > > +#include <bpf/bpf_helpers.h> > > + > > +SEC("xdp") > > +int _xdp_context(struct xdp_md *xdp) > > Maybe drop prefix "_" from the function name? > Sure. > > +{ > > + void *data = (void *)(unsigned long)xdp->data; > > + __u32 *metadata = (void *)(unsigned long)xdp->data_meta; > > The above code is okay as verifier will rewrite correctly with actual > address. But I still suggest to use "long" instead of "unsigned long" > to be consistent with other bpf programs. > Sure.
On 6/9/21 10:07 AM, Zvi Effron wrote: > On Sat, Jun 5, 2021 at 11:19 PM Yonghong Song <yhs@fb.com> wrote: >> On 6/4/21 3:02 PM, Zvi Effron wrote: >>> + opts.ctx_in = &ctx_in; >>> + opts.ctx_size_in = sizeof(ctx_in); >>> + >>> + opts.ctx_in = &ctx_in; >>> + opts.ctx_size_in = sizeof(ctx_in); >> >> The above two assignments are redundant. >> > > Good catch. > >>> + ctx_in.data_meta = 0; >>> + ctx_in.data = sizeof(__u32); >>> + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); >>> + err = bpf_prog_test_run_opts(prog_fd, &opts); >>> + ASSERT_OK(err, "bpf_prog_test_run(test1)"); >>> + ASSERT_EQ(opts.retval, XDP_PASS, "test1-retval"); >>> + ASSERT_EQ(opts.data_size_out, sizeof(pkt_v4), "test1-datasize"); >>> + ASSERT_EQ(opts.ctx_size_out, opts.ctx_size_in, "test1-ctxsize"); >>> + ASSERT_EQ(ctx_out.data_meta, 0, "test1-datameta"); >>> + ASSERT_EQ(ctx_out.data, ctx_out.data_meta, "test1-data"); >> >> I suggest just to test ctx_out.data == 0. It just happens >> the input data - meta = 4 and bpf program adjuested by 4. >> If they are not the same, the result won't be equal to data_meta. >> > > Sure. > >>> + ASSERT_EQ(ctx_out.data_end, sizeof(pkt_v4), "test1-dataend"); >>> + >>> + /* Data past the end of the kernel's struct xdp_md must be 0 */ >>> + bad_ctx[sizeof(bad_ctx) - 1] = 1; >>> + opts.ctx_in = bad_ctx; >>> + opts.ctx_size_in = sizeof(bad_ctx); >>> + err = bpf_prog_test_run_opts(prog_fd, &opts); >>> + ASSERT_EQ(errno, 22, "test2-errno"); >>> + ASSERT_ERR(err, "bpf_prog_test_run(test2)"); >> >> I suggest to drop this test. Basically you did here >> is to have non-zero egress_ifindex which is not allowed. >> You have a test below. >> > > We think the actual correction here is that bad_ctx is supposed to be one byte > larger than than struct xdp_md. It is misdeclared. We'll correct that. > >>> + >>> + /* The egress cannot be specified */ >>> + ctx_in.egress_ifindex = 1; >>> + err = bpf_prog_test_run_opts(prog_fd, &opts); >>> + ASSERT_EQ(errno, 22, "test3-errno"); >> >> Use EINVAL explicitly? The same for below a few other cases. >> > > Good suggestion. > >>> + ASSERT_ERR(err, "bpf_prog_test_run(test3)"); >>> + >>> + /* data_meta must reference the start of data */ >>> + ctx_in.data_meta = sizeof(__u32); >>> + ctx_in.data = ctx_in.data_meta; >>> + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); >>> + ctx_in.egress_ifindex = 0; >>> + err = bpf_prog_test_run_opts(prog_fd, &opts); >>> + ASSERT_EQ(errno, 22, "test4-errno"); >>> + ASSERT_ERR(err, "bpf_prog_test_run(test4)"); >>> + >>> + /* Metadata must be 32 bytes or smaller */ >>> + ctx_in.data_meta = 0; >>> + ctx_in.data = sizeof(__u32)*9; >>> + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); >>> + err = bpf_prog_test_run_opts(prog_fd, &opts); >>> + ASSERT_EQ(errno, 22, "test5-errno"); >>> + ASSERT_ERR(err, "bpf_prog_test_run(test5)"); >> >> This test is not necessary if ctx size should be >> <= sizeof(struct xdp_md). So far, I think we can >> require it must be sizeof(struct xdp_md). If >> in the future, kernel struct xdp_md is extended, >> it may be changed to accept both old and new >> xdp_md's similar to other uapi data strcture >> like struct bpf_prog_info if there is a desire. >> In my opinion, the kernel should just stick >> to sizeof(struct xdp_md) size since the functionality >> is implemented as a *testing* mechanism. >> > > You might be confusing the context (struct xdp_md) with the XDP metadata (data > just before the frame data). XDP allows at most 32 bytes of metadata. This test > is verifying that a metadata size >32 bytes is rejected. Right, you can keep this test. Previously I suggested to enforce sizeof(struct xdp_md) as the ctx size, but that may be too restrictive. > >>> + ctx_in.ingress_ifindex = 1; >>> + ctx_in.rx_queue_index = 1; >>> + err = bpf_prog_test_run_opts(prog_fd, &opts); >>> + ASSERT_EQ(errno, 22, "test10-errno"); >>> + ASSERT_ERR(err, "bpf_prog_test_run(test10)"); >> >> Why this failure? I guess it is due to device search failure, right? >> So this test MAY succeed if the underlying host happens with >> a proper configuration with ingress_ifindex = 1 and rx_queue_index = 1, >> right? >> > > I may be making incorrect assumptions, but my understanding is that interface > index 1 is always the loopback interface, and the loopback interface only ever > (in current kernels) has one rx queue. If that's not the case, we'll need to > adjust (or remove) the test. You could be correct. Please add some comments though. > >>> + >>> + test_xdp_context_test_run__destroy(skel); >>> +} [...]
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c new file mode 100644 index 000000000000..0dbdebbc66ce --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> +#include <network_helpers.h> +#include "test_xdp_context_test_run.skel.h" + +void test_xdp_context_test_run(void) +{ + struct test_xdp_context_test_run *skel = NULL; + char data[sizeof(pkt_v4) + sizeof(__u32)]; + char buf[128]; + char bad_ctx[sizeof(struct xdp_md)]; + struct xdp_md ctx_in, ctx_out; + DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts, + .data_in = &data, + .data_out = buf, + .data_size_in = sizeof(data), + .data_size_out = sizeof(buf), + .ctx_out = &ctx_out, + .ctx_size_out = sizeof(ctx_out), + .repeat = 1, + ); + int err, prog_fd; + + skel = test_xdp_context_test_run__open_and_load(); + if (!ASSERT_OK_PTR(skel, "skel")) + return; + prog_fd = bpf_program__fd(skel->progs._xdp_context); + + *(__u32 *)data = XDP_PASS; + *(struct ipv4_packet *)(data + sizeof(__u32)) = pkt_v4; + + memset(&ctx_in, 0, sizeof(ctx_in)); + opts.ctx_in = &ctx_in; + opts.ctx_size_in = sizeof(ctx_in); + + opts.ctx_in = &ctx_in; + opts.ctx_size_in = sizeof(ctx_in); + ctx_in.data_meta = 0; + ctx_in.data = sizeof(__u32); + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_OK(err, "bpf_prog_test_run(test1)"); + ASSERT_EQ(opts.retval, XDP_PASS, "test1-retval"); + ASSERT_EQ(opts.data_size_out, sizeof(pkt_v4), "test1-datasize"); + ASSERT_EQ(opts.ctx_size_out, opts.ctx_size_in, "test1-ctxsize"); + ASSERT_EQ(ctx_out.data_meta, 0, "test1-datameta"); + ASSERT_EQ(ctx_out.data, ctx_out.data_meta, "test1-data"); + ASSERT_EQ(ctx_out.data_end, sizeof(pkt_v4), "test1-dataend"); + + /* Data past the end of the kernel's struct xdp_md must be 0 */ + bad_ctx[sizeof(bad_ctx) - 1] = 1; + opts.ctx_in = bad_ctx; + opts.ctx_size_in = sizeof(bad_ctx); + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test2-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test2)"); + + /* The egress cannot be specified */ + ctx_in.egress_ifindex = 1; + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test3-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test3)"); + + /* data_meta must reference the start of data */ + ctx_in.data_meta = sizeof(__u32); + ctx_in.data = ctx_in.data_meta; + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); + ctx_in.egress_ifindex = 0; + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test4-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test4)"); + + /* Metadata must be 32 bytes or smaller */ + ctx_in.data_meta = 0; + ctx_in.data = sizeof(__u32)*9; + ctx_in.data_end = ctx_in.data + sizeof(pkt_v4); + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test5-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test5)"); + + /* Metadata's size must be a multiple of 4 */ + ctx_in.data = 3; + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test6-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test6)"); + + /* Total size of data must match data_end - data_meta */ + ctx_in.data = 0; + ctx_in.data_end = sizeof(pkt_v4) - 4; + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test7-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test7)"); + + ctx_in.data_end = sizeof(pkt_v4) + 4; + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test8-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test8)"); + + /* RX queue cannot be specified without specifying an ingress */ + ctx_in.data_end = sizeof(pkt_v4); + ctx_in.ingress_ifindex = 0; + ctx_in.rx_queue_index = 1; + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test9-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test9)"); + + ctx_in.ingress_ifindex = 1; + ctx_in.rx_queue_index = 1; + err = bpf_prog_test_run_opts(prog_fd, &opts); + ASSERT_EQ(errno, 22, "test10-errno"); + ASSERT_ERR(err, "bpf_prog_test_run(test10)"); + + test_xdp_context_test_run__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c new file mode 100644 index 000000000000..56fd0995b67c --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_xdp_context_test_run.c @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> + +SEC("xdp") +int _xdp_context(struct xdp_md *xdp) +{ + void *data = (void *)(unsigned long)xdp->data; + __u32 *metadata = (void *)(unsigned long)xdp->data_meta; + __u32 ret; + + if (metadata + 1 > data) + return XDP_ABORTED; + ret = *metadata; + if (bpf_xdp_adjust_meta(xdp, 4)) + return XDP_ABORTED; + return ret; +} + +char _license[] SEC("license") = "GPL";