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 <john.fastab...@gmail.com>
>> ---
>> 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.
