Re: [PATCH RFC bpf-next 1/3] bpf: Fix certain narrow loads with offsets

[Jakub Sitnicki <jakub@cloudflare.com>]

On Wed, Mar 09, 2022 at 01:34 PM +01, Ilya Leoshkevich wrote:
> On Wed, 2022-03-09 at 09:36 +0100, Jakub Sitnicki wrote:

[...]

>> 
>> Consider this - today the below is true on both LE and BE, right?
>> 
>>   *(u32 *)&ctx->remote_port == *(u16 *)&ctx->remote_port
>> 
>> because the loads get converted to:
>> 
>>   *(u16 *)&ctx_kern->sport == *(u16 *)&ctx_kern->sport
>> 
>> IOW, today, because of the bug that you are fixing here, the data
>> layout
>> changes from the PoV of the BPF program depending on the load size.
>> 
>> With 2-byte loads, without this patch, the data layout appears as:
>> 
>>   struct bpf_sk_lookup {
>>     ...
>>     __be16 remote_port;
>>     __be16 remote_port;
>>     ...
>>   }
>
> I see, one can indeed argue that this is also a part of the ABI now.
> So we're stuck between a rock and a hard place.
>
>> While for 4-byte loads, it appears as in your 2nd patch:
>> 
>>   struct bpf_sk_lookup {
>>     ...
>>     #if little-endian
>>     __be16 remote_port;
>>     __u16  :16; /* zero padding */
>>     #elif big-endian
>>     __u16  :16; /* zero padding */
>>     __be16 remote_port;
>>     #endif
>>     ...
>>   }
>> 
>> Because of that I don't see how we could keep complete ABI
>> compatiblity,
>> and have just one definition of struct bpf_sk_lookup that reflects
>> it. These are conflicting requirements.
>> 
>> I'd bite the bullet for 4-byte loads, for the sake of having an
>> endian-agnostic struct bpf_sk_lookup and struct bpf_sock definition
>> in
>> the uAPI header.
>> 
>> The sacrifice here is that the access converter will have to keep
>> rewriting 4-byte access to bpf_sk_lookup.remote_port and
>> bpf_sock.dst_port in this unexpected, quirky manner.
>> 
>> The expectation is that with time users will recompile their BPF
>> progs
>> against the updated bpf.h, and switch to 2-byte loads. That will make
>> the quirk in the access converter dead code in time.
>> 
>> I don't have any better ideas. Sorry.
>> 
>> [...]
>
> I agree, let's go ahead with this solution.
>
> The only remaining problem that I see is: the bug is in the common
> code, and it will affect the fields that we add in the future.
>
> Can we either document this state of things in a comment, or fix the
> bug and emulate the old behavior for certain fields?

I think we can fix the bug in the common code, and compensate for the
quirky 4-byte access to bpf_sk_lookup.remote_port and bpf_sock.dst_port
in the is_valid_access and convert_ctx_access.

With the patch as below, access to remote_port gets rewritten to:

* size=1, offset=0, r2 = *(u8 *)(r1 +36)
   0: (69) r2 = *(u16 *)(r1 +4)
   1: (54) w2 &= 255
   2: (b7) r0 = 0
   3: (95) exit

* size=1, offset=1, r2 = *(u8 *)(r1 +37)
   0: (69) r2 = *(u16 *)(r1 +4)
   1: (74) w2 >>= 8
   2: (54) w2 &= 255
   3: (b7) r0 = 0
   4: (95) exit

* size=1, offset=2, r2 = *(u8 *)(r1 +38)
   0: (b4) w2 = 0
   1: (54) w2 &= 255
   2: (b7) r0 = 0
   3: (95) exit

* size=1, offset=3, r2 = *(u8 *)(r1 +39)
   0: (b4) w2 = 0
   1: (74) w2 >>= 8
   2: (54) w2 &= 255
   3: (b7) r0 = 0
   4: (95) exit

* size=2, offset=0, r2 = *(u16 *)(r1 +36)
   0: (69) r2 = *(u16 *)(r1 +4)
   1: (b7) r0 = 0
   2: (95) exit

* size=2, offset=2, r2 = *(u16 *)(r1 +38)
   0: (b4) w2 = 0
   1: (b7) r0 = 0
   2: (95) exit

* size=4, offset=0, r2 = *(u32 *)(r1 +36)
   0: (69) r2 = *(u16 *)(r1 +4)
   1: (b7) r0 = 0
   2: (95) exit

How does that look to you?

Still need to give it a test on s390x.

--8<--

diff --git a/net/core/filter.c b/net/core/filter.c
index 65869fd510e8..2625a1d2dabc 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10856,13 +10856,24 @@ static bool sk_lookup_is_valid_access(int off, int size,
 	case bpf_ctx_range(struct bpf_sk_lookup, local_ip4):
 	case bpf_ctx_range_till(struct bpf_sk_lookup, remote_ip6[0], remote_ip6[3]):
 	case bpf_ctx_range_till(struct bpf_sk_lookup, local_ip6[0], local_ip6[3]):
-	case offsetof(struct bpf_sk_lookup, remote_port) ...
-	     offsetof(struct bpf_sk_lookup, local_ip4) - 1:
 	case bpf_ctx_range(struct bpf_sk_lookup, local_port):
 	case bpf_ctx_range(struct bpf_sk_lookup, ingress_ifindex):
 		bpf_ctx_record_field_size(info, sizeof(__u32));
 		return bpf_ctx_narrow_access_ok(off, size, sizeof(__u32));
 
+	case bpf_ctx_range(struct bpf_sk_lookup, remote_port):
+		/* Allow 4-byte access to 2-byte field for backward compatibility */
+		if (size == sizeof(__u32))
+			return off == offsetof(struct bpf_sk_lookup, remote_port);
+		bpf_ctx_record_field_size(info, sizeof(__be16));
+		return bpf_ctx_narrow_access_ok(off, size, sizeof(__be16));
+
+	case offsetofend(struct bpf_sk_lookup, remote_port) ...
+	     offsetof(struct bpf_sk_lookup, local_ip4) - 1:
+		/* Allow access to zero padding for backward compatiblity */
+		bpf_ctx_record_field_size(info, sizeof(__u16));
+		return bpf_ctx_narrow_access_ok(off, size, sizeof(__u16));
+
 	default:
 		return false;
 	}
@@ -10944,6 +10955,11 @@ static u32 sk_lookup_convert_ctx_access(enum bpf_access_type type,
 						     sport, 2, target_size));
 		break;
 
+	case offsetofend(struct bpf_sk_lookup, remote_port):
+		*target_size = 2;
+		*insn++ = BPF_MOV32_IMM(si->dst_reg, 0);
+		break;
+
 	case offsetof(struct bpf_sk_lookup, local_port):
 		*insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
 				      bpf_target_off(struct bpf_sk_lookup_kern,