Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup

[Jakub Sitnicki <jakub@cloudflare.com>]

On Sat, Feb 19, 2022 at 03:37 PM +01, Jakub Sitnicki wrote:
> On Thu, Feb 17, 2022 at 05:11 PM +01, Jakub Sitnicki wrote:
>> On Thu, Feb 17, 2022 at 03:18 PM +01, Ilya Leoshkevich wrote:
>>> On Wed, 2022-02-16 at 13:44 -0800, Andrii Nakryiko wrote:
>>>> On Wed, Feb 9, 2022 at 10:43 AM Jakub Sitnicki <jakub@cloudflare.com>
>>>> wrote:
>>
>> [...]
>>
>>>> > +       /* 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;
>>>> > +
>>>> 
>>>> Jakub, can you please double check that your patch set doesn't break
>>>> big-endian architectures? I've noticed that our s390x test runner is
>>>> now failing in the sk_lookup selftest. See [0]. Also CC'ing Ilya.
>>>
>>> I agree that this looks like an endianness issue. The new check seems
>>> to make little sense on big-endian to me, so I would just #ifdef it
>>> out.
>>
>> We have a very similar check for a load from context in
>> progs/test_sock_fields.c, which is not causing problems:
>>
>> static __noinline bool sk_dst_port__load_word(struct bpf_sock *sk)
>> {
>> 	__u32 *word = (__u32 *)&sk->dst_port;
>> 	return word[0] == bpf_htonl(0xcafe0000);
>> }
>>
>> So I think I just messed something up here. Will dig into it.
>
> Pretty sure the source of the problem here is undefined behaviour. Can't
> legally shift u16 by 16 bits like I did in the `bpf_ntohs(SRC_PORT) <<
> 16` expression. Will fix.

Proposed fix posted, but forgot to CC Ilya so linking here:

https://lore.kernel.org/bpf/20220221180358.169101-1-jakub@cloudflare.com/