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

On Wed Aug 5, 2026 at 7:31 PM CEST, Amery Hung wrote:
> 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.

Yeah I will add comments for this. I adopted Eduard's diff (w/ his SoB) on top
of the stack.