On Fri, 2024-12-20 at 11:55 -0800, Amery Hung wrote:
> From: Amery Hung <amery.hung@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
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?
