Re: [PATCH v9 bpf-next 06/10] btf: support kernel parsing of BTF with layout info
Alan Maguire <[email protected]>
| Newsgroups | org.kernel.vger.dwarves,org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On 26/02/2026 10:05, [email protected] wrote: >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >> --- a/kernel/bpf/btf.c >> +++ b/kernel/bpf/btf.c > > [ ... ] > >> @@ -5327,23 +5328,36 @@ static s32 btf_check_meta(struct btf_verifier_env *env, >> return -EINVAL; >> } >> >> + if (!btf_name_offset_valid(env->btf, t->name_off)) { >> + btf_verifier_log(env, "[%u] Invalid name_offset:%u", >> + env->log_type_id, t->name_off); >> + return -EINVAL; >> + } >> + >> + if (BTF_INFO_KIND(t->info) == BTF_KIND_UNKN) { >> btf_verifier_log(env, "[%u] Invalid kind:%u", >> env->log_type_id, BTF_INFO_KIND(t->info)); >> return -EINVAL; >> } >> >> + if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX && env->btf->layout && >> + ((BTF_INFO_KIND(t->info) + 1) * sizeof(struct btf_layout)) < >> + env->btf->hdr.layout_len) { > ^ > > Should this be <= instead of < ? If the layout section has exactly > N entries, layout_len is N * sizeof(struct btf_layout). For the > last covered kind (K = N - 1), this computes: > > (K + 1) * sizeof(struct btf_layout) == layout_len > > With < that evaluates to false, so the last kind covered by the > layout section falls through to the else branch and gets logged as > "Invalid kind" instead of "unknown but required kind". > > The libbpf equivalent in btf_type_size_unknown() uses: > > __u32 layout_kinds = btf->hdr.layout_len / sizeof(struct btf_layout); > if (!l || kind >= layout_kinds) { > > which accepts the last entry. Using <= here would match that > behavior. > looking at this again, is it worth making the distinction between a kind that's in the BTF layout but unknown to the kernel versus one that is not in the BTF layout and unknown to the kernel? In both cases we reject the BTF, and given that the kernel cannot be in the business of trusting the layout provided with that BTF it seems like it's a distinction without a practical difference. >> + btf_verifier_log(env, "[%u] unknown but required kind %u", >> + env->log_type_id, >> + BTF_INFO_KIND(t->info)); >> return -EINVAL; >> + } else { >> + if (BTF_INFO_KIND(t->info) > BTF_KIND_MAX) { >> + btf_verifier_log(env, "[%u] Invalid kind:%u", >> + env->log_type_id, BTF_INFO_KIND(t->info)); >> + return -EINVAL; >> + } >> + var_meta_size = btf_type_ops(t)->check_meta(env, t, meta_left); >> + if (var_meta_size < 0) >> + return var_meta_size; >> } > > > --- > 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/22436234361 > > AI-authorship-score: low > AI-authorship-explanation: Multi-year revision history with detailed reviewer interactions, idiomatic kernel C, and an off-by-one error typical of manual arithmetic translation are inconsistent with AI-generated code. > issues-found: 1 > issue-severity-score: low > issue-severity-explanation: Off-by-one in layout bounds check produces wrong verifier log message but does not change functional behavior since both paths return -EINVAL.