Re: [PATCH bpf-next v4 09/15] bpf: Add infrastructure to support attaching struct_ops to cgroups
From: bot+bpf-ci@kernel.org
Date: 2026-09-17 21:34:11
Also in:
bpf
quoted hunk ↗ jump to hunk
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 6ae70ef8e23d1..e27bcc86fc988 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c@@ -20,6 +20,7 @@ #include <linux/bpf.h> #include <linux/bpf_local_storage.h> #include <linux/bpf_verifier.h> +#include <linux/bpf-cgroup.h> #include <linux/module.h> #include <linux/list.h> #include <linux/seq_file.h>
[ ... ]
quoted hunk ↗ jump to hunk
@@ -10024,6 +10028,31 @@ static int btf_add_struct_ops(struct btf *btf, int btf_id, cgroup_atype = st_ops->cgroup_atype; if (cgroup_bpf_is_struct_ops_atype(cgroup_atype)) { + /* + * Cgroup struct_ops callers hold the RCU read lock around the + * entire trampoline call, including its trailing instructions. + * A regular RCU grace period therefore protects both the kdata + * and the trampoline image, so a tasks RCU grace period is not + * needed. + */
This isn't a bug, but the comment reads as though it applies to the entire guard below, when it only explains why free_after_tasks_rcu_gp is rejected. It says nothing about why an unrecognized cgroup_atype or a struct_ops supplying ->reg/->unreg is also rejected, which are the clauses a reader is most likely to encounter first. Would it make sense to either move the comment down to sit directly above the free_after_tasks_rcu_gp clause, or extend it to cover why reg/unreg/cgroup_atype are constrained for cgroup-attached struct_ops?
+ if (!cgroup_bpf_is_struct_ops_atype(cgroup_atype) ||
+ st_ops->reg || st_ops->unreg || st_ops->free_after_tasks_rcu_gp) {
+ pr_warn("struct_ops %s: cgroup_atype invalid or has reg/unreg/free_after_tasks_rcu_gp\n",
+ st_ops->name);
+ err = -EINVAL;
+ goto errout_release;
+ }[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 213570500b681..c0a4301e804dc 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c@@ -24,6 +24,33 @@ DEFINE_STATIC_KEY_ARRAY_FALSE(cgroup_bpf_enabled_key, MAX_CGROUP_BPF_ATTACH_TYPE); EXPORT_SYMBOL(cgroup_bpf_enabled_key); +struct cgroup_struct_ops { + u32 type_id; + void *cfi_stubs; + bool mult_rcu; +}; + +static struct cgroup_struct_ops cgroup_struct_ops[MAX_CGROUP_BPF_ATTACH_TYPE]; + +void cgroup_bpf_struct_ops_register(int atype, u32 type_id, void *cfi_stubs, bool mult_rcu) +{ + cgroup_struct_ops[atype].type_id = type_id; + cgroup_struct_ops[atype].cfi_stubs = cfi_stubs; + cgroup_struct_ops[atype].mult_rcu = mult_rcu; +} + +static enum cgroup_bpf_attach_type +find_atype_by_struct_ops_id(u32 type_id) +{ + enum cgroup_bpf_attach_type atype; + + for (atype = 0; atype < MAX_CGROUP_BPF_ATTACH_TYPE; atype++) { + if (cgroup_bpf_is_struct_ops_atype(atype) && + cgroup_struct_ops[atype].type_id == type_id) + return atype; + } + return CGROUP_BPF_ATTACH_TYPE_INVALID; +}
Can find_atype_by_struct_ops_id() distinguish between 'not registered' and
'type_id is 0'?
cgroup_struct_ops[] is static and zero-initialized, so until
cgroup_bpf_struct_ops_register() runs for a given atype,
cgroup_struct_ops[atype].type_id stays 0. The predicate
'cgroup_bpf_is_struct_ops_atype(atype)' does not check registration state
- it only tests whether atype is recognized (e.g., CGROUP_TCP_SOCK_OPS).
So a user-supplied type_id of 0 would match an unregistered slot.
This is fully reachable via bpf(BPF_PROG_QUERY, {attach_type=BPF_STRUCT_OPS,
type_id=0}) with CAP_BPF/CAP_NET_ADMIN. In __cgroup_bpf_query():
if (type == BPF_STRUCT_OPS) {
u32 type_id = attr->query.type_id;
atype = find_atype_by_struct_ops_id(type_id);
if (atype == CGROUP_BPF_ATTACH_TYPE_INVALID)
return -ENOENT;
Nothing validates attr->query.type_id == 0, so the query would return
success with empty results instead of -ENOENT.
Impact is confined to the returned errno - no out-of-bounds access since
cgroup_bpf_enabled(CGROUP_TCP_SOCK_OPS) is false in that state and
__cgroup_bpf_query() jumps to skip_count without walking any arrays.
Would it make sense to either require a non-zero type_id explicitly in
find_atype_by_struct_ops_id(), or record a separate 'registered' flag, so
the sentinel stays distinguishable from a valid key?
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1363,7 +1440,17 @@ int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr, int ret, from_atype, to_atype; u32 flags; + if (type == BPF_STRUCT_OPS) { + u32 type_id = attr->query.type_id; + + atype = find_atype_by_struct_ops_id(type_id); + if (atype == CGROUP_BPF_ATTACH_TYPE_INVALID) + return -ENOENT; + flags = 0; + if (!cgroup_bpf_enabled(atype)) + goto skip_count;
A subsystem pattern flags this as potentially concerning: cgroup_bpf_enabled(atype) expands to static_branch_unlikely(&cgroup_bpf_enabled_key[atype]), which with CONFIG_JUMP_LABEL=y reaches arch_static_branch() that requires a compile-time-constant key address via an asm "i" constraint. This is the first call site where atype is a runtime variable. Every pre-existing user (BPF_CGROUP_RUN_SK_PROG, BPF_CGROUP_RUN_SA_PROG, and the sites in net/ipv4/tcp.c, net/ipv4/udp.c, net/ipv6/udp.c) passes a constant enumerator, and those compile cleanly. Here the compiler can constant-fold atype only because find_atype_by_struct_ops_id() is static with one call site (so it inlines), its only non-INVALID return is guarded by cgroup_bpf_is_struct_ops_atype(atype) (i.e., atype == CGROUP_TCP_SOCK_OPS), which lets VRP prove atype == CGROUP_TCP_SOCK_OPS after the early return. Once a second cgroup struct_ops atype is added, the fold becomes impossible and this line would fail at build time with "impossible constraint in 'asm'". Would it be worth using a plain static_key_enabled() or an array-safe variant here, so the code does not depend on an optimizer transform that breaks when the recognized atype set grows? [ ... ] --- AI reviewed your patch. Please fix the bug or email reply why it's not a bug. See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35271288839