Re: [PATCH RFC bpf-next 2/3] bpf: Fix bpf_sk_lookup.remote_port on big-endian

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Sun, 2022-02-27 at 21:30 +0100, Jakub Sitnicki wrote:
> 
> On Sat, Feb 26, 2022 at 06:44 PM -08, Alexei Starovoitov wrote:
> > On Tue, Feb 22, 2022 at 07:25:58PM +0100, Ilya Leoshkevich wrote:
> > > On big-endian, the port is available in the second __u16, not the
> > > first
> > > one. Therefore, provide a big-endian-specific definition that
> > > reflects
> > > that. Also, define remote_port_compat in order to have nicer
> > > architecture-agnostic code in the verifier and in tests.
> > > 
> > > Fixes: 9a69e2b385f4 ("bpf: Make remote_port field in struct
> > > bpf_sk_lookup 16-bit wide")
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  include/uapi/linux/bpf.h       | 17 +++++++++++++++--
> > >  net/core/filter.c              |  5 ++---
> > >  tools/include/uapi/linux/bpf.h | 17 +++++++++++++++--
> > >  3 files changed, 32 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index afe3d0d7f5f2..7b0e5efa58e0 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -10,6 +10,7 @@
> > >  
> > >  #include <linux/types.h>
> > >  #include <linux/bpf_common.h>
> > > +#include <asm/byteorder.h>
> > >  
> > >  /* Extended instruction set based on top of classic BPF */
> > >  
> > > @@ -6453,8 +6454,20 @@ struct bpf_sk_lookup {
> > >         __u32 protocol;         /* IP protocol (IPPROTO_TCP,
> > > IPPROTO_UDP) */
> > >         __u32 remote_ip4;       /* Network byte order */
> > >         __u32 remote_ip6[4];    /* Network byte order */
> > > -       __be16 remote_port;     /* Network byte order */
> > > -       __u16 :16;              /* Zero padding */
> > > +       union {
> > > +               struct {
> > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN :
> > > defined(__LITTLE_ENDIAN)
> > > +                       __be16 remote_port;     /* Network byte
> > > order */
> > > +                       __u16 :16;              /* Zero padding
> > > */
> > > +#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN :
> > > defined(__BIG_ENDIAN)
> > > +                       __u16 :16;              /* Zero padding
> > > */
> > > +                       __be16 remote_port;     /* Network byte
> > > order */
> > > +#else
> > > +#error unspecified endianness
> > > +#endif
> > > +               };
> > > +               __u32 remote_port_compat;
> > 
> > Sorry this hack is not an option.
> > Don't have any suggestions at this point. Pls come up with
> > something else.
> 
> I think we can keep the bpf_sk_lookup definition as is, if we leave
> the
> 4-byte load from remote_port offset quirky behavior on little-endian.
> 
> Please take a look at the test fix I've posted for 4-byte load from
> bpf_sock dst_port that works for me on x86_64 and s390. It is exactly
> the same case as we're dealing with here:
> 
> https://lore.kernel.org/bpf/20220227202757.519015-4-jakub@cloudflare.com/T/#u
> 

What about 2-byte loads?

static __noinline bool sk_dst_port__load_half(struct bpf_sock *sk)
{
	__u16 *half = (__u16 *)&sk->dst_port;
	return half[0] == bpf_htons(0xcafe);
}

requires "ca fe ?? ??" in memory on BE, while

static __noinline bool sk_dst_port__load_word(struct bpf_sock *sk)
{
#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
	const __u8 SHIFT = 16;
#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
	const __u8 SHIFT = 0;
#else
#error "Unrecognized __BYTE_ORDER__"
#endif
	__u32 *word = (__u32 *)&sk->dst_port;
	return word[0] == bpf_htonl(0xcafe << SHIFT);
}

requires "00 00 ca fe". This is inconsistent. Furthermore, one
cannot see it with bpf_sock thanks to

	case offsetofend(struct bpf_sock, dst_port) ...
	     offsetof(struct bpf_sock, dst_ip4) - 1:
		return false;

however, with sk_lookup this is the case: loading the most significant
half of the port produces non-zero! So, it's not simply a quirkiness of
the 4-byte load, it's a mutual inconsistency between LSW loads, MSW
loads and 4-byte loads.

One might argue that we can live with that, especially since all the
user-relevant tests pass - here I can only say that an inconsistency on
such a fundamental level makes me nervous.

In order to resolve this inconsistency I've implemented patch 1 of this
series. With that, "sk->dst_port == bpf_htons(0xcafe)" starts to fail,
and that's where one needs something like this patch.

Best regards,
Ilya