Re: [PATCH bpf-next 05/13] bpf: abstract away global subprog arg preparation logic from reg state setup

[Eduard Zingerman <eddyz87@xxxxxxxxx>]

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

Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>

[...]
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 379ac0a28405..c3a5d0fe3cdf 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -704,6 +704,7 @@ enum bpf_arg_type {
>  
>  	ARG_PTR_TO_CTX,		/* pointer to context */
>  	ARG_ANYTHING,		/* any (initialized) argument is ok */
> +	ARG_SCALAR = ARG_ANYTHING, /* scalar value */

Nit: I agree that use of ARG_ANYTHING to denote scalars is confusing,
     but having two names for the same thing seems even more confusing.

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index d56433bf8aba..33a62df9c5a8 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6955,9 +6955,9 @@ int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog,
>   * 0 - Successfully converted BTF into bpf_reg_state
>   * (either PTR_TO_CTX or SCALAR_VALUE).
>   */
> -int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
> -			  struct bpf_reg_state *regs, u32 *arg_cnt)
> +int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)

Could you please also update the comment above this function?
It currently says: "Convert BTF of a function into bpf_reg_state if possible".

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ee707736ce6b..16d5550eda4d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[...]
> @@ -19860,33 +19855,44 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
[...]
>  		for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> -			if (regs[i].type == PTR_TO_CTX)
> +			arg = &sub->args[i - BPF_REG_1];
> +			reg = &regs[i];
> +
> +			if (arg->arg_type == ARG_PTR_TO_CTX) {
> +				reg->type = PTR_TO_CTX;
>  				mark_reg_known_zero(env, regs, i);
> -			else if (regs[i].type == SCALAR_VALUE)
> +			} else if (arg->arg_type == ARG_SCALAR) {
> +				reg->type = SCALAR_VALUE;
>  				mark_reg_unknown(env, regs, i);
> -			else if (base_type(regs[i].type) == PTR_TO_MEM) {
> -				const u32 mem_size = regs[i].mem_size;
> -
> +			} else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
> +				reg->type = PTR_TO_MEM;
> +				if (arg->arg_type & PTR_MAYBE_NULL)
> +					reg->type |= PTR_MAYBE_NULL;
>  				mark_reg_known_zero(env, regs, i);
> -				regs[i].mem_size = mem_size;
> -				regs[i].id = ++env->id_gen;
> +				reg->mem_size = arg->mem_size;
> +				reg->id = ++env->id_gen;
>  			}

Nit: maybe add an else branch here and report an error if unexpected
     argument type is returned by btf_prepare_func_args()?