| Message ID | 158945349549.97035.15316291762482444006.stgit@firesoul |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | XDP extend with knowledge of frame size | expand |
On Thu, May 14, 2020 at 3:51 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > Update the memory requirements, when adding xdp.frame_sz in BPF test_run > function bpf_prog_test_run_xdp() which e.g. is used by XDP selftests. > > Specifically add the expected reserved tailroom, but also allocated a > larger memory area to reflect that XDP frames usually comes in this > format. Limit the provided packet data size to 4096 minus headroom + > tailroom, as this also reflect a common 3520 bytes MTU limit with XDP. > > Note that bpf_test_init already use a memory allocation method that clears > memory. Thus, this already guards against leaking uninit kernel memory. > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > net/bpf/test_run.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index 29dbdd4c29f6..30ba7d38941d 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -470,25 +470,34 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, > int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, > union bpf_attr __user *uattr) > { > + u32 tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > + u32 headroom = XDP_PACKET_HEADROOM; > u32 size = kattr->test.data_size_in; > u32 repeat = kattr->test.repeat; > struct netdev_rx_queue *rxqueue; > struct xdp_buff xdp = {}; > u32 retval, duration; > + u32 max_data_sz; > void *data; > int ret; > > if (kattr->test.ctx_in || kattr->test.ctx_out) > return -EINVAL; > > - data = bpf_test_init(kattr, size, XDP_PACKET_HEADROOM + NET_IP_ALIGN, 0); > + /* XDP have extra tailroom as (most) drivers use full page */ > + max_data_sz = 4096 - headroom - tailroom; > + if (size > max_data_sz) > + return -EINVAL; > + > + data = bpf_test_init(kattr, max_data_sz, headroom, tailroom); Hi Jesper, above is buggy. max_data_sz is way more than what user space has. That causes xdp unit tests to fail sporadically with EFAULT. Like: ./test_progs -t xdp_perf test_xdp_perf:FAIL:xdp-perf err -1 errno 14 retval 0 size 0 #89 xdp_perf:FAIL For that test max_data_sz will be 3520 while user space prepared only 128 bytes and copy_from_user will fault.
On Sat, 16 May 2020 21:02:01 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Thu, May 14, 2020 at 3:51 AM Jesper Dangaard Brouer > <brouer@redhat.com> wrote: > > > > Update the memory requirements, when adding xdp.frame_sz in BPF test_run > > function bpf_prog_test_run_xdp() which e.g. is used by XDP selftests. > > > > Specifically add the expected reserved tailroom, but also allocated a > > larger memory area to reflect that XDP frames usually comes in this > > format. Limit the provided packet data size to 4096 minus headroom + > > tailroom, as this also reflect a common 3520 bytes MTU limit with XDP. > > > > Note that bpf_test_init already use a memory allocation method that clears > > memory. Thus, this already guards against leaking uninit kernel memory. > > > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > --- > > net/bpf/test_run.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > > index 29dbdd4c29f6..30ba7d38941d 100644 > > --- a/net/bpf/test_run.c > > +++ b/net/bpf/test_run.c > > @@ -470,25 +470,34 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, > > int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, > > union bpf_attr __user *uattr) > > { > > + u32 tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > > + u32 headroom = XDP_PACKET_HEADROOM; > > u32 size = kattr->test.data_size_in; > > u32 repeat = kattr->test.repeat; > > struct netdev_rx_queue *rxqueue; > > struct xdp_buff xdp = {}; > > u32 retval, duration; > > + u32 max_data_sz; > > void *data; > > int ret; > > > > if (kattr->test.ctx_in || kattr->test.ctx_out) > > return -EINVAL; > > > > - data = bpf_test_init(kattr, size, XDP_PACKET_HEADROOM + NET_IP_ALIGN, 0); > > + /* XDP have extra tailroom as (most) drivers use full page */ > > + max_data_sz = 4096 - headroom - tailroom; > > + if (size > max_data_sz) > > + return -EINVAL; > > + > > + data = bpf_test_init(kattr, max_data_sz, headroom, tailroom); > > Hi Jesper, > > above is buggy. > max_data_sz is way more than what user space has. > That causes xdp unit tests to fail sporadically with EFAULT. > Like: > ./test_progs -t xdp_perf > test_xdp_perf:FAIL:xdp-perf err -1 errno 14 retval 0 size 0 > #89 xdp_perf:FAIL > > For that test max_data_sz will be 3520 while user space prepared only 128 bytes > and copy_from_user will fault. I'm looking into this... BUT ... I'm getting unrelated compile errors for selftests/bpf in bpf-next tree (HEAD 96586dd9268d2). The compile error, see below signature, happens in ./progs/bpf_iter_ipv6_route.c (tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c). Related to commit: 7c128a6bbd4f ("tools/bpf: selftests: Add iterator programs for ipv6_route and netlink") (Author: Yonghong Song) - Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer $ make CLNG-LLC [test_maps] bpf_iter_ipv6_route.o progs/bpf_iter_ipv6_route.c:18:28: warning: declaration of 'struct bpf_iter__ipv6_route' will not be visible outside of this function [-Wvisibility] int dump_ipv6_route(struct bpf_iter__ipv6_route *ctx) ^ progs/bpf_iter_ipv6_route.c:20:28: error: incomplete definition of type 'struct bpf_iter__ipv6_route' struct seq_file *seq = ctx->meta->seq; ~~~^ progs/bpf_iter_ipv6_route.c:18:28: note: forward declaration of 'struct bpf_iter__ipv6_route' int dump_ipv6_route(struct bpf_iter__ipv6_route *ctx) ^ progs/bpf_iter_ipv6_route.c:21:28: error: incomplete definition of type 'struct bpf_iter__ipv6_route' struct fib6_info *rt = ctx->rt; ~~~^ progs/bpf_iter_ipv6_route.c:18:28: note: forward declaration of 'struct bpf_iter__ipv6_route' int dump_ipv6_route(struct bpf_iter__ipv6_route *ctx) ^ 1 warning and 2 errors generated. llc: error: llc: <stdin>:1:1: error: expected top-level entity BPF obj compilation failed ^ make: *** [Makefile:365: /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/bpf_iter_ipv6_route.o] Error 1
On Mon, 18 May 2020 11:52:34 +0200 Jesper Dangaard Brouer <brouer@redhat.com> wrote: > ... I'm getting unrelated compile errors for selftests/bpf in > bpf-next tree (HEAD 96586dd9268d2). > > The compile error, see below signature, happens in ./progs/bpf_iter_ipv6_route.c > (tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c). > > Related to commit: > 7c128a6bbd4f ("tools/bpf: selftests: Add iterator programs for ipv6_route and netlink") (Author: Yonghong Song) After re-compiling the kernel in the same tree, this issue goes away. I guess this is related to: #include "vmlinux.h"
On Mon, May 18, 2020 at 3:33 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > On Mon, 18 May 2020 11:52:34 +0200 > Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > > ... I'm getting unrelated compile errors for selftests/bpf in > > bpf-next tree (HEAD 96586dd9268d2). > > > > The compile error, see below signature, happens in ./progs/bpf_iter_ipv6_route.c > > (tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c). > > > > Related to commit: > > 7c128a6bbd4f ("tools/bpf: selftests: Add iterator programs for ipv6_route and netlink") (Author: Yonghong Song) > > After re-compiling the kernel in the same tree, this issue goes away. > > I guess this is related to: > #include "vmlinux.h" > Yes, I ran into the same issue with libbpf CI tests for older kernel. vmlinux.h for it simply doesn't contain definitions of bpf_iter__xxx types. I think for selftests, it's better to have local copies of those struct definitions, to make it compilable against older vmlinux.h. > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer >
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 29dbdd4c29f6..30ba7d38941d 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -470,25 +470,34 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, union bpf_attr __user *uattr) { + u32 tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); + u32 headroom = XDP_PACKET_HEADROOM; u32 size = kattr->test.data_size_in; u32 repeat = kattr->test.repeat; struct netdev_rx_queue *rxqueue; struct xdp_buff xdp = {}; u32 retval, duration; + u32 max_data_sz; void *data; int ret; if (kattr->test.ctx_in || kattr->test.ctx_out) return -EINVAL; - data = bpf_test_init(kattr, size, XDP_PACKET_HEADROOM + NET_IP_ALIGN, 0); + /* XDP have extra tailroom as (most) drivers use full page */ + max_data_sz = 4096 - headroom - tailroom; + if (size > max_data_sz) + return -EINVAL; + + data = bpf_test_init(kattr, max_data_sz, headroom, tailroom); if (IS_ERR(data)) return PTR_ERR(data); xdp.data_hard_start = data; - xdp.data = data + XDP_PACKET_HEADROOM + NET_IP_ALIGN; + xdp.data = data + headroom; xdp.data_meta = xdp.data; xdp.data_end = xdp.data + size; + xdp.frame_sz = headroom + max_data_sz + tailroom; rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0); xdp.rxq = &rxqueue->xdp_rxq; @@ -496,8 +505,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration, true); if (ret) goto out; - if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN || - xdp.data_end != xdp.data + size) + if (xdp.data != data + headroom || xdp.data_end != xdp.data + size) size = xdp.data_end - xdp.data; ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration); out:
Update the memory requirements, when adding xdp.frame_sz in BPF test_run function bpf_prog_test_run_xdp() which e.g. is used by XDP selftests. Specifically add the expected reserved tailroom, but also allocated a larger memory area to reflect that XDP frames usually comes in this format. Limit the provided packet data size to 4096 minus headroom + tailroom, as this also reflect a common 3520 bytes MTU limit with XDP. Note that bpf_test_init already use a memory allocation method that clears memory. Thus, this already guards against leaking uninit kernel memory. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- net/bpf/test_run.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)