Re: [PATCH bpf-next v3 06/15] bpf: Add prog_list_init_item(), prog_list_replace_item(), and prog_list_id()

On Mon, Jul 13, 2026 at 2:56 PM Emil Tsalapatis <[email protected]> wrote:
>
> On Mon Jul 6, 2026 at 1:19 PM EDT, Amery Hung wrote:
> > From: Martin KaFai Lau <[email protected]>
> >
> > Add three helpers to abstract operations on a bpf_prog_list entry.
> >
> > Right now, bpf_prog_array_item is initialized from prog_list_prog(pl),
> > which returns either pl->prog or pl->link->link.prog. This will not work
> > when struct_ops is attached to a cgroup because the attachment is backed
> > by a struct_ops map instead of a BPF prog.
> >
> > The same applies to __cgroup_bpf_query(). Instead of always copying a
> > prog id to userspace, struct_ops cgroup attachment will need to copy the
> > struct_ops map id.
> >
> > Refactor bpf_prog_array_item initialization into prog_list_init_item()
> > and prog_list_replace_item(), and refactor id lookup into prog_list_id().
> > These helpers will be extended to support pl->link->map in a later patch.
> >
> > This is a no-op change.
>
> Reviewed-by: Emil Tsalapatis <[email protected]>
>
> Followup on prog_list_prog() since I hadn't realized it was moved to the
> helpers in this patchset: Maybe we can just make it not return NULL
> since it's impossible instead of adding error handling to minimize
> churn.

Thanks for reviewing this patchset!

You are right that this set avoids calling prog_list_prog() on a
struct_ops entry and so NULL is never reaches the caller.

I'd still rather not drop the NULL return to avoid baking a caller
precondition into this generic helper. Nothing in prog_list_prog()'s
name or signature says "non-struct_ops entries only", so I'd rather
keep it the callers' responsibility to know which entries are backed
by a prog, prog link, struct_ops link, or being detached.

>
> >
> > Signed-off-by: Martin KaFai Lau <[email protected]>
> > Signed-off-by: Amery Hung <[email protected]>
> > ---
> >  kernel/bpf/cgroup.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index b100c04cb9c8..b43f0bff184c 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -399,6 +399,22 @@ static struct bpf_prog *prog_list_prog(struct bpf_prog_list *pl)
> >       return NULL;
> >  }
> >
> > +static void prog_list_init_item(struct bpf_prog_list *pl, struct bpf_prog_array_item *item)
> > +{
> > +     item->prog = prog_list_prog(pl);
> > +     bpf_cgroup_storages_assign(item->cgroup_storage, pl->storage);
> > +}
> > +
> > +static void prog_list_replace_item(struct bpf_prog_list *pl, struct bpf_prog_array_item *item)
> > +{
> > +     WRITE_ONCE(item->prog, pl->link->link.prog);
> > +}
> > +
> > +static u32 prog_list_id(struct bpf_prog_list *pl)
> > +{
> > +     return prog_list_prog(pl)->aux->id;
> > +}
> > +
> >  /* count number of elements in the list.
> >   * it's slow but the list cannot be long
> >   */
> > @@ -492,9 +508,7 @@ static int compute_effective_progs(struct cgroup *cgrp,
> >                               item = &progs->items[fstart];
> >                               fstart++;
> >                       }
> > -                     item->prog = prog_list_prog(pl);
> > -                     bpf_cgroup_storages_assign(item->cgroup_storage,
> > -                                                pl->storage);
> > +                     prog_list_init_item(pl, item);
> >                       cnt++;
> >               }
> >
> > @@ -1015,7 +1029,7 @@ static void replace_effective_prog(struct cgroup *cgrp,
> >                               desc->bpf.effective[atype],
> >                               lockdep_is_held(&cgroup_mutex));
> >               item = &progs->items[pos];
> > -             WRITE_ONCE(item->prog, pl->link->link.prog);
> > +             prog_list_replace_item(pl, item);
> >       }
> >  }
> >
> > @@ -1318,15 +1332,13 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> >               } else {
> >                       struct hlist_head *progs;
> >                       struct bpf_prog_list *pl;
> > -                     struct bpf_prog *prog;
> >                       u32 id;
> >
> >                       progs = &cgrp->bpf.progs[atype];
> >                       cnt = min_t(int, prog_list_length(progs, NULL), total_cnt);
> >                       i = 0;
> >                       hlist_for_each_entry(pl, progs, node) {
> > -                             prog = prog_list_prog(pl);
> > -                             id = prog->aux->id;
> > +                             id = prog_list_id(pl);
> >                               if (copy_to_user(prog_ids + i, &id, sizeof(id)))
> >                                       return -EFAULT;
> >                               if (++i == cnt)
>