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

[sashiko-bot@kernel.org]

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.

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519215841.2984970-1-martin.lau@linux.dev?part=9