Re: [PATCH bpf-next v2 01/14] bpf: Support getting referenced kptr from struct_ops argument

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-12-20 at 11:55 -0800, Amery Hung wrote:
> From: Amery Hung <amery.hung@bytedance.com>
> 
> Allows struct_ops programs to acqurie referenced kptrs from arguments
> by directly reading the argument.
> 
> The verifier will acquire a reference for struct_ops a argument tagged
> with "__ref" in the stub function in the beginning of the main program.
> The user will be able to access the referenced kptr directly by reading
> the context as long as it has not been released by the program.
> 
> This new mechanism to acquire referenced kptr (compared to the existing
> "kfunc with KF_ACQUIRE") is introduced for ergonomic and semantic reasons.
> In the first use case, Qdisc_ops, an skb is passed to .enqueue in the
> first argument. This mechanism provides a natural way for users to get a
> referenced kptr in the .enqueue struct_ops programs and makes sure that a
> qdisc will always enqueue or drop the skb.
> 
> Signed-off-by: Amery Hung <amery.hung@bytedance.com>
> ---

Hi Amery,

Sorry, for joining so late in the review process.
Decided to take a look at verifier related changes.
Overall the patch looks good to me,
but I dislike the part allocating parameter ids.

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 28246c59e12e..c2f4f84e539d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6682,6 +6682,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  			info->reg_type = ctx_arg_info->reg_type;
>  			info->btf = ctx_arg_info->btf ? : btf_vmlinux;
>  			info->btf_id = ctx_arg_info->btf_id;
> +			info->ref_obj_id = ctx_arg_info->refcounted ? ++nr_ref_args : 0;
>  			return true;
>  		}
>  	}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f27274e933e5..26305571e377 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[...]

> @@ -22161,6 +22182,16 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  		mark_reg_known_zero(env, regs, BPF_REG_1);
>  	}
>  
> +	/* Acquire references for struct_ops program arguments tagged with "__ref".
> +	 * These should be the earliest references acquired. btf_ctx_access() will
> +	 * assume the ref_obj_id of the n-th __ref-tagged argument to be n.
> +	 */
> +	if (!subprog && env->prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> +		for (i = 0; i < env->prog->aux->ctx_arg_info_size; i++)
> +			if (env->prog->aux->ctx_arg_info[i].refcounted)
> +				acquire_reference(env, 0);
> +	}
> +
>  	ret = do_check(env);
>  out:
>  	/* check for NULL is necessary, since cur_state can be freed inside

I think it would be cleaner if:
- each program would own it's instance of 'env->prog->aux->ctx_arg_info';
- ref_obj_id field would be added to 'struct bpf_ctx_arg_aux';
- parameter ids would be allocated in do_check_common(), but without
  reliance on being first to allocate.

Or add some rigour to this thing and e.g. make env->id_gen signed
and declare an enum of special ids like:

  enum special_ids {
  	STRUCT_OPS_CTX_PARAM_0 = -1,
  	STRUCT_OPS_CTX_PARAM_1 = -2,
  	STRUCT_OPS_CTX_PARAM_2 = -3,
  	...
  }

and update the loop above as:

	if (!subprog && env->prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
		for (i = 0; i < env->prog->aux->ctx_arg_info_size; i++)
			if (env->prog->aux->ctx_arg_info[i].refcounted)
            	/* imagined function that acquires an id with specific value */
				acquire_special_reference(env, 0, STRUCT_OPS_CTX_PARAM_0 - i /* desired id */);
	}

wdyt?