From: Philo Lu <lulie@linux.alibaba.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Daniel Borkmann <daniel@iogearbox.net>,
John Fastabend <john.fastabend@gmail.com>,
Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>, Eddy Z <eddyz87@gmail.com>,
Song Liu <song@kernel.org>,
Yonghong Song <yonghong.song@linux.dev>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
Jiri Olsa <jolsa@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>,
Daniel Rosenberg <drosen@google.com>,
Xuan Zhuo <xuanzhuo@linux.alibaba.com>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() for tp_btf
Date: Thu, 9 May 2024 09:11:12 +0800 [thread overview]
Message-ID: <b8afd57d-1167-4821-91e3-8e3e34cf94c4@linux.alibaba.com> (raw)
In-Reply-To: <07ea0e86-ca28-42e9-9e8f-a4188aef1096@linux.dev>
On 2024/5/9 08:24, Martin KaFai Lau wrote:
> On 5/6/24 4:29 PM, Alexei Starovoitov wrote:
>> On Mon, May 6, 2024 at 2:39 PM Martin KaFai Lau <martin.lau@linux.dev>
>> wrote:
>>>
>>> On 4/30/24 5:18 AM, Philo Lu wrote:
>>>> Making tp_btf able to use bpf_dynptr_from_skb(), which is useful for
>>>> skb
>>>> parsing, especially for non-linear paged skb data. This is achieved by
>>>> adding KF_TRUSTED_ARGS flag to bpf_dynptr_from_skb and registering it
>>>> for TRACING progs. With KF_TRUSTED_ARGS, args from fentry/fexit are
>>>> excluded, so that unsafe progs like fexit/__kfree_skb are not allowed.
>>>>
>>>> We also need the skb dynptr to be read-only in tp_btf. Because
>>>> may_access_direct_pkt_data() returns false by default when checking
>>>> bpf_dynptr_from_skb, there is no need to add BPF_PROG_TYPE_TRACING
>>>> to it
>>>> explicitly.
>>>>
>>>> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
>>>> ---
>>>> net/core/filter.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index 786d792ac816..399492970b8c 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -11990,7 +11990,7 @@ int bpf_dynptr_from_skb_rdonly(struct
>>>> sk_buff *skb, u64 flags,
>>>> }
>>>>
>>>> BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
>>>> -BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
>>>> +BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
>>>
>>> I can see the usefulness of having the same way parsing the header as
>>> the
>>> tc-bpf. However, it implicitly means the skb->data and skb_shinfo are
>>> trusted
>>> also. afaik, it should be as long as skb is not NULL.
>>>
>>> From looking at include/trace/events, there is case that skb is
>>> NULL. e.g.
>>> tcp_send_reset. It is not something new though, e.g. using skb->sk in
>>> the tp_btf
>>> could be bad already. This should be addressed before allowing more
>>> kfunc/helper.
>>
>> Good catch.
>> We need to fix this part first:
>> if (prog_args_trusted(prog))
>> info->reg_type |= PTR_TRUSTED;
>>
>> Brute force fix by adding PTR_MAYBE_NULL is probably overkill.
>> I suspect passing NULL into tracepoint is more of an exception than
>> the rule.
>> Maybe we can use kfunc's "__" suffix approach for tracepoint args?
>> [43947] FUNC_PROTO '(anon)' ret_type_id=0 vlen=4
>> '__data' type_id=10
>> 'sk' type_id=3434
>> 'skb' type_id=2386
>> 'reason' type_id=39860
>> [43948] FUNC '__bpf_trace_tcp_send_reset' type_id=43947 linkage=static
>>
>> Then do:
>> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
>> index 49b5ee091cf6..325e8a31729a 100644
>> --- a/include/trace/events/tcp.h
>> +++ b/include/trace/events/tcp.h
>> @@ -91,7 +91,7 @@ DEFINE_RST_REASON(FN, FN)
>> TRACE_EVENT(tcp_send_reset,
>>
>> TP_PROTO(const struct sock *sk,
>> - const struct sk_buff *skb,
>> + const struct sk_buff *skb__nullable,
>>
>> and detect it in btf_ctx_access().
>
> +1. It is a neat solution. Thanks for the suggestion.
>
> Philo, can you give it a try to fix this in the next re-spin?
Glad to do that. I'll try to implement a "__nullable" suffix for tracepoint.
Thanks.
next prev parent reply other threads:[~2024-05-09 1:11 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 12:18 [PATCH bpf-next 0/2] bpf: Allow skb dynptr for tp_btf Philo Lu
2024-04-30 12:18 ` [PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() " Philo Lu
2024-05-06 21:39 ` Martin KaFai Lau
2024-05-06 23:29 ` Alexei Starovoitov
2024-05-09 0:24 ` Martin KaFai Lau
2024-05-09 1:11 ` Philo Lu [this message]
2024-04-30 12:18 ` [PATCH bpf-next 2/2] selftests/bpf: Expand skb dynptr selftests " Philo Lu
2024-05-06 21:43 ` Martin KaFai Lau
2024-05-07 3:15 ` Philo Lu
2024-05-09 0:22 ` 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=b8afd57d-1167-4821-91e3-8e3e34cf94c4@linux.alibaba.com \
--to=lulie@linux.alibaba.com \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=drosen@google.com \
--cc=eddyz87@gmail.com \
--cc=edumazet@google.com \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=martin.lau@linux.dev \
--cc=mykolal@fb.com \
--cc=pabeni@redhat.com \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=xuanzhuo@linux.alibaba.com \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).