Re: [PATCH bpf-next v3 2/9] bpf: Support __arena and __arena_nullable on struct_ops arguments

On Tue, Aug 4, 2026 at 10:14 PM Eduard Zingerman <[email protected]> wrote:
>
> On Tue, 2026-08-04 at 16:55 -0700, Eduard Zingerman wrote:
> > On Mon, 2026-08-03 at 14:51 +0200, Kumar Kartikeya Dwivedi wrote:
> >
> > Lgtm except for several nits below.
> >
> > ...
> >
> > > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> >
> > ...
> >
> > > @@ -226,26 +229,30 @@ static int prepare_arg_info(struct btf *btf,
> > >     info = info_buf;
> > >     for (arg_no = 0; arg_no < nargs; arg_no++) {
> > >             /* Skip arguments that is not suffixed with
> > > -            * "__nullable or __ref".
> > > +            * "__nullable", "__ref", "__arena" or "__arena_nullable".
> > >              */
> > >             is_nullable = btf_param_match_suffix(btf, &stub_args[arg_no],
> > >                                                  MAYBE_NULL_SUFFIX);
> > >             is_refcounted = btf_param_match_suffix(btf, &stub_args[arg_no],
> > >                                                    REFCOUNTED_SUFFIX);
> > > +           is_arena_nullable = btf_param_match_suffix(btf, &stub_args[arg_no],
> > > +                                                      ARENA_MAYBE_NULL_SUFFIX);
> > > +           is_arena = btf_param_match_suffix(btf, &stub_args[arg_no], ARENA_SUFFIX);
> > >
> > >             if (is_nullable)
> > >                     suffix = MAYBE_NULL_SUFFIX;
> > >             else if (is_refcounted)
> > >                     suffix = REFCOUNTED_SUFFIX;
> > > +           else if (is_arena_nullable)
> > > +                   suffix = ARENA_MAYBE_NULL_SUFFIX;
> > > +           else if (is_arena)
> > > +                   suffix = ARENA_SUFFIX;
> >
> > As discussed already, that's unfortunate that p__arena and p__nullable
> > are valid, but p__arena__nullable is not, substituted by
> > p__arena_nullable. Same goes for global subprogs.
> >
> > >             else
> > >                     continue;
> > >
> > > -           /* Should be a pointer to struct */
> > > -           pointed_type = btf_type_resolve_ptr(btf,
> > > -                                               args[arg_no].type,
> > > -                                               &arg_btf_id);
> > > -           if (!pointed_type ||
> > > -               !btf_type_is_struct(pointed_type)) {
> > > +           /* Should be a pointer to struct, or any pointer for __arena/__arena_nullable */
> > > +           pointed_type = btf_type_resolve_ptr(btf, args[arg_no].type, &arg_btf_id);
> > > +           if (!pointed_type || (!is_arena && !is_arena_nullable && !btf_type_is_struct(pointed_type))) {
> > >                     pr_warn("stub function %s has %s tagging to an unsupported type\n",
> > >                             stub_fname, suffix);
> > >                     goto err_out;
> >
> > ...
> >
> > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > > index ed7999ad6c66..d2d7a2904345 100644
> > > --- a/kernel/bpf/trampoline.c
> > > +++ b/kernel/bpf/trampoline.c
> >
> > ...
> >
> > > @@ -695,6 +743,22 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
> > >             goto out;
> > >     }
> > >
> > > +   /*
> > > +    * Arena ctx args are converted only by the struct_ops indirect
> > > +    * trampoline, which dispatches to a single known prog. Generic
> > > +    * trampolines can mix progs with different arenas, so no conversion
> > > +    * is possible here. Not reachable today: only struct_ops progs get
> > > +    * arena ctx args and they never ride generic trampolines.
> > > +    */
> > > +   for (kind = 0; kind < BPF_TRAMP_MAX; kind++) {
> > > +           for (i = 0; i < tnodes[kind].nr_nodes; i++) {
> > > +                   if (bpf_prog_has_arena_ctx_arg(tnodes[kind].nodes[i]->link->prog)) {
> > > +                           err = -ENOTSUPP;
> > > +                           goto out;
> > > +                   }
> > > +           }
> > > +   }
> >
> > Nit: should this be done in __bpf_trampoline_link_prog() or
> >      bpf_trampoline_add_prog()? To avoid doing the same check on unlink.
> >      Since this is an invariant violation, do we want to add WARN_ONCE?
> >
> > > +
> > >     /* clear all bits except SHARE_IPMODIFY and TAIL_CALL_CTX */
> > >     tr->flags &= (BPF_TRAMP_F_SHARE_IPMODIFY | BPF_TRAMP_F_TAIL_CALL_CTX);
> > >
> >
> > ...
>
> Thinking a bit more about this. Since kfuncs already use flags in
> btf_func_model, I think it would be nice to use them for struct_ops as
> well. E.g. as in the patch attached (applied on top of this series).
> Wdyt?

I like that it relies on existing annotations instead of yet another
auxiliary field.

The mapping for arena args to different function models can be confusing though.

For the verfication purpose: both __arena and __arena_nullable maps to
KF_ARG_PTR_TO_ARENA | PTR_MAYBE_NULL.
For the JIT purpose: __arena maps to BTF_FMODEL_ARENA_ARG;
__arena_nullable maps to BTF_FMODEL_ARENA_ARG |
BTF_FMODEL_NULLABLE_ARG.

It might deserve a brief inline comment in get_kfunc_arg_type() saying
why both tags map to KF_ARG_PTR_TO_ARENA | PTR_MAYBE_NULL.