Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] bpf: propagate st_ops->init() error in bpf_struct_ops_desc_init() - [Medium] bpf: premature btf_idr publication of vmlinux BTF before btf_apply_deferred() - [Medium] bpf: missing bpf_get_btf_vmlinux() in cgroup_bpf_prog_query() --
commit ee51b32030c948007ccf4ba129182f790083514d Author: Jay Wang <[email protected]> bpf: defer kfunc and struct_ops registrations until first BTF use When CONFIG_DEBUG_INFO_BTF_LAZY is enabled, this commit queues kfunc set, dtor kfunc, and struct_ops registrations made before vmlinux BTF is parsed and replays them synchronously inside bpf_get_btf_vmlinux() before publishing btf_vmlinux. It also triggers vmlinux BTF parsing from btf_get_fd_by_id() and BPF_BTF_GET_NEXT_ID so kernel BTF IDs are allocated before IDR lookups. > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index a7bb780743fb2..a5b1cd63a1e43 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6930,17 +6930,54 @@ struct btf *btf_parse_vmlinux(void) > } > > static DEFINE_MUTEX(btf_vmlinux_lock); > +/* CONFIG_DEBUG_INFO_BTF_LAZY: the task applying deferred work, and its BTF > */ > +static struct task_struct *btf_vmlinux_builder; > +static struct btf *btf_vmlinux_new; > +static int btf_apply_deferred(void); > +static void btf_drop_all_deferred(int err); > + > +static bool btf_vmlinux_building(void) > +{ > + return in_task() && READ_ONCE(btf_vmlinux_builder) == current; > +} > > struct btf *bpf_get_btf_vmlinux(void) > { > /* Pairs with the smp_store_release() on the parse path below. */ > struct btf *btf = smp_load_acquire(&btf_vmlinux); > > - if (!btf && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) { > - mutex_lock(&btf_vmlinux_lock); > - btf = btf_vmlinux; > - if (!btf) { > - btf = btf_parse_vmlinux(); > + /* with CONFIG_DEBUG_INFO_BTF_LAZY any call may be the one that parses > */ > + if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF_LAZY)) > + might_sleep(); > + if (btf || !IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) > + return btf; > + /* the registrations btf_apply_deferred() applies see the new BTF */ > + if (btf_vmlinux_building()) > + return btf_vmlinux_new; > + > + mutex_lock(&btf_vmlinux_lock); > + btf = btf_vmlinux; > + if (!btf) { > + btf = btf_parse_vmlinux(); > + if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF_LAZY) && !IS_ERR(btf)) { > + int err; > + > + btf_vmlinux_new = btf; > + WRITE_ONCE(btf_vmlinux_builder, current); > + err = btf_apply_deferred(); > + WRITE_ONCE(btf_vmlinux_builder, NULL); > + btf_vmlinux_new = NULL; > + if (err) { > + /* nothing saw it: drop it with what was > applied to it */ > + bpf_ctx_convert.t = NULL; > + btf_put(btf); > + btf = ERR_PTR(err); > + } > + } > + /* a lazy parse that ran out of memory is retried by the next > caller */ > + if (!IS_ENABLED(CONFIG_DEBUG_INFO_BTF_LAZY) || btf != > ERR_PTR(-ENOMEM)) { > + if (IS_ERR(btf)) > + btf_drop_all_deferred(PTR_ERR(btf)); > /* > * Order the parsed BTF contents and the globals the > * parse populated (e.g. bpf_ctx_convert.t) before > @@ -6949,8 +6986,8 @@ struct btf *bpf_get_btf_vmlinux(void) > */ > smp_store_release(&btf_vmlinux, btf); > } > - mutex_unlock(&btf_vmlinux_lock); > } > + mutex_unlock(&btf_vmlinux_lock); > return btf; > } > > @@ -9302,6 +9339,10 @@ int btf_get_fd_by_id(u32 id) > struct btf *btf; > int fd; > > + /* kernel BTFs get their ids while parsed, before they are published */ > + if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF_LAZY)) > + bpf_get_btf_vmlinux(); > + > rcu_read_lock(); > btf = idr_find(&btf_idr, id); > if (!btf || !refcount_inc_not_zero(&btf->refcnt)) [Severity: Medium] Can a concurrent caller of btf_get_fd_by_id() or __sys_bpf() (for BPF_BTF_GET_NEXT_ID) observe and pin btf_vmlinux_new via btf_idr while btf_apply_deferred() is still mutating it or rolling it back on -ENOMEM? In kernel/bpf/btf.c, btf_parse_vmlinux() allocates the IDR entry before btf_apply_deferred() runs: btf_parse_vmlinux() err = btf_alloc_id(btf); Both btf_get_fd_by_id() and the BPF_BTF_GET_NEXT_ID case in __sys_bpf() call bpf_get_btf_vmlinux() and ignore its return value before querying btf_idr without holding btf_vmlinux_lock. If task A calls btf_get_fd_by_id() and its bpf_get_btf_vmlinux() attempt fails with ERR_PTR(-ENOMEM) (for example, if task A is killed or hits a memory limit during vmalloc()), bpf_get_btf_vmlinux() leaves btf_vmlinux as NULL and releases btf_vmlinux_lock. Because btf_get_fd_by_id() ignores the error, task A proceeds to idr_find(&btf_idr, id). If task B concurrently acquires btf_vmlinux_lock, runs btf_parse_vmlinux() (publishing btf_vmlinux_new into btf_idr via btf_alloc_id()), and enters btf_apply_deferred(), task A can find task B's unpublished btf_vmlinux_new in btf_idr and increment btf->refcnt from 1 to 2 via refcount_inc_not_zero(). This exposes btf_vmlinux_new to userspace while btf_apply_deferred() is still mutating kfunc_set_tab, dtor_kfunc_tab, and struct_ops_tab via krealloc() and sort(). Furthermore, if task B's btf_apply_deferred() then fails with -ENOMEM, the rollback path in bpf_get_btf_vmlinux() assumes nothing else saw btf: if (err) { /* nothing saw it: drop it with what was applied to it */ bpf_ctx_convert.t = NULL; btf_put(btf); btf = ERR_PTR(err); } Because task A holds an extra reference, btf_put(btf) will not drop btf->refcnt to zero or call btf_free_id(btf), leaving the aborted vmlinux BTF in btf_idr while a subsequent caller parses and allocates a second vmlinux BTF. [ ... ] > @@ -11009,6 +11131,9 @@ int __register_bpf_struct_ops(struct bpf_struct_ops > *st_ops) > struct btf *btf; > int err = 0; > > + if (btf_defer_reg(st_ops->owner, BTF_DEFER_ST_OPS, 0, st_ops, 0, &err)) > + return err; > + > btf = btf_get_module_btf(st_ops->owner); > if (!btf) > return check_btf_kconfigs(st_ops->owner, "struct_ops"); [Severity: Medium] When __register_bpf_struct_ops() is deferred until the first bpf_get_btf_vmlinux() call, could BPF_PROG_QUERY with attach_type BPF_STRUCT_OPS fail with -ENOENT if invoked before vmlinux BTF is parsed? Normally, built-in cgroup struct_ops (such as bpf_tcp_ops registered in __bpf_tcp_ops_init()) are registered during initcalls via: __register_bpf_struct_ops() btf_add_struct_ops() cgroup_bpf_struct_ops_register() which populates cgroup_struct_ops[cgroup_atype].type_id. With CONFIG_DEBUG_INFO_BTF_LAZY enabled, btf_defer_reg() defers __register_bpf_struct_ops(), leaving cgroup_struct_ops[].type_id as 0 until bpf_get_btf_vmlinux() runs btf_apply_deferred(). If a user reads the struct_ops type_id from /sys/kernel/btf/vmlinux (which decompresses raw BTF via btf_vmlinux_data() without calling bpf_get_btf_vmlinux()) and invokes BPF_PROG_QUERY with attach_type BPF_STRUCT_OPS, execution reaches kernel/bpf/cgroup.c: bpf_prog_query() cgroup_bpf_prog_query() cgroup_bpf_query() __cgroup_bpf_query() 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; Because neither cgroup_bpf_prog_query() nor __cgroup_bpf_query() calls bpf_get_btf_vmlinux(), the deferred struct_ops registrations have not been applied yet, so find_atype_by_struct_ops_id() returns CGROUP_BPF_ATTACH_TYPE_INVALID and the query fails with -ENOENT instead of succeeding with prog_cnt = 0. > @@ -11034,6 +11159,93 @@ int __register_bpf_struct_ops(struct bpf_struct_ops > *st_ops) > EXPORT_SYMBOL_GPL(__register_bpf_struct_ops); > #endif > > +static void btf_free_deferred_reg(struct btf_deferred_reg *reg) > +{ > + list_del(®->list); > + if (reg->kind != BTF_DEFER_ST_OPS) > + kfree(reg->data); > + kfree(reg); > +} > + > +/* > + * Called under btf_vmlinux_lock, by the task that parsed the vmlinux BTF and > + * before it publishes it, so that nothing sees the BTF without its kfuncs > + * and struct_ops. bpf_get_btf_vmlinux() returns the new BTF to > + * this task, so the registrations are applied as if they were made now, and > + * any they make in turn (a struct_ops ->init() may register kfuncs) too. > + * They run in whichever task needs the BTF first, with the locks it holds: > + * they must not load modules or take locks that users of the BTF hold. > + * > + * Returns -ENOMEM if memory ran out, which may be only because that task is > + * being killed: the caller then drops the new BTF with whatever was applied > + * to it, and the next caller starts over with the queue intact, as for the > + * vmlinux BTF itself; a struct_ops ->init() then runs again with the BTF of > + * that round. Any other error of a registration is only logged. > + */ > +static int btf_apply_deferred(void) > +{ > + struct btf_deferred_reg *reg, *tmp; > + int err; > + > + list_for_each_entry(reg, &btf_deferred_regs, list) { > + /* a going module's exit() runs before GOING drops its entries > */ > + if (!try_module_get(reg->owner)) > + continue; > + err = 0; > + switch (reg->kind) { > + case BTF_DEFER_KFUNCS: > + err = __register_btf_kfunc_id_set(reg->hook, reg->data); > + break; > + case BTF_DEFER_DTORS: > + err = register_btf_id_dtor_kfuncs(reg->data, reg->cnt, > reg->owner); > + break; > + case BTF_DEFER_ST_OPS: > +#if defined(CONFIG_BPF_JIT) > + err = __register_bpf_struct_ops(reg->data); > +#endif > + break; > + } > + module_put(reg->owner); > + if (err == -ENOMEM) > + return err; [Severity: Medium] What happens if a struct_ops ->init() callback fails with -ENOMEM while registering kfuncs during btf_apply_deferred()? In-tree struct_ops ->init() callbacks such as bm_bpf_init() in fs/binfmt_misc_bpf.c and bpf_io_init() in io_uring/bpf-ops.c call register_btf_kfunc_id_set() and return its error code. When btf_apply_deferred() replays BTF_DEFER_ST_OPS while btf_vmlinux_building() is true, register_btf_kfunc_id_set() runs directly against btf_vmlinux_new via btf_populate_kfunc_set(). If memory allocation fails with -ENOMEM in btf_populate_kfunc_set(), it jumps to label end and frees the entire kfunc_set_tab on btf_vmlinux_new: btf_populate_kfunc_set() ... end: btf_free_kfunc_set_tab(btf); return ret; However, in kernel/bpf/bpf_struct_ops.c, bpf_struct_ops_desc_init() discards the -ENOMEM return value from st_ops->init(btf) and replaces it with -EINVAL: if (st_ops->init(btf)) { pr_warn("Error in init bpf_struct_ops %s\n", st_ops->name); err = -EINVAL; goto errout; } As a result, __register_bpf_struct_ops() returns -EINVAL instead of -ENOMEM to btf_apply_deferred(). Because btf_apply_deferred() only aborts the round when err == -ENOMEM, wouldn't it log a warning, free all queued entries in btf_deferred_regs, and return 0, causing bpf_get_btf_vmlinux() to permanently publish a btf_vmlinux whose kfunc_set_tab was wiped out by btf_free_kfunc_set_tab()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
