| Message ID | 20200528001423.58575-6-dsahern@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add support for XDP programs in DEVMAP entries | expand |
On Wed, May 27, 2020 at 5:15 PM David Ahern <dsahern@kernel.org> wrote: > > Add tests to verify ability to add an XDP program to a > entry in a DEVMAP. > > Add negative tests to show DEVMAP programs can not be > attached to devices as a normal XDP program, and accesses > to egress_ifindex require BPF_XDP_DEVMAP attach type. > > Signed-off-by: David Ahern <dsahern@kernel.org> > --- > .../bpf/prog_tests/xdp_devmap_attach.c | 94 +++++++++++++++++++ > .../selftests/bpf/progs/test_xdp_devmap.c | 19 ++++ > .../selftests/bpf/progs/test_xdp_devmap2.c | 19 ++++ > .../bpf/progs/test_xdp_with_devmap.c | 17 ++++ > 4 files changed, 149 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c > create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap.c > create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap2.c > create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c > new file mode 100644 > index 000000000000..d81b2b366f39 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c > @@ -0,0 +1,94 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <uapi/linux/bpf.h> > +#include <linux/if_link.h> > +#include <test_progs.h> > + > +#define IFINDEX_LO 1 > + > +void test_xdp_devmap_attach(void) > +{ > + struct bpf_prog_load_attr attr = { > + .prog_type = BPF_PROG_TYPE_XDP, > + }; > + struct bpf_object *obj, *dm_obj = NULL; > + int err, dm_fd = -1, fd = -1, map_fd; > + struct bpf_prog_info info = {}; > + struct devmap_val val = { > + .ifindex = IFINDEX_LO, > + }; > + __u32 id, len = sizeof(info); > + __u32 duration = 0, idx = 0; > + > + attr.file = "./test_xdp_with_devmap.o", > + err = bpf_prog_load_xattr(&attr, &obj, &fd); please use skeletons instead of loading .o files. > + if (CHECK(err, "load of xdp program with 8-byte devmap", > + "err %d errno %d\n", err, errno)) > + return; > + [...] > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap.c > new file mode 100644 > index 000000000000..64fc2c3cae01 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_xdp_devmap.c > @@ -0,0 +1,19 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* program inserted into devmap entry */ > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > + > +SEC("xdp_devmap_log") > +int xdpdm_devlog(struct xdp_md *ctx) > +{ > + char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n"; > + void *data_end = (void *)(long)ctx->data_end; > + void *data = (void *)(long)ctx->data; > + unsigned int len = data_end - data; > + > + bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len); > + > + return XDP_PASS; > +} > + > +char _license[] SEC("license") = "GPL"; > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c > new file mode 100644 > index 000000000000..64fc2c3cae01 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c > @@ -0,0 +1,19 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* program inserted into devmap entry */ > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > + > +SEC("xdp_devmap_log") > +int xdpdm_devlog(struct xdp_md *ctx) > +{ > + char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n"; > + void *data_end = (void *)(long)ctx->data_end; > + void *data = (void *)(long)ctx->data; > + unsigned int len = data_end - data; > + > + bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len); instead of just printing ifindexes, why not return them through global variable and validate in a test? > + > + return XDP_PASS; > +} > + > +char _license[] SEC("license") = "GPL"; > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c > new file mode 100644 > index 000000000000..815cd59b4866 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c > @@ -0,0 +1,17 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > + > +struct bpf_map_def SEC("maps") dm_ports = { > + .type = BPF_MAP_TYPE_DEVMAP, > + .key_size = sizeof(__u32), > + .value_size = sizeof(struct devmap_val), > + .max_entries = 4, > +}; > + This is an old syntax for maps, all the selftests were converted to BTF-defined maps. Please update. > +SEC("xdp_devmap") > +int xdp_with_devmap(struct xdp_md *ctx) > +{ > + return bpf_redirect_map(&dm_ports, 1, 0); > +} > -- > 2.21.1 (Apple Git-122.3) >
On Thu, 28 May 2020 00:08:34 -0700 Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > diff --git a/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c > > new file mode 100644 > > index 000000000000..815cd59b4866 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c > > @@ -0,0 +1,17 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#include <linux/bpf.h> > > +#include <bpf/bpf_helpers.h> > > + > > +struct bpf_map_def SEC("maps") dm_ports = { > > + .type = BPF_MAP_TYPE_DEVMAP, > > + .key_size = sizeof(__u32), > > + .value_size = sizeof(struct devmap_val), > > + .max_entries = 4, > > +}; > > + > > This is an old syntax for maps, all the selftests were converted to > BTF-defined maps. Please update. LOL - The map type BPF_MAP_TYPE_DEVMAP does not support BTF for key and value, which it what I've been trying to point out... (and yes, I do have code that makes it work in my tree.). That said, you should use the new SEC(".maps") definitions, but you need to use some tricks to avoid a BTF-ID getting generated. Let me help you with something that should work: /* DEVMAP values */ struct devmap_val { __u32 ifindex; /* device index */ }; struct { __uint(type, BPF_MAP_TYPE_DEVMAP); __uint(key_size, sizeof(u32)); __uint(value_size, sizeof(struct devmap_val)); __uint(max_entries, 4); } dm_ports SEC(".maps"); Notice by setting key_size and value_size, instead of the "__type", then a BTF-ID will be generated for this map. Normally with proper BTF it should look like: struct { __uint(type, BPF_MAP_TYPE_DEVMAP); __type(key, u32); __type(value, struct devmap_val); __uint(max_entries, 4); } dm_ports_with_BTF SEC(".maps");
On 5/28/20 1:08 AM, Andrii Nakryiko wrote: >> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c >> new file mode 100644 >> index 000000000000..d81b2b366f39 >> --- /dev/null >> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c >> @@ -0,0 +1,94 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +#include <uapi/linux/bpf.h> >> +#include <linux/if_link.h> >> +#include <test_progs.h> >> + >> +#define IFINDEX_LO 1 >> + >> +void test_xdp_devmap_attach(void) >> +{ >> + struct bpf_prog_load_attr attr = { >> + .prog_type = BPF_PROG_TYPE_XDP, >> + }; >> + struct bpf_object *obj, *dm_obj = NULL; >> + int err, dm_fd = -1, fd = -1, map_fd; >> + struct bpf_prog_info info = {}; >> + struct devmap_val val = { >> + .ifindex = IFINDEX_LO, >> + }; >> + __u32 id, len = sizeof(info); >> + __u32 duration = 0, idx = 0; >> + >> + attr.file = "./test_xdp_with_devmap.o", >> + err = bpf_prog_load_xattr(&attr, &obj, &fd); > > please use skeletons instead of loading .o files. I will look into it. > >> + if (CHECK(err, "load of xdp program with 8-byte devmap", >> + "err %d errno %d\n", err, errno)) >> + return; >> + > > [...] > >> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c >> new file mode 100644 >> index 000000000000..64fc2c3cae01 >> --- /dev/null >> +++ b/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c >> @@ -0,0 +1,19 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* program inserted into devmap entry */ >> +#include <linux/bpf.h> >> +#include <bpf/bpf_helpers.h> >> + >> +SEC("xdp_devmap_log") >> +int xdpdm_devlog(struct xdp_md *ctx) >> +{ >> + char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n"; >> + void *data_end = (void *)(long)ctx->data_end; >> + void *data = (void *)(long)ctx->data; >> + unsigned int len = data_end - data; >> + >> + bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len); > > instead of just printing ifindexes, why not return them through global > variable and validate in a test? The point of this program to access the egress_ifindex with the expected attached type NOT set to BPF_XDP_DEVMAP to FAIL the verifier checks.
On 5/28/20 4:25 AM, Jesper Dangaard Brouer wrote: > That said, you should use the new SEC(".maps") definitions, but you > need to use some tricks to avoid a BTF-ID getting generated. Let me > help you with something that should work: > > /* DEVMAP values */ > struct devmap_val { > __u32 ifindex; /* device index */ > }; > > struct { > __uint(type, BPF_MAP_TYPE_DEVMAP); > __uint(key_size, sizeof(u32)); > __uint(value_size, sizeof(struct devmap_val)); > __uint(max_entries, 4); > } dm_ports SEC(".maps"); > > Notice by setting key_size and value_size, instead of the "__type", > then a BTF-ID will be generated for this map. > Normally with proper BTF it should look like: > > struct { > __uint(type, BPF_MAP_TYPE_DEVMAP); > __type(key, u32); > __type(value, struct devmap_val); > __uint(max_entries, 4); > } dm_ports_with_BTF SEC(".maps"); > > Thanks for the tip.
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file mode 100644 index 000000000000..d81b2b366f39 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <uapi/linux/bpf.h> +#include <linux/if_link.h> +#include <test_progs.h> + +#define IFINDEX_LO 1 + +void test_xdp_devmap_attach(void) +{ + struct bpf_prog_load_attr attr = { + .prog_type = BPF_PROG_TYPE_XDP, + }; + struct bpf_object *obj, *dm_obj = NULL; + int err, dm_fd = -1, fd = -1, map_fd; + struct bpf_prog_info info = {}; + struct devmap_val val = { + .ifindex = IFINDEX_LO, + }; + __u32 id, len = sizeof(info); + __u32 duration = 0, idx = 0; + + attr.file = "./test_xdp_with_devmap.o", + err = bpf_prog_load_xattr(&attr, &obj, &fd); + if (CHECK(err, "load of xdp program with 8-byte devmap", + "err %d errno %d\n", err, errno)) + return; + + /* can not attach program with DEVMAPs that allow programs */ + err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE); + CHECK(err == 0, "Generic attach of program with 8-byte devmap", + "should have failed\n"); + + map_fd = bpf_object__find_map_fd_by_name(obj, "dm_ports"); + if (CHECK(map_fd < 0, "Lookup dm_ports map", + "err %d errno %d\n", err, errno)) + goto out_close; + + /* + * basic test - load program to be installed in devmap; + * also verifies access to ingress and egress ifindices. + */ + attr.expected_attach_type = BPF_XDP_DEVMAP; + attr.file = "./test_xdp_devmap.o"; + err = bpf_prog_load_xattr(&attr, &dm_obj, &dm_fd); + if (CHECK(err, "Load of BPF_XDP_DEVMAP program", + "err %d errno %d\n", err, errno)) + goto out_close; + + err = bpf_obj_get_info_by_fd(dm_fd, &info, &len); + if (CHECK_FAIL(err)) + goto out_close; + + val.bpf_prog_fd = dm_fd; + err = bpf_map_update_elem(map_fd, &idx, &val, 0); + CHECK(err, "Add program to devmap entry", + "err %d errno %d\n", err, errno); + + err = bpf_map_lookup_elem(map_fd, &id, &val); + CHECK(err, "Read devmap entry", + "err %d errno %d\n", err, errno); + CHECK(info.id != val.bpf_prog_id, "Expected program id in devmap entry", + "expected %u read %u\n", info.id, val.bpf_prog_id); + + /* can not attach BPF_XDP_DEVMAP program to a device */ + err = bpf_set_link_xdp_fd(IFINDEX_LO, dm_fd, XDP_FLAGS_SKB_MODE); + CHECK(err == 0, "Attach of BPF_XDP_DEVMAP program", + "should have failed\n"); + + bpf_object__close(dm_obj); + + /* Load of BPF_XDP_DEVMAP as XDP should fail because of egress index */ + attr.expected_attach_type = 0; + err = bpf_prog_load_xattr(&attr, &dm_obj, &dm_fd); + if (CHECK(err == 0, + "Load of XDP program accessing egress ifindex without attach type", + "should have failed\n")) + bpf_object__close(dm_obj); + + attr.file = "./xdp_dummy.o"; + err = bpf_prog_load_xattr(&attr, &dm_obj, &dm_fd); + if (CHECK_FAIL(err)) + goto out_close; + + val.ifindex = 1; + val.bpf_prog_fd = dm_fd; + err = bpf_map_update_elem(map_fd, &idx, &val, 0); + CHECK(err == 0, "Add non-BPF_XDP_DEVMAP program to devmap entry", + "should have failed\n"); + + bpf_object__close(dm_obj); + +out_close: + bpf_object__close(obj); +} diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap.c new file mode 100644 index 000000000000..64fc2c3cae01 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_xdp_devmap.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0 +/* program inserted into devmap entry */ +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> + +SEC("xdp_devmap_log") +int xdpdm_devlog(struct xdp_md *ctx) +{ + char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n"; + void *data_end = (void *)(long)ctx->data_end; + void *data = (void *)(long)ctx->data; + unsigned int len = data_end - data; + + bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len); + + return XDP_PASS; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c b/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c new file mode 100644 index 000000000000..64fc2c3cae01 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_xdp_devmap2.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0 +/* program inserted into devmap entry */ +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> + +SEC("xdp_devmap_log") +int xdpdm_devlog(struct xdp_md *ctx) +{ + char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n"; + void *data_end = (void *)(long)ctx->data_end; + void *data = (void *)(long)ctx->data; + unsigned int len = data_end - data; + + bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len); + + return XDP_PASS; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c new file mode 100644 index 000000000000..815cd59b4866 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> + +struct bpf_map_def SEC("maps") dm_ports = { + .type = BPF_MAP_TYPE_DEVMAP, + .key_size = sizeof(__u32), + .value_size = sizeof(struct devmap_val), + .max_entries = 4, +}; + +SEC("xdp_devmap") +int xdp_with_devmap(struct xdp_md *ctx) +{ + return bpf_redirect_map(&dm_ports, 1, 0); +}
Add tests to verify ability to add an XDP program to a entry in a DEVMAP. Add negative tests to show DEVMAP programs can not be attached to devices as a normal XDP program, and accesses to egress_ifindex require BPF_XDP_DEVMAP attach type. Signed-off-by: David Ahern <dsahern@kernel.org> --- .../bpf/prog_tests/xdp_devmap_attach.c | 94 +++++++++++++++++++ .../selftests/bpf/progs/test_xdp_devmap.c | 19 ++++ .../selftests/bpf/progs/test_xdp_devmap2.c | 19 ++++ .../bpf/progs/test_xdp_with_devmap.c | 17 ++++ 4 files changed, 149 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap.c create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap2.c create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c