| Message ID | 20220317165826.1099418-2-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic | expand |
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 >
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. [...]
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,
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(-)