Re: [PATCH bpf-next 10/13] bpf: defer kfunc and struct_ops registrations until first BTF use

[sashiko-bot@kernel.org]

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 <wanjay@amazon.com>

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(&reg->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/20261031130959.23472-1-wanjay@amazon.com?part=10