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

[bot+bpf-ci]

> bpf: defer kfunc and struct_ops registrations until first BTF use

TL;DR:
- btf_get_fd_by_id() may return an fd for a half-built vmlinux BTF after a
  failed -ENOMEM parse round, possibly racing with struct_ops table updates
- a permanent -ENOMEM may be retried forever, with the queue never dropped
- Co-developed-by lacks a Signed-off-by

> bpf: defer kfunc and struct_ops registrations until first BTF use
>
> [ ... ]
>
> Co-developed-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx>
> TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx>
> Signed-off-by: Jay Wang <wanjay@xxxxxxxxxx>

The Co-developed-by: tag is followed by a "TODO-NEEDS-IHOR: Signed-off-by:"
placeholder instead of a real Signed-off-by: line from the co-developer.

Should the placeholder be resolved, or the Co-developed-by: tag dropped,
before this is sent?  Documentation/process/submitting-patches.rst expects a
Co-developed-by: to be followed immediately by that co-author's
Signed-off-by:.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c

[ ... ]

> @@ -6928,33 +6928,70 @@ struct btf *btf_parse_vmlinux(void)
>  struct btf *bpf_get_btf_vmlinux(void)
>  {

[ ... ]

> +	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)) {

[ ... ]

> +		/* 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
>  			 * the pointer publication. Pairs with the acquire
>  			 * on the lockless fast path above.
>  			 */
>  			smp_store_release(&btf_vmlinux, btf);
>  		}

Is every -ENOMEM from btf_parse_vmlinux() really transient here?

btf_vmlinux_data() in kernel/bpf/sysfs_btf.c, from the previous patch in the
series, also returns -ENOMEM for a permanent failure.  Its comment says
"ERR_PTR(-ENOMEM) if that cannot be done (now: memory; for good: no copy of
the compressed BTF)".  If the vmalloc() in btf_vmlinux_keep_zst() fails,
btf_zst becomes NULL and every later call ends up at:

	if (!btf_zst)
		return ERR_PTR(btf_vmlinux_broken ? -EINVAL : -ENOMEM);

>From then on bpf_get_btf_vmlinux() never reaches btf_drop_all_deferred() and
never publishes btf_vmlinux.  Every queued built-in registration and its
kmemdup() copy is kept, no "dropped N deferred BTF registrations" warning is
printed, and each later caller (verifier, BPF_BTF_GET_FD_BY_ID, the
CAP_SYS_ADMIN BPF_BTF_GET_NEXT_ID, module BTF parse) takes btf_vmlinux_lock
again to re-run a parse that cannot succeed.

This also looks different from the commit message, which says "If the vmlinux
BTF fails for good, the queue is dropped with a warning."

Should btf_vmlinux_data() return a different errno for the permanent case, or
should this retry test be able to tell the two cases apart?

[ ... ]

> @@ -9300,12 +9337,16 @@ int btf_get_info_by_fd(const struct btf *btf,
>  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))
>  		btf = ERR_PTR(-ENOENT);
>  	rcu_read_unlock();

Can this hand out an fd for a vmlinux BTF that is still being built?

The return value of bpf_get_btf_vmlinux() is dropped here.  If this task's
own parse round fails with -ENOMEM (the commit message names the case where
the task is being killed and vmalloc() gives up), btf_vmlinux stays NULL and
btf_vmlinux_lock is released.  Another task can then take the lock and start
a new round.

btf_parse_vmlinux() gives that new BTF an id in btf_idr via btf_alloc_id()
before btf_apply_deferred() runs, so the idr_find() plus
refcount_inc_not_zero() above can return it while its kfunc and struct_ops
tables are still being filled in under btf_vmlinux_lock.

That seems to conflict with the commit message ("Nobody else sees btf_vmlinux
before its kfuncs and struct_ops are in"), and with the assumption in the
drop path of bpf_get_btf_vmlinux():

	/* nothing saw it: drop it with what was applied to it */
	bpf_ctx_convert.t = NULL;
	btf_put(btf);

If the second round also fails with -ENOMEM, that btf_put() takes the
refcount from 2 to 1.  The BTF is not freed and btf_free_id() is not called,
so a second, partially applied "vmlinux" BTF stays in btf_idr and can be
found by BPF_BTF_GET_NEXT_ID and BPF_BTF_GET_FD_BY_ID until that fd is
closed.

The fd could also be used while the other round is still applying
registrations.  For example, BPF_PROG_LOAD accepts a kernel BTF fd as
attach_btf_obj_fd, and for struct_ops programs the verifier then walks
btf->struct_ops_tab while btf_add_struct_ops() may krealloc() it.

Would returning the error from btf_get_fd_by_id() when bpf_get_btf_vmlinux()
fails (or at least on -ENOMEM) close this?


---
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/41205212136