From: Jakub Sitnicki <jakub@cloudflare•com>
To: Martin KaFai Lau <kafai@fb•com>
Cc: bpf@vger•kernel.org, netdev@vger•kernel.org,
Alexei Starovoitov <ast@kernel•org>,
Daniel Borkmann <daniel@iogearbox•net>,
Andrii Nakryiko <andrii@kernel•org>,
kernel-team@cloudflare•com, Ilya Leoshkevich <iii@linux•ibm.com>,
Heiko Carstens <hca@linux•ibm.com>,
Vasily Gorbik <gor@linux•ibm.com>
Subject: Re: [PATCH bpf-next 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field
Date: Fri, 18 Mar 2022 16:22:57 +0100 [thread overview]
Message-ID: <87fsnfz3lj.fsf@cloudflare.com> (raw)
In-Reply-To: <20220318012219.wtrpgaawg4czsqcj@kafai-mbp.dhcp.thefacebook.com>
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.
[...]
next prev parent reply other threads:[~2022-03-18 15:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-17 16:58 [PATCH bpf-next 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic Jakub Sitnicki
2022-03-17 16:58 ` [PATCH bpf-next 1/3] bpf: Treat bpf_sk_lookup remote_port as a 2-byte field Jakub Sitnicki
2022-03-18 1:22 ` Martin KaFai Lau
2022-03-18 15:22 ` Jakub Sitnicki [this message]
2022-03-17 16:58 ` [PATCH bpf-next 2/3] selftests/bpf: Fix u8 narrow load checks for bpf_sk_lookup remote_port Jakub Sitnicki
2022-03-17 16:58 ` [PATCH bpf-next 3/3] selftests/bpf: Fix test for 4-byte load from remote_port on big-endian Jakub Sitnicki
2022-03-18 1:23 ` [PATCH bpf-next 0/3] Make 2-byte access to bpf_sk_lookup->remote_port endian-agnostic Martin KaFai Lau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87fsnfz3lj.fsf@cloudflare.com \
--to=jakub@cloudflare$(echo .)com \
--cc=andrii@kernel$(echo .)org \
--cc=ast@kernel$(echo .)org \
--cc=bpf@vger$(echo .)kernel.org \
--cc=daniel@iogearbox$(echo .)net \
--cc=gor@linux$(echo .)ibm.com \
--cc=hca@linux$(echo .)ibm.com \
--cc=iii@linux$(echo .)ibm.com \
--cc=kafai@fb$(echo .)com \
--cc=kernel-team@cloudflare$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox