* [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic
@ 2022-03-19 18:33 Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field Jakub Sitnicki
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Jakub Sitnicki @ 2022-03-19 18:33 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
kernel-team, Martin KaFai Lau, Ilya Leoshkevich, Heiko Carstens,
Vasily Gorbik
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/
v1 -> v2:
- Remove needless check that 4-byte load is from &ctx->remote_port offset
(Martin)
[v1]: https://lore.kernel.org/bpf/20220317165826.1099418-1-jakub@cloudflare.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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf-next v2 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field
2022-03-19 18:33 [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic Jakub Sitnicki
@ 2022-03-19 18:33 ` Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 2/3] selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port Jakub Sitnicki
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Jakub Sitnicki @ 2022-03-19 18:33 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
kernel-team, Martin KaFai Lau, Ilya Leoshkevich, Heiko Carstens,
Vasily Gorbik
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")
Acked-by: Martin KaFai Lau <kafai@fb.com>
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..a7044e98765e 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 true;
+ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf-next v2 2/3] selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port
2022-03-19 18:33 [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field Jakub Sitnicki
@ 2022-03-19 18:33 ` Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 3/3] selftests/bpf: Fix test for 4-byte load from remote_port on big-endian Jakub Sitnicki
2022-03-21 2:10 ` [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Jakub Sitnicki @ 2022-03-19 18:33 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
kernel-team, Martin KaFai Lau, Ilya Leoshkevich, Heiko Carstens,
Vasily Gorbik
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>
Acked-by: Martin KaFai Lau <kafai@fb.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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf-next v2 3/3] selftests/bpf: Fix test for 4-byte load from remote_port on big-endian
2022-03-19 18:33 [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 2/3] selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port Jakub Sitnicki
@ 2022-03-19 18:33 ` Jakub Sitnicki
2022-03-21 2:10 ` [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Jakub Sitnicki @ 2022-03-19 18:33 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
kernel-team, Martin KaFai Lau, Ilya Leoshkevich, Heiko Carstens,
Vasily Gorbik
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")
Acked-by: Martin KaFai Lau <kafai@fb.com>
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic
2022-03-19 18:33 [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic Jakub Sitnicki
` (2 preceding siblings ...)
2022-03-19 18:33 ` [PATCH bpf-next v2 3/3] selftests/bpf: Fix test for 4-byte load from remote_port on big-endian Jakub Sitnicki
@ 2022-03-21 2:10 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-03-21 2:10 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: bpf, netdev, ast, daniel, andrii, kernel-team, kafai, iii, hca, gor
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Sat, 19 Mar 2022 19:33:53 +0100 you 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]:
>
> [...]
Here is the summary with links:
- [bpf-next,v2,1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field
https://git.kernel.org/bpf/bpf-next/c/058ec4a7d9cf
- [bpf-next,v2,2/3] selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port
https://git.kernel.org/bpf/bpf-next/c/3c69611b8926
- [bpf-next,v2,3/3] selftests/bpf: Fix test for 4-byte load from remote_port on big-endian
https://git.kernel.org/bpf/bpf-next/c/ce5236800116
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-21 2:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-19 18:33 [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 2/3] selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port Jakub Sitnicki
2022-03-19 18:33 ` [PATCH bpf-next v2 3/3] selftests/bpf: Fix test for 4-byte load from remote_port on big-endian Jakub Sitnicki
2022-03-21 2:10 ` [PATCH bpf-next v2 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.