> 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 <[email protected]> > TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <[email protected]> > Signed-off-by: Jay Wang <[email protected]> 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
