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 19, 2026 at 10:50:02PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] bpf_cgroup_struct_ops_foreach does not skip dummy items, leading to unintended execution of cfi_stubs.
> - [High] Using a loadable module's cfi_stubs as a dummy sentinel causes a Use-After-Free when the module is unloaded and reloaded.
> - [Medium] The macro bpf_cgroup_struct_ops_foreach uses rcu_dereference(), which is incompatible with sleepable struct_ops programs.
> - [Medium] cgroup_bpf_struct_ops_register() lacks concurrency protection and global uniqueness enforcement.
> --
> 
> commit bad29e4e0d1357905491993ee45063766d0f7705
> Author: Martin KaFai Lau <martin.lau@kernel.org>
> 
> bpf: Add infrastructure to support attaching struct_ops to cgroups
> 
> This patch adds necessary infrastructure to attach a struct_ops
> map to a cgroup. The initial need was to support migrating
> the legacy BPF_PROG_TYPE_SOCK_OPS to a struct_ops.
> 
> > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> > index b2e79c2b41d51..8080f4a5c14b6 100644
> > --- a/include/linux/bpf-cgroup.h
> > +++ b/include/linux/bpf-cgroup.h
> 
> [ ... ]
> 
> > @@ -110,6 +112,18 @@ struct bpf_prog_list {
> >  	u32 flags;
> >  };
> >  
> > +#define bpf_cgroup_struct_ops_foreach(var, item, cgrp, atype)		\
> > +	for (item = rcu_dereference((cgrp)->bpf.effective[atype])->items;\
> > +	     ((var) = READ_ONCE(item->kdata));				\
> > +	     item++)
> 
> [Severity: High]
> Does this macro need to skip dummy sentinel items?
> 
> In cgroups with BPF_F_ALLOW_MULTI, compute_effective_progs() inserts dummy
> sentinel items at the boundaries between cgroup levels. For struct_ops, this
> dummy is the cfi_stubs pointer.
> 
> This macro appears to iterate over all non-NULL items and fails to skip the
> dummy sentinel, which could cause the subsystem to repeatedly yield the
> cfi_stubs pointer as a valid attached struct_ops instance.

The ops in the cfi_stubs is safe to be called. It intentionally skips
checking for dummy in the fast path.

> 
> [Severity: Medium]
> Will this rcu_dereference() trigger Lockdep warnings for sleepable struct_ops
> programs?
> 
> The commit message mentions adding free_after_mult_rcu_gp to support a mix of
> sleepable and non-sleepable struct_ops, but plain rcu_dereference() enforces
> rcu_read_lock_held().
> 
> If a subsystem iterates the array while holding rcu_read_lock_trace()
> (required for sleepable execution), this might trigger a 'suspicious
> rcu_dereference_check() usage' warning. Conversely, if it holds
> rcu_read_lock(), the struct_ops program cannot sleep without causing a panic.

The proposed bpf_tcp_ops only has non-sleepable programs.

The future sleepable callsite will need another version of foreach or
rcu_dereference_check can be used here also. I think rcu_dereference_check
is better.

> 
> > +
> > +static inline bool cgroup_bpf_is_struct_ops_atype(enum cgroup_bpf_attach_type atype)
> > +{
> 
> [ ... ]
> 
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index a033aa479ab6e..d496db48d2b86 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -24,6 +24,29 @@
> >  DEFINE_STATIC_KEY_ARRAY_FALSE(cgroup_bpf_enabled_key, MAX_CGROUP_BPF_ATTACH_TYPE);
> >  EXPORT_SYMBOL(cgroup_bpf_enabled_key);
> >  
> > +static u32 struct_ops_type_id[MAX_CGROUP_BPF_ATTACH_TYPE];
> > +static void *struct_ops_cfi_stubs[MAX_CGROUP_BPF_ATTACH_TYPE];
> > +static bool struct_ops_mult_rcu[MAX_CGROUP_BPF_ATTACH_TYPE];
> > +
> > +void cgroup_bpf_struct_ops_register(int atype, u32 type_id, void *cfi_stubs, bool mult_rcu)
> > +{
> > +	struct_ops_type_id[atype] = type_id;
> > +	struct_ops_cfi_stubs[atype] = cfi_stubs;
> > +	struct_ops_mult_rcu[atype] = mult_rcu;
> > +}
> 
> [Severity: Medium]
> Does this registration function need concurrency protection to prevent global
> array corruption?
> 
> If two modules are loaded concurrently and declare the same cgroup_atype, it
> seems this could cause a data race or silently overwrite the registered type_id
> and cfi_stubs of a legitimate module.

This will be a bug in the subsystems having the same cgroup_atype

but (the next one)...

> 
> > +
> > +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.