| Message ID | 20220209184333.654927-3-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2ed0dc5937d38f282841d22c5a5354ec264c7b8d |
| Delegated to: | BPF |
| Headers | show |
| Series | Split bpf_sk_lookup remote_port field | expand |
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 >
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 > >
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. [...]
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.
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/
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) ||