Re: [PATCH] bpf: fix selftest/bpf/test_pkt_md_access on s390x

[Daniel Borkmann]

On 08/04/2017 09:06 PM, Daniel Borkmann wrote:
> Hi Thomas,
>
> On 08/04/2017 08:40 AM, Thomas Richter wrote:
> > Commit 18f3d6be6be1 ("selftests/bpf: Add test cases to test narrower ctx field loads")
> > introduced new eBPF test cases. One of them (test_pkt_md_access.c)
> > fails on s390x. The BPF verifier error message is:
>
> Could you submit the fix to: netdev@xxxxxxxxxxxxxxx
>
> (BPF patches are handled/accepted there.)
>
> Thanks a lot,
> Daniel
>
> > [root@s8360046 bpf]# ./test_progs
> > test_pkt_access:PASS:ipv4 349 nsec
> > test_pkt_access:PASS:ipv6 212 nsec
> > [....]
> > libbpf: load bpf program failed: Permission denied
> > libbpf: -- BEGIN DUMP LOG ---
> > libbpf:
> > 0: (71) r2 = *(u8 *)(r1 +0)
> > invalid bpf_context access off=0 size=1
> >
> > libbpf: -- END LOG --
> > libbpf: failed to load program 'test1'
> > libbpf: failed to load object './test_pkt_md_access.o'
> > Summary: 29 PASSED, 1 FAILED
> > [root@s8360046 bpf]#
> >
> > This is caused by a byte endianess issue. S390x is a big endian
> > architecture.  Pointer access to the lowest byte or halfword of a
> > four byte value need to add an offset.
> > On little endian architectures this offset is not needed.
> >
> > Fix this and use the same approach as the originator used for other files
> > (for example test_verifier.c) in his original commit.
> >
> > With this fix the test program test_progs succeeds on s390x:
> > [root@s8360046 bpf]# ./test_progs
> > test_pkt_access:PASS:ipv4 236 nsec
> > test_pkt_access:PASS:ipv6 217 nsec
> > test_xdp:PASS:ipv4 3624 nsec
> > test_xdp:PASS:ipv6 1722 nsec
> > test_l4lb:PASS:ipv4 926 nsec
> > test_l4lb:PASS:ipv6 1322 nsec
> > test_tcp_estats:PASS: 0 nsec
> > test_bpf_obj_id:PASS:get-fd-by-notexist-prog-id 0 nsec
> > test_bpf_obj_id:PASS:get-fd-by-notexist-map-id 0 nsec
> > test_bpf_obj_id:PASS:get-prog-info(fd) 0 nsec
> > test_bpf_obj_id:PASS:get-map-info(fd) 0 nsec
> > test_bpf_obj_id:PASS:get-prog-info(fd) 0 nsec
> > test_bpf_obj_id:PASS:get-map-info(fd) 0 nsec
> > test_bpf_obj_id:PASS:get-prog-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-prog-info(next_id->fd) 0 nsec
> > test_bpf_obj_id:PASS:get-prog-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-prog-info(next_id->fd) 0 nsec
> > test_bpf_obj_id:PASS:check total prog id found by get_next_id 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:check get-map-info(next_id->fd) 0 nsec
> > test_bpf_obj_id:PASS:get-map-fd(next_id) 0 nsec
> > test_bpf_obj_id:PASS:check get-map-info(next_id->fd) 0 nsec
> > test_bpf_obj_id:PASS:check total map id found by get_next_id 0 nsec
> > test_pkt_md_access:PASS: 277 nsec
> > Summary: 30 PASSED, 0 FAILED
> > [root@s8360046 bpf]#
> >
> > Signed-off-by: Thomas Richter <tmricht@xxxxxxxxxxxxxxxxxx>
> > ---
> >   tools/testing/selftests/bpf/test_pkt_md_access.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/test_pkt_md_access.c b/tools/testing/selftests/bpf/test_pkt_md_access.c
> > index 71729d4..8393ced 100644
> > --- a/tools/testing/selftests/bpf/test_pkt_md_access.c
> > +++ b/tools/testing/selftests/bpf/test_pkt_md_access.c
> > @@ -12,12 +12,23 @@
> >
> >   int _version SEC("version") = 1;
> >
> > +#ifdef __LITTLE_ENDIAN

One more thing I noticed, could you do above check with:

  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__

This is basically the version that clang exposes, so that
this could then properly be compiled with both, bpfel/bpfeb
targets, see also 78a5a93c1eeb ("bpf, tests: fix endianness
selection") for some more context.

Thanks again,
Daniel

> >   #define TEST_FIELD(TYPE, FIELD, MASK)                    \
> >       {                                \
> >           TYPE tmp = *(volatile TYPE *)&skb->FIELD;        \
> >           if (tmp != ((*(volatile __u32 *)&skb->FIELD) & MASK))    \
> >               return TC_ACT_SHOT;                \
> >       }
> > +#else
> > +#define TEST_FIELD_OFFSET(a, b)    ((sizeof(a) - sizeof(b)) / sizeof(b))
> > +#define TEST_FIELD(TYPE, FIELD, MASK)                    \
> > +    {                                \
> > +        TYPE tmp = *((volatile TYPE *)&skb->FIELD +        \
> > +                  TEST_FIELD_OFFSET(skb->FIELD, TYPE));    \
> > +        if (tmp != ((*(volatile __u32 *)&skb->FIELD) & MASK))    \
> > +            return TC_ACT_SHOT;                \
> > +    }
> > +#endif
> >
> >   SEC("test1")
> >   int process(struct __sk_buff *skb)