* [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field
@ 2022-02-09 18:43 Jakub Sitnicki
2022-02-09 18:43 ` [PATCH bpf-next v2 1/2] bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide Jakub Sitnicki
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jakub Sitnicki @ 2022-02-09 18:43 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
kernel-team, Yonghong Song
Following the recent split-up of the bpf_sock dst_port field, apply the same to
technique to the bpf_sk_lookup remote_port field to make uAPI more user
friendly.
v1 -> v2:
- Remove remote_port range check and cast to be16 in TEST_RUN for sk_lookup
(kernel test robot)
Jakub Sitnicki (2):
bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide
selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
include/uapi/linux/bpf.h | 3 ++-
net/bpf/test_run.c | 4 ++--
net/core/filter.c | 3 ++-
tools/include/uapi/linux/bpf.h | 3 ++-
tools/testing/selftests/bpf/progs/test_sk_lookup.c | 6 ++++++
5 files changed, 14 insertions(+), 5 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf-next v2 1/2] bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide
2022-02-09 18:43 [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field Jakub Sitnicki
@ 2022-02-09 18:43 ` Jakub Sitnicki
2022-02-09 18:43 ` [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup Jakub Sitnicki
2022-02-09 19:50 ` [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field patchwork-bot+netdevbpf
2 siblings, 0 replies; 9+ messages in thread
From: Jakub Sitnicki @ 2022-02-09 18:43 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
kernel-team, Yonghong Song
remote_port is another case of a BPF context field documented as a 32-bit
value in network byte order for which the BPF context access converter
generates a load of a zero-padded 16-bit integer in network byte order.
First such case was dst_port in bpf_sock which got addressed in commit
4421a582718a ("bpf: Make dst_port field in struct bpf_sock 16-bit wide").
Loading 4-bytes from the remote_port offset and converting the value with
bpf_ntohl() leads to surprising results, as the expected value is shifted
by 16 bits.
Reduce the confusion by splitting the field in two - a 16-bit field holding
a big-endian integer, and a 16-bit zero-padding anonymous field that
follows it.
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
include/uapi/linux/bpf.h | 3 ++-
net/bpf/test_run.c | 4 ++--
net/core/filter.c | 3 ++-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a7f0ddedac1f..afe3d0d7f5f2 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6453,7 +6453,8 @@ struct bpf_sk_lookup {
__u32 protocol; /* IP protocol (IPPROTO_TCP, IPPROTO_UDP) */
__u32 remote_ip4; /* Network byte order */
__u32 remote_ip6[4]; /* Network byte order */
- __u32 remote_port; /* Network byte order */
+ __be16 remote_port; /* Network byte order */
+ __u16 :16; /* Zero padding */
__u32 local_ip4; /* Network byte order */
__u32 local_ip6[4]; /* Network byte order */
__u32 local_port; /* Host byte order */
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 0220b0822d77..8c2608567555 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -1146,7 +1146,7 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog, const union bpf_attr *kat
if (!range_is_zero(user_ctx, offsetofend(typeof(*user_ctx), local_port), sizeof(*user_ctx)))
goto out;
- if (user_ctx->local_port > U16_MAX || user_ctx->remote_port > U16_MAX) {
+ if (user_ctx->local_port > U16_MAX) {
ret = -ERANGE;
goto out;
}
@@ -1154,7 +1154,7 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog, const union bpf_attr *kat
ctx.family = (u16)user_ctx->family;
ctx.protocol = (u16)user_ctx->protocol;
ctx.dport = (u16)user_ctx->local_port;
- ctx.sport = (__force __be16)user_ctx->remote_port;
+ ctx.sport = user_ctx->remote_port;
switch (ctx.family) {
case AF_INET:
diff --git a/net/core/filter.c b/net/core/filter.c
index 99a05199a806..83f06d3b2c52 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10843,7 +10843,8 @@ 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 bpf_ctx_range(struct bpf_sk_lookup, remote_port):
+ 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));
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
2022-02-09 18:43 [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field Jakub Sitnicki
2022-02-09 18:43 ` [PATCH bpf-next v2 1/2] bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide Jakub Sitnicki
@ 2022-02-09 18:43 ` Jakub Sitnicki
2022-02-16 21:44 ` Andrii Nakryiko
2022-02-09 19:50 ` [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field patchwork-bot+netdevbpf
2 siblings, 1 reply; 9+ messages in thread
From: Jakub Sitnicki @ 2022-02-09 18:43 UTC (permalink / raw)
To: bpf
Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
kernel-team, Yonghong Song
Extend the context access tests for sk_lookup prog to cover the surprising
case of a 4-byte load from the remote_port field, where the expected value
is actually shifted by 16 bits.
Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 3 ++-
tools/testing/selftests/bpf/progs/test_sk_lookup.c | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a7f0ddedac1f..afe3d0d7f5f2 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6453,7 +6453,8 @@ struct bpf_sk_lookup {
__u32 protocol; /* IP protocol (IPPROTO_TCP, IPPROTO_UDP) */
__u32 remote_ip4; /* Network byte order */
__u32 remote_ip6[4]; /* Network byte order */
- __u32 remote_port; /* Network byte order */
+ __be16 remote_port; /* Network byte order */
+ __u16 :16; /* Zero padding */
__u32 local_ip4; /* Network byte order */
__u32 local_ip6[4]; /* Network byte order */
__u32 local_port; /* Host byte order */
diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
index 83b0aaa52ef7..bf5b7caefdd0 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
@@ -392,6 +392,7 @@ int ctx_narrow_access(struct bpf_sk_lookup *ctx)
{
struct bpf_sock *sk;
int err, family;
+ __u32 val_u32;
bool v4;
v4 = (ctx->family == AF_INET);
@@ -418,6 +419,11 @@ 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) */
+ val_u32 = *(__u32 *)&ctx->remote_port;
+ if (val_u32 != bpf_htonl(bpf_ntohs(SRC_PORT) << 16))
+ return SK_DROP;
+
/* Narrow loads from local_port field. Expect DST_PORT. */
if (LSB(ctx->local_port, 0) != ((DST_PORT >> 0) & 0xff) ||
LSB(ctx->local_port, 1) != ((DST_PORT >> 8) & 0xff) ||
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field
2022-02-09 18:43 [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field Jakub Sitnicki
2022-02-09 18:43 ` [PATCH bpf-next v2 1/2] bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide Jakub Sitnicki
2022-02-09 18:43 ` [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup Jakub Sitnicki
@ 2022-02-09 19:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-02-09 19:50 UTC (permalink / raw)
To: Jakub Sitnicki; +Cc: bpf, netdev, ast, daniel, andrii, kernel-team, yhs
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Wed, 9 Feb 2022 19:43:31 +0100 you wrote:
> Following the recent split-up of the bpf_sock dst_port field, apply the same to
> technique to the bpf_sk_lookup remote_port field to make uAPI more user
> friendly.
>
> v1 -> v2:
> - Remove remote_port range check and cast to be16 in TEST_RUN for sk_lookup
> (kernel test robot)
>
> [...]
Here is the summary with links:
- [bpf-next,v2,1/2] bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide
https://git.kernel.org/bpf/bpf-next/c/9a69e2b385f4
- [bpf-next,v2,2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
https://git.kernel.org/bpf/bpf-next/c/2ed0dc5937d3
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] 9+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
2022-02-09 18:43 ` [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup Jakub Sitnicki
@ 2022-02-16 21:44 ` Andrii Nakryiko
2022-02-17 14:18 ` Ilya Leoshkevich
0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2022-02-16 21:44 UTC (permalink / raw)
To: Jakub Sitnicki, Ilya Leoshkevich
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, kernel-team, Yonghong Song
On Wed, Feb 9, 2022 at 10:43 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> Extend the context access tests for sk_lookup prog to cover the surprising
> case of a 4-byte load from the remote_port field, where the expected value
> is actually shifted by 16 bits.
>
> Acked-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
> tools/include/uapi/linux/bpf.h | 3 ++-
> tools/testing/selftests/bpf/progs/test_sk_lookup.c | 6 ++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index a7f0ddedac1f..afe3d0d7f5f2 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -6453,7 +6453,8 @@ struct bpf_sk_lookup {
> __u32 protocol; /* IP protocol (IPPROTO_TCP, IPPROTO_UDP) */
> __u32 remote_ip4; /* Network byte order */
> __u32 remote_ip6[4]; /* Network byte order */
> - __u32 remote_port; /* Network byte order */
> + __be16 remote_port; /* Network byte order */
> + __u16 :16; /* Zero padding */
> __u32 local_ip4; /* Network byte order */
> __u32 local_ip6[4]; /* Network byte order */
> __u32 local_port; /* Host byte order */
> diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
> index 83b0aaa52ef7..bf5b7caefdd0 100644
> --- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
> +++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
> @@ -392,6 +392,7 @@ int ctx_narrow_access(struct bpf_sk_lookup *ctx)
> {
> struct bpf_sock *sk;
> int err, family;
> + __u32 val_u32;
> bool v4;
>
> v4 = (ctx->family == AF_INET);
> @@ -418,6 +419,11 @@ 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) */
> + 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.
[0] https://github.com/libbpf/libbpf/runs/5220996832?check_suite_focus=true
> /* Narrow loads from local_port field. Expect DST_PORT. */
> if (LSB(ctx->local_port, 0) != ((DST_PORT >> 0) & 0xff) ||
> LSB(ctx->local_port, 1) != ((DST_PORT >> 8) & 0xff) ||
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
2022-02-16 21:44 ` Andrii Nakryiko
@ 2022-02-17 14:18 ` Ilya Leoshkevich
2022-02-17 16:11 ` Jakub Sitnicki
0 siblings, 1 reply; 9+ messages in thread
From: Ilya Leoshkevich @ 2022-02-17 14:18 UTC (permalink / raw)
To: Andrii Nakryiko, Jakub Sitnicki
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, kernel-team, Yonghong Song
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:
> >
> > Extend the context access tests for sk_lookup prog to cover the
> > surprising
> > case of a 4-byte load from the remote_port field, where the
> > expected value
> > is actually shifted by 16 bits.
> >
> > Acked-by: Yonghong Song <yhs@fb.com>
> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> > ---
> > tools/include/uapi/linux/bpf.h | 3 ++-
> > tools/testing/selftests/bpf/progs/test_sk_lookup.c | 6 ++++++
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/include/uapi/linux/bpf.h
> > b/tools/include/uapi/linux/bpf.h
> > index a7f0ddedac1f..afe3d0d7f5f2 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -6453,7 +6453,8 @@ struct bpf_sk_lookup {
> > __u32 protocol; /* IP protocol (IPPROTO_TCP,
> > IPPROTO_UDP) */
> > __u32 remote_ip4; /* Network byte order */
> > __u32 remote_ip6[4]; /* Network byte order */
> > - __u32 remote_port; /* Network byte order */
> > + __be16 remote_port; /* Network byte order */
> > + __u16 :16; /* Zero padding */
> > __u32 local_ip4; /* Network byte order */
> > __u32 local_ip6[4]; /* Network byte order */
> > __u32 local_port; /* Host byte order */
> > diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
> > b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
> > index 83b0aaa52ef7..bf5b7caefdd0 100644
> > --- a/tools/testing/selftests/bpf/progs/test_sk_lookup.c
> > +++ b/tools/testing/selftests/bpf/progs/test_sk_lookup.c
> > @@ -392,6 +392,7 @@ int ctx_narrow_access(struct bpf_sk_lookup
> > *ctx)
> > {
> > struct bpf_sock *sk;
> > int err, family;
> > + __u32 val_u32;
> > bool v4;
> >
> > v4 = (ctx->family == AF_INET);
> > @@ -418,6 +419,11 @@ 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) */
> > + 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.
>
> [0]
> https://github.com/libbpf/libbpf/runs/5220996832?check_suite_focus=true
>
> > /* Narrow loads from local_port field. Expect DST_PORT. */
> > if (LSB(ctx->local_port, 0) != ((DST_PORT >> 0) & 0xff) ||
> > LSB(ctx->local_port, 1) != ((DST_PORT >> 8) & 0xff) ||
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
2022-02-17 14:18 ` Ilya Leoshkevich
@ 2022-02-17 16:11 ` Jakub Sitnicki
2022-02-19 14:37 ` Jakub Sitnicki
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Sitnicki @ 2022-02-17 16:11 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, kernel-team, Yonghong Song
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.
[...]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
2022-02-17 16:11 ` Jakub Sitnicki
@ 2022-02-19 14:37 ` Jakub Sitnicki
2022-02-21 18:34 ` Jakub Sitnicki
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Sitnicki @ 2022-02-19 14:37 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, kernel-team, Yonghong Song
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup
2022-02-19 14:37 ` Jakub Sitnicki
@ 2022-02-21 18:34 ` Jakub Sitnicki
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Sitnicki @ 2022-02-21 18:34 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, kernel-team, Yonghong Song
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/
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-02-21 18:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09 18:43 [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field Jakub Sitnicki
2022-02-09 18:43 ` [PATCH bpf-next v2 1/2] bpf: Make remote_port field in struct bpf_sk_lookup 16-bit wide Jakub Sitnicki
2022-02-09 18:43 ` [PATCH bpf-next v2 2/2] selftests/bpf: Cover 4-byte load from remote_port in bpf_sk_lookup Jakub Sitnicki
2022-02-16 21:44 ` Andrii Nakryiko
2022-02-17 14:18 ` Ilya Leoshkevich
2022-02-17 16:11 ` Jakub Sitnicki
2022-02-19 14:37 ` Jakub Sitnicki
2022-02-21 18:34 ` Jakub Sitnicki
2022-02-09 19:50 ` [PATCH bpf-next v2 0/2] Split bpf_sk_lookup remote_port field 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.