netdev - Re: unstable xdp tests. Was: [PATCH net-next v4 31/33] bpf: add xdp.frame_sz in bpf_prog_test_run

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200518115234.3b6925de@carbon>
Date:   Mon, 18 May 2020 11:52:34 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Yonghong Song <yhs@...com>
Cc:     sameehj@...zon.com, Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        John Fastabend <john.fastabend@...il.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        David Ahern <dsahern@...il.com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Andrii Nakryiko <andriin@...com>, brouer@...hat.com
Subject: Re: unstable xdp tests. Was: [PATCH net-next v4 31/33] bpf: add
 xdp.frame_sz in bpf_prog_test_run_xdp().

On Sat, 16 May 2020 21:02:01 -0700
Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:

> On Thu, May 14, 2020 at 3:51 AM Jesper Dangaard Brouer
> <brouer@...hat.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@...hat.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