[PATCH bpf-next 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic

[PATCH bpf-next 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic

[Jakub Sitnicki]

This patch set is a result of a discussion we had around the RFC patchset from
Ilya [1]. The fix for the narrow loads from the RFC series is still relevant,
but this series does not depend on it. Nor is it required to unbreak sk_lookup
tests on BE, if this series gets applied.

To summarize the takeaways from [1]:

 1) we want to make 2-byte load from ctx->remote_port portable across LE and BE,
 2) we keep the 4-byte load from ctx->remote_port as it is today - result varies
    on endianess of the platform.

[1] https://lore.kernel.org/bpf/20220222182559.2865596-2-iii@linux.ibm.com/

Jakub Sitnicki (3):
  bpf: Treat bpf_sk_lookup remote_port as a 2-byte field
  selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port
  selftests/bpf: Fix test for 4-byte load from remote_port on big-endian

 net/core/filter.c                             | 20 +++++++++++++++++--
 .../selftests/bpf/progs/test_sk_lookup.c      | 13 ++++++++----
 2 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.35.1

[PATCH bpf-next 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field

[Jakub Sitnicki]

In commit 9a69e2b385f4 ("bpf: Make remote_port field in struct
bpf_sk_lookup 16-bit wide") the remote_port field has been split up and
re-declared from u32 to be16.

However, the accompanying changes to the context access converter have not
been well thought through when it comes big-endian platforms.

Today 2-byte wide loads from offsetof(struct bpf_sk_lookup, remote_port)
are handled as narrow loads from a 4-byte wide field.

This by itself is not enough to create a problem, but when we combine

 1. 32-bit wide access to ->remote_port backed by a 16-wide wide load, with
 2. inherent difference between litte- and big-endian in how narrow loads
    need have to be handled (see bpf_ctx_narrow_access_offset),

we get inconsistent results for a 2-byte loads from &ctx->remote_port on LE
and BE architectures. This in turn makes BPF C code for the common case of
2-byte load from ctx->remote_port not portable.

To rectify it, inform the context access converter that remote_port is
2-byte wide field, and only 1-byte loads need to be treated as narrow
loads.

At the same time, we special-case the 4-byte load from &ctx->remote_port to
continue handling it the same way as do today, in order to keep the
existing BPF programs working.

Fixes: 9a69e2b385f4 ("bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide")
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/filter.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 03655f2074ae..9b1e453baf6d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10989,13 +10989,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 compatibility */
+		bpf_ctx_record_field_size(info, sizeof(__u16));
+		return bpf_ctx_narrow_access_ok(off, size, sizeof(__u16));
+
 	default:
 		return false;
 	}
@@ -11077,6 +11088,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,
-- 
2.35.1

Re: [PATCH bpf-next 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field

[Martin KaFai Lau]

On Thu, Mar 17, 2022 at 05:58:24PM +0100, Jakub Sitnicki wrote:
> In commit 9a69e2b385f4 ("bpf: Make remote_port field in struct
> bpf_sk_lookup 16-bit wide") the remote_port field has been split up and
> re-declared from u32 to be16.
> 
> However, the accompanying changes to the context access converter have not
> been well thought through when it comes big-endian platforms.
> 
> Today 2-byte wide loads from offsetof(struct bpf_sk_lookup, remote_port)
> are handled as narrow loads from a 4-byte wide field.
> 
> This by itself is not enough to create a problem, but when we combine
> 
>  1. 32-bit wide access to ->remote_port backed by a 16-wide wide load, with
>  2. inherent difference between litte- and big-endian in how narrow loads
>     need have to be handled (see bpf_ctx_narrow_access_offset),
> 
> we get inconsistent results for a 2-byte loads from &ctx->remote_port on LE
> and BE architectures. This in turn makes BPF C code for the common case of
> 2-byte load from ctx->remote_port not portable.
> 
> To rectify it, inform the context access converter that remote_port is
> 2-byte wide field, and only 1-byte loads need to be treated as narrow
> loads.
> 
> At the same time, we special-case the 4-byte load from &ctx->remote_port to
> continue handling it the same way as do today, in order to keep the
> existing BPF programs working.
> 
> Fixes: 9a69e2b385f4 ("bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide")
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  net/core/filter.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 03655f2074ae..9b1e453baf6d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -10989,13 +10989,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);
nit. The bad "off" value should have been rejected earlier in the
"if (off % size != 0)" check?

> +		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 compatibility */
> +		bpf_ctx_record_field_size(info, sizeof(__u16));
> +		return bpf_ctx_narrow_access_ok(off, size, sizeof(__u16));
> +
>  	default:
>  		return false;
>  	}
> @@ -11077,6 +11088,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,
> -- 
> 2.35.1
> 

Re: [PATCH bpf-next 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field

[Jakub Sitnicki]

On Thu, Mar 17, 2022 at 06:22 PM -07, Martin KaFai Lau wrote:
> On Thu, Mar 17, 2022 at 05:58:24PM +0100, Jakub Sitnicki wrote:
>> In commit 9a69e2b385f4 ("bpf: Make remote_port field in struct
>> bpf_sk_lookup 16-bit wide") the remote_port field has been split up and
>> re-declared from u32 to be16.
>> 
>> However, the accompanying changes to the context access converter have not
>> been well thought through when it comes big-endian platforms.
>> 
>> Today 2-byte wide loads from offsetof(struct bpf_sk_lookup, remote_port)
>> are handled as narrow loads from a 4-byte wide field.
>> 
>> This by itself is not enough to create a problem, but when we combine
>> 
>>  1. 32-bit wide access to ->remote_port backed by a 16-wide wide load, with
>>  2. inherent difference between litte- and big-endian in how narrow loads
>>     need have to be handled (see bpf_ctx_narrow_access_offset),
>> 
>> we get inconsistent results for a 2-byte loads from &ctx->remote_port on LE
>> and BE architectures. This in turn makes BPF C code for the common case of
>> 2-byte load from ctx->remote_port not portable.
>> 
>> To rectify it, inform the context access converter that remote_port is
>> 2-byte wide field, and only 1-byte loads need to be treated as narrow
>> loads.
>> 
>> At the same time, we special-case the 4-byte load from &ctx->remote_port to
>> continue handling it the same way as do today, in order to keep the
>> existing BPF programs working.
>> 
>> Fixes: 9a69e2b385f4 ("bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide")
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>>  net/core/filter.c | 20 ++++++++++++++++++--
>>  1 file changed, 18 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 03655f2074ae..9b1e453baf6d 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -10989,13 +10989,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);
> nit. The bad "off" value should have been rejected earlier in the
> "if (off % size != 0)" check?

Good catch. That is always true. I will respin.

Thanks for reviewing the patch sets.

[...]

[PATCH bpf-next 2/3] selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port

[Jakub Sitnicki]

In commit 9a69e2b385f4 ("bpf: Make remote_port field in struct
bpf_sk_lookup 16-bit wide") ->remote_port field changed from __u32 to
__be16.

However, narrow load tests which exercise 1-byte sized loads from
offsetof(struct bpf_sk_lookup, remote_port) were not adopted to reflect the
change.

As a result, on little-endian we continue testing loads from addresses:

 - (__u8 *)&ctx->remote_port + 3
 - (__u8 *)&ctx->remote_port + 4

which map to the zero padding following the remote_port field, and don't
break the tests because there is no observable change.

While on big-endian, we observe breakage because tests expect to see zeros
for values loaded from:

 - (__u8 *)&ctx->remote_port - 1
 - (__u8 *)&ctx->remote_port - 2

Above addresses map to ->remote_ip6 field, which precedes ->remote_port,
and are populated during the bpf_sk_lookup IPv6 tests.

Unsurprisingly, on s390x we observe:

  #136/38 sk_lookup/narrow access to ctx v4:OK
  #136/39 sk_lookup/narrow access to ctx v6:FAIL

Fix it by removing the checks for 1-byte loads from offsets outside of the
->remote_port field.

Fixes: 9a69e2b385f4 ("bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide")
Suggested-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/bpf/progs/test_sk_lookup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
index bf5b7caefdd0..38b7a1fe67b6 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
@@ -413,8 +413,7 @@ 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;
-- 
2.35.1

[PATCH bpf-next 3/3] selftests/bpf: Fix test for 4-byte load from remote_port on big-endian

[Jakub Sitnicki]

The context access converter rewrites the 4-byte load from
bpf_sk_lookup->remote_port to a 2-byte load from bpf_sk_lookup_kern
structure.

It means that we cannot treat the destination register contents as a 32-bit
value, or the code will not be portable across big- and little-endian
architectures.

This is exactly the same case as with 4-byte loads from bpf_sock->dst_port
so follow the approach outlined in [1] and treat the register contents as a
16-bit value in the test.

[1]: https://lore.kernel.org/bpf/20220317113920.1068535-5-jakub@cloudflare.com/

Fixes: 2ed0dc5937d3 ("selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup")
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/bpf/progs/test_sk_lookup.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
index 38b7a1fe67b6..6058dcb11b36 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
@@ -418,9 +418,15 @@ int ctx_narrow_access(struct bpf_sk_lookup *ctx)
 	if (LSW(ctx->remote_port, 0) != SRC_PORT)
 		return SK_DROP;
 
-	/* Load from remote_port field with zero padding (backward compatibility) */
+	/*
+	 * NOTE: 4-byte load from bpf_sk_lookup at remote_port offset
+	 * is quirky. It gets rewritten by the access converter to a
+	 * 2-byte load for backward compatibility. Treating the load
+	 * result as a be16 value makes the code portable across
+	 * little- and big-endian platforms.
+	 */
 	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. */
-- 
2.35.1

Re: [PATCH bpf-next 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic

[Martin KaFai Lau]

On Thu, Mar 17, 2022 at 05:58:23PM +0100, Jakub Sitnicki wrote:
> This patch set is a result of a discussion we had around the RFC patchset from
> Ilya [1]. The fix for the narrow loads from the RFC series is still relevant,
> but this series does not depend on it. Nor is it required to unbreak sk_lookup
> tests on BE, if this series gets applied.
> 
> To summarize the takeaways from [1]:
> 
>  1) we want to make 2-byte load from ctx->remote_port portable across LE and BE,
>  2) we keep the 4-byte load from ctx->remote_port as it is today - result varies
>     on endianess of the platform.
> 
> [1] https://lore.kernel.org/bpf/20220222182559.2865596-2-iii@linux.ibm.com/
For the set,

Acked-by: Martin KaFai Lau <kafai@fb.com>