diff mbox series

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

Message ID 20240430121805.104618-2-lulie@linux.alibaba.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series bpf: Allow skb dynptr for tp_btf | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 954 this patch: 954
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: netdev@vger.kernel.org
netdev/build_clang success Errors and warnings before: 938 this patch: 938
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 965 this patch: 965
netdev/checkpatch warning WARNING: line length of 90 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 6 this patch: 6
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc

Commit Message

Philo Lu April 30, 2024, 12:18 p.m. UTC 
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(-)

Comments

Martin KaFai Lau May 6, 2024, 9:39 p.m. UTC | #1 
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.

I would like to hear how others think about it.

pw-bot: cr

>   BTF_KFUNCS_END(bpf_kfunc_check_set_skb)
>   
>   BTF_KFUNCS_START(bpf_kfunc_check_set_xdp)
> @@ -12039,6 +12039,7 @@ static int __init bpf_kfunc_init(void)
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_XMIT, &bpf_kfunc_set_skb);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_SEG6LOCAL, &bpf_kfunc_set_skb);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_NETFILTER, &bpf_kfunc_set_skb);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_kfunc_set_skb);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
>   					       &bpf_kfunc_set_sock_addr);
Alexei Starovoitov May 6, 2024, 11:29 p.m. UTC | #2 
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().
Martin KaFai Lau May 9, 2024, 12:24 a.m. UTC | #3 
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?
Philo Lu May 9, 2024, 1:11 a.m. UTC | #4 
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.
diff mbox series

Patch

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)
 BTF_KFUNCS_END(bpf_kfunc_check_set_skb)
 
 BTF_KFUNCS_START(bpf_kfunc_check_set_xdp)
@@ -12039,6 +12039,7 @@  static int __init bpf_kfunc_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_XMIT, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_SEG6LOCAL, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_NETFILTER, &bpf_kfunc_set_skb);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_kfunc_set_skb);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
 					       &bpf_kfunc_set_sock_addr);