Re: [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
[...]

> @@ -6845,7 +6845,47 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
>  	 * Only PTR_TO_CTX and SCALAR are supported atm.
>  	 */
>  	for (i = 0; i < nargs; i++) {
> +		bool is_nonnull = false;
> +		const char *tag;
> +
>  		t = btf_type_by_id(btf, args[i].type);
> +
> +		tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");

Nit: this does a linear scan over all BTF type ids for each
     function parameter, which is kind of ugly.

> +		if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
> +			tag = NULL;
> +		} else if (IS_ERR(tag)) {
> +			bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
> +			return PTR_ERR(tag);
> +		}
> +		/* 'arg:<tag>' decl_tag takes precedence over derivation of
> +		 * register type from BTF type itself
> +		 */
> +		if (tag) {
> +			/* disallow arg tags in static subprogs */
> +			if (!is_global) {
> +				bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> +				return -EOPNOTSUPP;
> +			}

Nit: this would be annoying if someone would add/remove 'static' a few
     times while developing BPF program. Are there safety reasons to
     forbid this?

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5787b7fd16ba..61e778dbde10 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9268,9 +9268,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
>  			ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
>  			if (ret < 0)
>  				return ret;
> -
>  			if (check_mem_reg(env, reg, regno, arg->mem_size))
>  				return -EINVAL;
> +			if (!(arg->arg_type & PTR_MAYBE_NULL) && (reg->type & PTR_MAYBE_NULL)) {
> +				bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
> +				return -EINVAL;
> +			}
> +		} else if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
> +			if (reg->type != PTR_TO_PACKET_META) {
> +				bpf_log(log, "arg#%d expected pkt_meta, but got %s\n",
> +					i, reg_type_str(env, reg->type));
> +				return -EINVAL;
> +			}
> +		} else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
> +			if (reg->type != PTR_TO_PACKET) {

I think it is necessary to check that 'reg->umax_value == 0'.
check_packet_access() uses reg->umax_value to bump
env->prog->aux->max_pkt_offset. When body of a global function is
verified it starts with 'umax_value == 0'.
Might be annoying from usability POV, however.

> +				bpf_log(log, "arg#%d expected pkt, but got %s\n",
> +					i, reg_type_str(env, reg->type));
> +				return -EINVAL;
> +			}
> +		} else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
> +			if (reg->type != PTR_TO_PACKET_END) {
> +				bpf_log(log, "arg#%d expected pkt_end, but got %s\n",
> +					i, reg_type_str(env, reg->type));
> +				return -EINVAL;
> +			}
>  		} else {
>  			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
>  				i, arg->arg_type);

[...]