On Wed, Jun 18, 2025 at 3:19 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
>
> On Thu, Jun 12, 2025 at 4:08 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, Jun 9, 2025 at 4:27 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> > >
> > > Allows struct_ops implementors to infer the calling struct_ops instance
> > > inside a kfunc through prog->aux->this_st_ops. A new field, flags, is
> > > added to bpf_struct_ops. If BPF_STRUCT_OPS_F_THIS_PTR is set in flags,
> > > a pointer to the struct_ops structure registered to the kernel (i.e.,
> > > kvalue->data) will be saved to prog->aux->this_st_ops. To access it in
> > > a kfunc, use BPF_STRUCT_OPS_F_THIS_PTR with __prog argument [0]. The
> > > verifier will fixup the argument with a pointer to prog->aux. this_st_ops
> > > is protected by rcu and is valid until a struct_ops map is unregistered
> > > updated.
> > >
> > > For a struct_ops map with BPF_STRUCT_OPS_F_THIS_PTR, to make sure all
> > > programs in it have the same this_st_ops, cmpxchg is used. Only if a
> > > program is not already used in another struct_ops map also with
> > > BPF_STRUCT_OPS_F_THIS_PTR can it be assigned to the current struct_ops
> > > map.
> > >
> >
> > Have you considered an alternative to storing this_st_ops in
> > bpf_prog_aux by setting it at runtime (in struct_ops trampoline) into
> > bpf_run_ctx (which I think all struct ops programs have set), and then
> > letting any struct_ops kfunc just access it through current (see other
> > uses of bpf_run_ctx, if you are unfamiliar). This would avoid all this
> > business with extra flags and passing bpf_prog_aux as an extra
> > argument.
> >
>
> I didn't know this. Thanks for suggesting an alternative!
>
> > There will be no "only one struct_ops for this BPF program" limitation
> > either: technically, you could have the same BPF program used from two
> > struct_ops maps just fine (even at the same time). Then depending on
> > which struct_ops is currently active, you'd have a corresponding
> > bpf_run_ctx's struct_ops pointer. It feels like a cleaner approach to
> > me.
> >
>
> This is a cleaner approach for struct_ops operators. To make it work
> for kfuncs called in timer callback, I think prog->aux->st_ops is
> still needed, but at least we can unify how to get this_st_ops in
> kfunc, in a way that does not requires adding __prog to every kfuncs.
>
> +enum bpf_run_ctx_type {
> + BPF_CG_RUN_CTX = 0,
> + BPF_TRACE_RUN_CTX,
> + BPF_TRAMP_RUN_CTX,
> + BPF_TIMER_RUN_CTX,
> +};
>
> struct bpf_run_ctx {
> + enum bpf_run_ctx_type type;
> };
>
> +struct bpf_timer_run_ctx {
> + struct bpf_prog_aux *aux;
> +};
>
> struct bpf_tramp_run_ctx {
> ...
> + void *st_ops;
> };
>
> In bpf_struct_ops_prepare_trampoline(), the st_ops assignment will be
> emitted to the trampoline.
>
> In bpf_timer_cb(), prepare bpf_timer_run_ctx, where st_ops comes from
> prog->aux->this_st_ops and set current->bpf_ctx.
>
> Finally, in kfuncs that want to know the current struct_ops, call this
> new function below:
>
> +void *bpf_struct_ops_current_st_ops(void)
> +{
> + struct bpf_prog_aux aux;
> +
> + if (!current->bpf_ctx)
> + return NULL;
> +
> + switch(current->bpf_ctx->type) {
> + case BPF_TRAMP_RUN_CTX:
> + return (struct bpf_tramp_run_ctx *)(current->bpf_ctx)->st_ops;
> + case BPF_TIMER_RUN_CTX:
> + aux = (struct bpf_timer_run_ctx *)(current->bpf_ctx)->aux;
> + return rcu_dereference(aux->this_st_ops);
> + }
> + return NULL;
> +}
>
> What do you think?
I'm not sure I particularly like different ways to get this st_ops
pointer depending whether we are in an async callback or not. So if
bpf_prog_aux is inevitable, I'd just stick to that. As far as
bpf_run_ctx, though, instead of bpf_run_ctx_type, shall we just put
`struct bpf_prog_aux *` pointer into common struct bpf_run_ctx and
always set it for all program types that support run_ctx?
>
> > And in the trampoline itself it would be a hard-coded single word
> > assignment on the stack, so should be basically a no-op from
> > performance point of view.
> >
> > > [0]
> > > commit bc049387b41f ("bpf: Add support for __prog argument suffix to
> > > pass in prog->aux")
> > > https://lore.kernel.org/r/20250513142812.1021591-1-memxor@xxxxxxxxx
> > >
> > > Signed-off-by: Amery Hung <ameryhung@xxxxxxxxx>
> > > ---
> > > include/linux/bpf.h | 10 ++++++++++
> > > kernel/bpf/bpf_struct_ops.c | 38 +++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 48 insertions(+)
> > >
> >
> > [...]
