Re: [PATCH v7 bpf-next 06/10] bpf: Add bpf_sock_destroy kfunc

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

On 5/4/23 5:13 PM, Martin KaFai Lau wrote:
> 
> Follow up on the v6 patch-set regarding KF_TRUSTED_ARGS.
> KF_TRUSTED_ARGS is needed here to avoid the cases where a PTR_TO_BTF_ID sk is 
> obtained by following another pointer. eg. getting a sk pointer (may be even 
> NULL) by following another sk pointer. The recent PTR_TRUSTED concept in the 
> verifier can guard this. I tried and the following should do:
> 
> diff --git i/net/core/filter.c w/net/core/filter.c
> index 68b228f3eca6..d82e038da0e3 100644
> --- i/net/core/filter.c
> +++ w/net/core/filter.c
> @@ -11767,7 +11767,7 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
>   __diag_pop()
> 
>   BTF_SET8_START(sock_destroy_kfunc_set)
> -BTF_ID_FLAGS(func, bpf_sock_destroy)
> +BTF_ID_FLAGS(func, bpf_sock_destroy, KF_TRUSTED_ARGS)
>   BTF_SET8_END(sock_destroy_kfunc_set)
> 
>   static int tracing_iter_filter(const struct bpf_prog *prog, u32 kfunc_id)
> diff --git i/net/ipv4/tcp_ipv4.c w/net/ipv4/tcp_ipv4.c
> index 887f83a90d85..a769284e8291 100644
> --- i/net/ipv4/tcp_ipv4.c
> +++ w/net/ipv4/tcp_ipv4.c
> @@ -3354,7 +3354,7 @@ static struct bpf_iter_reg tcp_reg_info = {
>       .ctx_arg_info_size    = 1,
>       .ctx_arg_info        = {
>           { offsetof(struct bpf_iter__tcp, sk_common),
> -          PTR_TO_BTF_ID_OR_NULL },
> +          PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },

Alexei, what do you think about having "PTR_MAYBE_NULL | PTR_TRUSTED" here?
The verifier side looks fine (eg. is_trusted_reg() is taking PTR_MAYBE_NULL into 
consideration). However, it seems this will be the first "PTR_MAYBE_NULL | 
PTR_TRUSTED" use case and not sure if PTR_MAYBE_NULL may conceptually conflict 
with the PTR_TRUSTED idea (like PTR_TRUSTED should not be NULL).

>       },
>       .get_func_proto        = bpf_iter_tcp_get_func_proto,
>       .seq_info        = &tcp_seq_info,
> diff --git i/net/ipv4/udp.c w/net/ipv4/udp.c
> index 746c85f2bb03..945b641b363b 100644
> --- i/net/ipv4/udp.c
> +++ w/net/ipv4/udp.c
> @@ -3646,7 +3646,7 @@ static struct bpf_iter_reg udp_reg_info = {
>       .ctx_arg_info_size    = 1,
>       .ctx_arg_info        = {
>           { offsetof(struct bpf_iter__udp, udp_sk),
> -          PTR_TO_BTF_ID_OR_NULL },
> +          PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },
>       },
>       .seq_info        = &udp_seq_info,
>   };