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

[Jakub Sitnicki <jakub@cloudflare.com>]

On Mon, Mar 14, 2022 at 07:25 PM +01, Ilya Leoshkevich wrote:
> On Mon, 2022-03-14 at 18:35 +0100, Jakub Sitnicki wrote:
>> On Thu, Mar 10, 2022 at 11:57 PM +01, Jakub Sitnicki wrote:
>> > 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.
>> 
>> Context conversion with patch below applied looks correct to me on
>> s390x
>> as well:
>> 
>> * size=1, offset=0, r2 = *(u8 *)(r1 +36)
>>    0: (69) r2 = *(u16 *)(r1 +4)
>>    1: (bc) w2 = w2
>>    2: (74) w2 >>= 8
>>    3: (bc) w2 = w2
>>    4: (54) w2 &= 255
>>    5: (bc) w2 = w2
>>    6: (b7) r0 = 0
>>    7: (95) exit
>> 
>> * size=1, offset=1, r2 = *(u8 *)(r1 +37)
>>    0: (69) r2 = *(u16 *)(r1 +4)
>>    1: (bc) w2 = w2
>>    2: (54) w2 &= 255
>>    3: (bc) w2 = w2
>>    4: (b7) r0 = 0
>>    5: (95) exit
>> 
>> * size=1, offset=2, r2 = *(u8 *)(r1 +38)
>>    0: (b4) w2 = 0
>>    1: (bc) w2 = w2
>>    2: (74) w2 >>= 8
>>    3: (bc) w2 = w2
>>    4: (54) w2 &= 255
>>    5: (bc) w2 = w2
>>    6: (b7) r0 = 0
>>    7: (95) exit
>> 
>> * size=1, offset=3, r2 = *(u8 *)(r1 +39)
>>    0: (b4) w2 = 0
>>    1: (bc) w2 = w2
>>    2: (54) w2 &= 255
>>    3: (bc) w2 = w2
>>    4: (b7) r0 = 0
>>    5: (95) exit
>> 
>> * size=2, offset=0, r2 = *(u16 *)(r1 +36)
>>    0: (69) r2 = *(u16 *)(r1 +4)
>>    1: (bc) w2 = w2
>>    2: (b7) r0 = 0
>>    3: (95) exit
>> 
>> * size=2, offset=2, r2 = *(u16 *)(r1 +38)
>>    0: (b4) w2 = 0
>>    1: (bc) w2 = w2
>>    2: (b7) r0 = 0
>>    3: (95) exit
>> 
>> * size=4, offset=0, r2 = *(u32 *)(r1 +36)
>>    0: (69) r2 = *(u16 *)(r1 +4)
>>    1: (bc) w2 = w2
>>    2: (b7) r0 = 0
>>    3: (95) exit
>> 
>> If we go this way, this should unbreak the bpf selftests on BE,
>> independently of the patch 1 from this series.
>> 
>> Will send it as a patch, so that we continue the review discussion.
>
> I applied this patch to bpf-next, and while it looks reasonable, the
> test still fails, e.g. here:
>
> 	/* 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))
> 		return SK_DROP;
>

You are right. That's that the check I recently added that broke the bpf
CI runs for s390x [1], and started our thread.

I had a stale build of test_progs with a fix akin to patch [2] that I
was testing and missed that. Thanks for giving it a run.

If we go with Martin's suggestion [3] here and avoid bpf_htonl(), then
we could make it work and save ourselves endianess checks.

IOW, a patch like below would be also needed to unbreak the tests.

First chunk is copied from your fixes to test_sk_lookup in patch 3 in
this RFC series, naturally.

[1] https://lore.kernel.org/bpf/CAEf4BzaRNLw9_EnaMo5e46CdEkzbJiVU3j9oxnsemBKjNFf3wQ@mail.gmail.com/
[2] https://lore.kernel.org/bpf/20220227202757.519015-4-jakub@cloudflare.com/
[3] https://lore.kernel.org/bpf/20220301062207.d3aqge5qg623asr6@kafai-mbp.dhcp.thefacebook.com/

---8<---

diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
index bf5b7caefdd0..2765a4bd500c 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
@@ -413,15 +413,14 @@ int ctx_narrow_access(struct bpf_sk_lookup *ctx)

        /* 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)
+           LSB(ctx->remote_port, 1) != ((SRC_PORT >> 8) & 0xff))
                return SK_DROP;
        if (LSW(ctx->remote_port, 0) != SRC_PORT)
                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 (val_u32 != SRC_PORT)
                return SK_DROP;

        /* Narrow loads from local_port field. Expect DST_PORT. */