Re: [PATCH bpf-next 07/13] bpf: parse the vmlinux BTF before taking cand_cache_mutex

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5325 bytes --]

> 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 <ihor.solodrai@linux.dev>
>     [pass the error of the fetch on instead of -EINVAL from the peek]
>     Signed-off-by: Jay Wang <wanjay@amazon.com>

The author's sign-off is the placeholder line "TODO-NEEDS-IHOR:
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>", 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