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

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

On Mon, 2022-02-28 at 14:26 +0100, Jakub Sitnicki wrote:
> On Mon, Feb 28, 2022 at 11:19 AM +01, Ilya Leoshkevich wrote:
> > 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.
> 
> Truth be told I can't reproduce the said failure.
> I've extended the test with an additional check:
> 
>    306          bool ok = sk->dst_port == bpf_htons(0xcafe);
>    307          if (!ok)
>    308                  RET_LOG();
>    309          if (!sk_dst_port__load_word(sk))
>    310                  RET_LOG();
> 
> ... but it translates to the same BPF assembly as
> sk_dst_port__load_half. That is:
> 
> ; bool ok = sk->dst_port == bpf_htons(0xcafe);
>    9: (69) r1 = *(u16 *)(r6 +12)
>   10: (bc) w1 = w1
> ; if (!ok)
>   11: (16) if w1 == 0xcafe goto pc+2
>   12: (b4) w1 = 308
>   13: (05) goto pc+14
> 
> During the test I had patch 1 from this series applied on top of [1].
> The latter series should not matter, though, I didn't touch the
> access
> converter.
> 
> Mind sharing what does the `bpftool prog objdump` output look like
> for
> the failing test on your side?
> 
> [1]
> https://lore.kernel.org/bpf/20220227202757.519015-1-jakub@cloudflare.com/
> 

Sure.

As I mentioned, it's best demonstrated with sk_lookup, so that's what
I'll be using. Apply this on top of bpf-next:

--- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
@@ -392,7 +392,7 @@ int ctx_narrow_access(struct bpf_sk_lookup *ctx)
 {
 	struct bpf_sock *sk;
 	int err, family;
-	__u32 val_u32;
+	__u32 *ptr_u32;
 	bool v4;
 
 	v4 = (ctx->family == AF_INET);
@@ -411,17 +411,12 @@ int ctx_narrow_access(struct bpf_sk_lookup *ctx)
 	if (LSW(ctx->protocol, 0) != IPPROTO_TCP)
 		return SK_DROP;
 
-	/* Narrow loads from remote_port field. Expect SRC_PORT. */
-	if (LSB(ctx->remote_port, 0) != ((SRC_PORT >> 0) & 0xff) ||
-	    LSB(ctx->remote_port, 1) != ((SRC_PORT >> 8) & 0xff) ||
-	    LSB(ctx->remote_port, 2) != 0 || LSB(ctx->remote_port, 3)
!= 0)
+	ptr_u32 = (__u32 *)&ctx->remote_port;
+	if (LSW(*ptr_u32, 0) != SRC_PORT)
 		return SK_DROP;
-	if (LSW(ctx->remote_port, 0) != SRC_PORT)
+	if (LSW(*ptr_u32, 1) != 0)
 		return SK_DROP;
-
-	/* Load from remote_port field with zero padding (backward
compatibility) */
-	val_u32 = *(__u32 *)&ctx->remote_port;
-	if (val_u32 != bpf_htonl(bpf_ntohs(SRC_PORT) << 16))
+	if (*ptr_u32 != SRC_PORT)
 		return SK_DROP;
 
 	/* Narrow loads from local_port field. Expect DST_PORT. */

The checks look as follows:

$ llvm-objdump -d -l tools/testing/selftests/bpf/test_sk_lookup.o

; if (LSW(*ptr_u32, 0) != SRC_PORT)
     370:	69 26 00 26 00 00 00 00	r2 = *(u16 *)(r6 + 38)
     371:	56 20 01 00 00 00 1f 48	if w2 != 8008 goto
+256 <LBB15_135>

; if (LSW(*ptr_u32, 1) != 0)
     372:	69 26 00 24 00 00 00 00	r2 = *(u16 *)(r6 + 36)
     373:	56 20 00 fe 00 00 00 00	if w2 != 0 goto +254
<LBB15_135>

; if (*ptr_u32 != SRC_PORT)
     374:	61 26 00 24 00 00 00 00	r2 = *(u32 *)(r6 + 36)
     375:	56 20 00 fc 00 00 1f 48	if w2 != 8008 goto
+252 <LBB15_135>

After loading:

# tools/testing/selftests/bpf/tools/sbin/bpftool prog dump xlated id
141

; if (LSW(*ptr_u32, 0) != SRC_PORT)
  49: (69) r2 = *(u16 *)(r6 +4)
  50: (bc) w2 = w2
  51: (56) if w2 != 0x1f48 goto pc+938

; if (LSW(*ptr_u32, 1) != 0)
  52: (69) r2 = *(u16 *)(r6 +4)
  53: (bc) w2 = w2
  54: (56) if w2 != 0x0 goto pc+935

; if (*ptr_u32 != SRC_PORT)
  55: (69) r2 = *(u16 *)(r6 +4)
  56: (bc) w2 = w2
  57: (56) if w2 != 0x1f48 goto pc+932

Note that both LSW(0) and LSW(1) loads result in the same code after
rewriting. This is because the offset is ignored for this particular
combination of sizes.

Best regards,
Ilya