From: John Fastabend Subject: Re: [PATCH bpf-next 1/3] bpf: sockmap, metadata support for reporting size of msg Date: Wed, 19 Dec 2018 22:30:05 -0800 Message-ID: References: <1545004026-5127-1-git-send-email-john.fastabend@gmail.com> <1545004026-5127-2-git-send-email-john.fastabend@gmail.com> <20181220024949.xrgns7nelsxglkhp@ast-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: daniel@iogearbox.net, ast@kernel.org, netdev@vger.kernel.org To: Alexei Starovoitov Return-path: Received: from mail-io1-f65.google.com ([209.85.166.65]:45249 "EHLO mail-io1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727943AbeLTGaT (ORCPT ); Thu, 20 Dec 2018 01:30:19 -0500 Received: by mail-io1-f65.google.com with SMTP id p7so345482iog.12 for ; Wed, 19 Dec 2018 22:30:18 -0800 (PST) In-Reply-To: <20181220024949.xrgns7nelsxglkhp@ast-mbp.dhcp.thefacebook.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/19/18 6:49 PM, Alexei Starovoitov wrote: > On Sun, Dec 16, 2018 at 03:47:04PM -0800, John Fastabend wrote: >> This adds metadata to sk_msg_md for BPF programs to read the sk_msg >> size. >> >> When the SK_MSG program is running under an application that is using >> sendfile the data is not copied into sk_msg buffers by default. Rather >> the BPF program uses sk_msg_pull_data to read the bytes in. This >> avoids doing the costly memcopy instructions when they are not in >> fact needed. However, if we don't know the size of the sk_msg we >> have to guess if needed bytes are available by doing a pull request >> which may fail. By including the size of the sk_msg BPF programs can >> check the size before issuing sk_msg_pull_data requests. >> >> Additionally, the same applies for sendmsg calls when the application >> provides multiple iovs. Here the BPF program needs to pull in data >> to update data pointers but its not clear where the data ends without >> a size parameter. In many cases "guessing" is not easy to do >> and results in multiple calls to pull and without bounded loops >> everything gets fairly tricky. >> >> Clean this up by including a u32 size field. Note, all writes into >> sk_msg_md are rejected already from sk_msg_is_valid_access so nothing >> additional is needed there. >> >> Signed-off-by: John Fastabend >> --- >> include/linux/skmsg.h | 3 +++ >> include/uapi/linux/bpf.h | 1 + >> net/core/filter.c | 6 ++++++ >> 3 files changed, 10 insertions(+) >> >> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h >> index 2a11e9d..eb8f6cb 100644 >> --- a/include/linux/skmsg.h >> +++ b/include/linux/skmsg.h >> @@ -36,6 +36,9 @@ struct sk_msg_sg { >> struct scatterlist data[MAX_MSG_FRAGS + 1]; >> }; >> >> +/* UAPI in filter.c depends on struct sk_msg_sg being first element. If >> + * this is moved filter.c also must be updated. >> + */ >> struct sk_msg { >> struct sk_msg_sg sg; >> void *data; >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >> index 597afdb..498badc 100644 >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -2608,6 +2608,7 @@ struct sk_msg_md { >> __u32 local_ip6[4]; /* Stored in network byte order */ >> __u32 remote_port; /* Stored in network byte order */ >> __u32 local_port; /* stored in host byte order */ >> + __u32 size; /* Total size of sk_msg */ >> }; >> >> struct sk_reuseport_md { >> diff --git a/net/core/filter.c b/net/core/filter.c >> index bd0df75..55fd237 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -7513,6 +7513,12 @@ static u32 sk_msg_convert_ctx_access(enum bpf_access_type type, >> *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->dst_reg, >> offsetof(struct sock_common, skc_num)); >> break; >> + >> + case offsetof(struct sk_msg_md, size): >> + *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_msg_sg, size), >> + si->dst_reg, si->src_reg, >> + offsetof(struct sk_msg_sg, size)); >> + break; > > John, > > this change broke "test_verifier 129" test. Will need to see why its not failing for me. But thanks for catching. > Now it's failing. > But upon further examination both sk_msg_is_valid_access() > and that test were incorrect. > Here is the bug: > if (off < 0 || off >= sizeof(struct sk_msg_md)) > thanks. Will use bpf_ctx_range() check instead. > sizeof() includes padding to 8 bytes. > So out of bounds access passes sk_msg_is_valid_access(), > but rewritten incorrectly by sk_msg_convert_ctx_access() > and the test is testing for wrong thing. > errstr = "R0 !read_ok" is not the message it should be looking for. typo :( > > After this patch and following adjustment to test_verifier.c > that test is now failing while verifier doing the right thing. > Please submit two patches to fix this: > 1 - fix sk_msg_convert_ctx_access > 2 - fix test_verifier > Creating fix now thanks.