On Thu, Jan 23, 2025 at 1:57 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
This is very similar to what v1 is doing but I missed the first part.
Replacing assignments of prog->aux->ctx_arg_info with deep copy should
make ctx_arg_info a per-program thing. I agree with you that creating
this assumption of ref_obj_id is unnecessary and I will change it.
https://lore.kernel.org/bpf/20241213232958.2388301-1-amery.hung@xxxxxxxxxxxxx/
Thanks,
Amery
>
> 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?
>
