> bpf: parse the vmlinux BTF before taking cand_cache_mutex TL;DR: - New early returns on vmlinux BTF fetch failure may skip verifier log messages in btf.c, contradicting "No functional change" - Author's Signed-off-by is a TODO placeholder
> bpf: parse the vmlinux BTF before taking cand_cache_mutex > > bpf_core_find_cands() calls bpf_get_btf_vmlinux() with cand_cache_mutex > held, from btf_get_ptr_to_btf_id() and bpf_core_apply(). Once the > vmlinux BTF is parsed on first use (later in this series), parsing also > parses the module BTFs kept until then, under btf_module_mutex, while a > module going away takes btf_module_mutex and then cand_cache_mutex > (purge_cand_cache()). That would be an ABBA deadlock: > > cand_cache_mutex -> btf_vmlinux_lock -> btf_module_mutex > btf_module_mutex -> cand_cache_mutex > > Have both callers fetch the BTF before they take cand_cache_mutex, and > make bpf_core_find_cands() only peek with bpf_peek_btf_vmlinux(). The > ordering was found by design review. With the whole series applied > and the fetch moved back under the mutex, PROVE_LOCKING reports this > cycle at the first rmmod of a BTF module after a kernel CO-RE load > parsed the BTF; with this patch the same sequence is clean. > > No functional change: the BTF is parsed at the same points, just > before the mutex is taken; a broken BTF fails the load with the error > of its parse, and bpf_core_find_cands() handles a missing BTF as before. > > TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <[email protected]> > [pass the error of the fetch on instead of -EINVAL from the peek] > Signed-off-by: Jay Wang <[email protected]> The author's sign-off is the placeholder line "TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <[email protected]>", and the only real Signed-off-by tag is the one from Jay Wang. Can this be applied with that placeholder in place? The submitting-patches document (Documentation/process/submitting-patches.rst) expects a Signed-off-by from the author, and checkpatch will flag both the unknown tag and the missing sign-off. Separately, the commit message says "No functional change", but the verifier log differs when the vmlinux BTF fetch fails. Before this patch the fetch ran inside bpf_core_find_cands() and both callers logged the failure. After it, the new early returns skip that logging (see the comments on the diff below). Could that sentence be reworded to match? > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 3e947bdb224c..095362c0a1cd 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c [ ... ] > @@ -8608,6 +8609,10 @@ static int btf_get_ptr_to_btf_id(struct > bpf_verifier_log *log, int arg_idx, > t = btf_type_by_id(btf, t->type); > } > > + /* parse it, if this is its first use, before taking cand_cache_mutex */ > + vmlinux_btf = bpf_get_btf_vmlinux(); > + if (IS_ERR(vmlinux_btf)) > + return PTR_ERR(vmlinux_btf); > mutex_lock(&cand_cache_mutex); > cc = bpf_core_find_cands(&ctx, type_id); > if (IS_ERR(cc)) { Previously a failed fetch came back through bpf_core_find_cands() and ended up in the bpf_log() just below: bpf_log(log, "arg#%d reference type('%s %s') candidate matching error: %d\n", arg_idx, btf_type_str(t), __btf_name_by_offset(btf, t->name_off), err); Doesn't this early return skip that message, so a load that fails because btf_vmlinux is an ERR_PTR (btf_parse_vmlinux() failing with -ENOMEM or on malformed BTF) now returns a bare errno with nothing in the verifier log? Earlier in this series, commit 32ad8adbb1a8 ("bpf: fetch the vmlinux BTF where kernel types enter a program") added vmlinux_btf_err() in verifier.c so that every fetch point reports a failed parse: verbose(env, "in-kernel BTF could not be parsed: %ld\n", PTR_ERR(btf)); Would a bpf_log() before this return, along the lines of that message, keep the diagnostic? None of the later patches in this series appears to add one at this site. [ ... ] > @@ -10725,8 +10731,18 @@ int bpf_core_apply(struct bpf_core_ctx *ctx, const > struct bpf_core_relo *relo, > > if (need_cands) { > struct bpf_cand_cache *cc; > + struct btf *main_btf; > int i; > > + /* > + * Parsing the vmlinux BTF takes btf_module_mutex, which is > + * taken before cand_cache_mutex when a module goes away. > + */ > + main_btf = bpf_get_btf_vmlinux(); > + if (IS_ERR(main_btf)) { > + kfree(specs); > + return PTR_ERR(main_btf); > + } > mutex_lock(&cand_cache_mutex); > cc = bpf_core_find_cands(ctx, relo->type_id); > if (IS_ERR(cc)) { The same applies here. A failed fetch used to reach the bpf_log() in this branch: bpf_log(ctx->log, "target candidate search failed for %d\n", relo->type_id); and, at BPF_LOG_LEVEL2, the candidate cache dump that follows it. Can this return PTR_ERR(main_btf) leave the CO-RE relocation failing with no log output at all? Would a bpf_log() here, matching the other fetch points in the series, make sense? --- 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
