Re: [PATCH bpf-next 1/2] bpf: Allow bpf_dynptr_from_skb() for tp_btf

[Martin KaFai Lau <martin.lau@linux.dev>]

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?