Re: [RFC PATCH bpf-next 09/12] bpf: Add infrastructure to support attaching struct_ops to cgroups

[Martin KaFai Lau <martin.lau@linux.dev>]

On Tue, May 26, 2026 at 10:54:00AM -0700, Amery Hung wrote:
> > > > +static enum cgroup_bpf_attach_type find_atype_by_struct_ops_id(u32 type_id)
> > >
> > > [ ... ]
> > >
> > > >  static void *bpf_cgroup_array_dummy(enum cgroup_bpf_attach_type atype)
> > > >  {
> > > > +   if (cgroup_bpf_is_struct_ops_atype(atype))
> > > > +           return struct_ops_cfi_stubs[atype];
> > > >     return bpf_prog_dummy();
> > > >  }
> > >
> > > [Severity: High]
> > > Can using a loadable module's cfi_stubs as a dummy sentinel cause a
> > > use-after-free when the module is unloaded and reloaded?
> > >
> > > If all maps are detached, the module providing the struct_ops can be unloaded,
> > > and its cfi_stubs memory freed. However, the cgroup's effective array might
> > > still retain the dangling pointer to the freed cfi_stubs if BPF_F_ALLOW_MULTI
> > > was set.
> > >
> > > If the module is reloaded, cgroup_bpf_struct_ops_register() updates
> > > struct_ops_cfi_stubs[atype] with a new pointer. If a user then queries the
> > > cgroup via BPF_PROG_QUERY, the loop checks
> > > item->prog == bpf_cgroup_array_dummy(atype) to skip the dummy. Since the
> > > array contains the old freed pointer, it won't match the new cfi_stubs
> > > pointer, and the query loop might mistakenly treat the old dummy as a valid
> > > map, causing a use-after-free when calling bpf_struct_ops_kdata_map_id().
> >
> > Agree. This will be an issue on BPF_PROG_QUERY when the module reload.
> > I did think about the module reload case but I have only considered the
> > rcu-reader in bpf_cgroup_struct_ops_foreach.
> > BPF_PROG_QUERY does not use rcu_read_lock and depends on cgroup_lock.
> > This will need to be addressed.
> >
> 
> Also thinking of the potential problems of leaving dangling cfi_stubs,
> but I don't see any other cases.
> 
> In this instance, it seems adding a cgroup_bpf_enabled() guard in
> __cgroup_bpf_query should be enough. The dangling cfi_stubs will not
> be cleared until update_effective_progs(), which happens during the
> first struct_ops map attachment. cgroup_bpf_enabled() will be false
> until a struct_ops map is attached.

In __cgroup_struct_ops_link_detach, the static_branch_dec is done without
the cgroup_lock() for now. May be moving it before the cgroup_unlock()
is enough?

__cgroup_bpf_query() has already held cgroup_lock(). I was thinking to
use cgroup_lock() in cgroup_bpf_struct_ops_register() also but it will
need a cgroup_bpf_struct_ops_"un"register() when the module is in
MODULE_STATE_GOING.

Either way, this should fail early in __cgroup_bpf_query() once detected.