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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMB2axPoBXnrFE0iRT8rAs8txi0jabcb5ctGoFz85-HyDSpghQ@mail.gmail.com>
Date: Thu, 23 Jan 2025 11:41:22 -0800
From: Amery Hung <ameryhung@...il.com>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, daniel@...earbox.net, 
	andrii@...nel.org, alexei.starovoitov@...il.com, martin.lau@...nel.org, 
	sinquersw@...il.com, toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us, 
	stfomichev@...il.com, ekarani.silvestre@....ufcg.edu.br, 
	yangpeihao@...u.edu.cn, xiyou.wangcong@...il.com, yepeilin.cs@...il.com, 
	amery.hung@...edance.com
Subject: Re: [PATCH bpf-next v2 01/14] bpf: Support getting referenced kptr
 from struct_ops argument

On Thu, Jan 23, 2025 at 1:57 AM Eduard Zingerman <eddyz87@...il.com> wrote:
>
> On Fri, 2024-12-20 at 11:55 -0800, Amery Hung wrote:
> > From: Amery Hung <amery.hung@...edance.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@...edance.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.

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@bytedance.com/

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?
>