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?
> 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(+)
> >
>
> [...]
