Re: [PATCH bpf-next v2 05/15] bpf: add bpf_skc_to_tcp6_sock() helper

[Martin KaFai Lau <kafai@fb.com>]

On Sat, Jun 20, 2020 at 10:55:05PM -0700, Yonghong Song wrote:
> The helper is used in tracing programs to cast a socket
> pointer to a tcp6_sock pointer.
> The return value could be NULL if the casting is illegal.
> 
> A new helper return type RET_PTR_TO_BTF_ID_OR_NULL is added
> so the verifier is able to deduce proper return types for the helper.
> 
> Different from the previous BTF_ID based helpers,
> the bpf_skc_to_tcp6_sock() argument can be several possible
> btf_ids. More specifically, all possible socket data structures
> with sock_common appearing in the first in the memory layout.
> This patch only added socket types related to tcp and udp.
> 
> All possible argument btf_id and return value btf_id
> for helper bpf_skc_to_tcp6_sock() are pre-calculcated and
> cached. In the future, it is even possible to precompute
> these btf_id's at kernel build time.
> 

[ ... ]

> @@ -4600,7 +4609,7 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>  	struct bpf_reg_state *regs;
>  	struct bpf_call_arg_meta meta;
>  	bool changes_data;
> -	int i, err;
> +	int i, err, ret_btf_id;
Nit.
Try to keep the rev xmas tree.
or just move "int ret_btf_id;" to the latter "else if (fn->ret_type...)"

>  
>  	/* find function prototype */
>  	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
> @@ -4644,10 +4653,12 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>  	meta.func_id = func_id;
>  	/* check args */
>  	for (i = 0; i < 5; i++) {
> -		err = btf_resolve_helper_id(&env->log, fn, i);
> -		if (err > 0)
> -			meta.btf_id = err;
> -		err = check_func_arg(env, BPF_REG_1 + i, fn->arg_type[i], &meta);
> +		if (!fn->check_btf_id) {
> +			err = btf_resolve_helper_id(&env->log, fn, i);
> +			if (err > 0)
> +				meta.btf_id = err;
> +		}
> +		err = check_func_arg(env, i, &meta, fn);
>  		if (err)
>  			return err;
>  	}
> @@ -4750,6 +4761,16 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>  		regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
>  		regs[BPF_REG_0].id = ++env->id_gen;
>  		regs[BPF_REG_0].mem_size = meta.mem_size;
> +	} else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
> +		mark_reg_known_zero(env, regs, BPF_REG_0);
> +		regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
> +		ret_btf_id = *fn->ret_btf_id;
> +		if (ret_btf_id == 0) {
> +			verbose(env, "invalid return type %d of func %s#%d\n",
> +				fn->ret_type, func_id_name(func_id), func_id);
> +			return -EINVAL;
> +		}
> +		regs[BPF_REG_0].btf_id = ret_btf_id;
>  	} else {
>  		verbose(env, "unknown return type %d of func %s#%d\n",
>  			fn->ret_type, func_id_name(func_id), func_id);
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index afaec7e082d9..478c10d1ec33 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1515,6 +1515,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_skb_output_proto;
>  	case BPF_FUNC_xdp_output:
>  		return &bpf_xdp_output_proto;
> +	case BPF_FUNC_skc_to_tcp6_sock:
> +		return &bpf_skc_to_tcp6_sock_proto;
>  #endif
>  	case BPF_FUNC_seq_printf:
>  		return prog->expected_attach_type == BPF_TRACE_ITER ?
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 73395384afe2..8ca365c5bd10 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -47,6 +47,7 @@
>  #include <linux/seccomp.h>
>  #include <linux/if_vlan.h>
>  #include <linux/bpf.h>
> +#include <linux/btf.h>
>  #include <net/sch_generic.h>
>  #include <net/cls_cgroup.h>
>  #include <net/dst_metadata.h>
> @@ -9191,3 +9192,82 @@ void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog)
>  {
>  	bpf_dispatcher_change_prog(BPF_DISPATCHER_PTR(xdp), prev_prog, prog);
>  }
> +
> +/* Define a list of socket types which can be the argument for
> + * skc_to_*_sock() helpers. All these sockets should have
> + * sock_common as the first argument in its memory layout.
> + */
> +#define BPF_SOCK_CAST_TYPES \
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_INET_CONN_SOCK, "inet_connection_sock")	\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_INET_REQ_SOCK, "inet_request_sock")	\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_INET_SOCK, "inet_sock")			\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_INET_TW_SOCK, "inet_timewait_sock")	\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_REQ_SOCK, "request_sock")			\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_SOCK, "sock")				\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_SOCK_COMMON, "sock_common")		\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_TCP_SOCK, "tcp_sock")			\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_TCP_REQ_SOCK, "tcp_request_sock")		\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_TCP_TW_SOCK, "tcp_timewait_sock")		\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_TCP6_SOCK, "tcp6_sock")			\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_UDP_SOCK, "udp_sock")			\
> +	BPF_SOCK_CAST_TYPE(SOCK_CAST_UDP6_SOCK, "udp6_sock")
> +
> +enum {
> +#define BPF_SOCK_CAST_TYPE(name, str) name,
> +BPF_SOCK_CAST_TYPES
> +MAX_SOCK_CAST_TYPE,
> +#undef BPF_SOCK_CAST_TYPE
> +};
> +
> +static const char *sock_cast_types[] = {
> +#define BPF_SOCK_CAST_TYPE(name, str) str,
> +BPF_SOCK_CAST_TYPES
> +#undef BPF_SOCK_CAST_TYPE
> +};
> +
> +static int sock_cast_btf_ids[MAX_SOCK_CAST_TYPE];
Thanks for doing this.

I think they can be reused outside of casting in the future, e.g. bpf_tcp_ca.c.
If you respin, do you mind to remove the "_cast_" and "_CAST_".
May be naming it to BTF_SOCK_TYPE_xxx, btf_sock_ids?

[ ... ]

> +BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk)
> +{
> +     /* tcp6_sock type is not generated in dwarf and hence btf,
> +      * trigger an explicit type generation here.
> +      */
> +     BTF_TYPE_EMIT(struct tcp6_sock);
> +     if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP &&
> +         sk->sk_family == AF_INET6)
IS_ENABLED(CONFIG_IPV6) just came to my mind.  I think
it should be fine without checking it to keep the #if macro
to minimal.  No sk should be in AF_INET6 in that case.

> +             return (unsigned long)sk;
> +
> +     return (unsigned long)NULL;
> +}
> +
> +> +const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto = {
> +	.func			= bpf_skc_to_tcp6_sock,
> +	.gpl_only		= true,
s/true/false/  With that,

Acked-by: Martin KaFai Lau <kafai@fb.com>

> +	.ret_type		= RET_PTR_TO_BTF_ID_OR_NULL,
> +	.arg1_type		= ARG_PTR_TO_BTF_ID,
> +	.check_btf_id		= check_arg_btf_id,
> +	.ret_btf_id		= &sock_cast_btf_ids[SOCK_CAST_TCP6_SOCK],
> +};