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@xxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxx/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
