Re: [PATCH 10/13] perf header: Sanity check HEADER_HYBRID_TOPOLOGY

Arnaldo Carvalho de Melo
2026-04-10T01:01:27Z

On Fri, Apr 10, 2026 at 12:58:37AM +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 1 potential issue(s):
> - [Low] The MAX_PMU_MAPPINGS macro is defined locally inside process_pmu_mappings() but reused in process_hybrid_topology().
> --
>
> commit b5af27fec41475de3166cc72abe94ddc329689cc
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> perf header: Sanity check HEADER_HYBRID_TOPOLOGY
>
> This commit adds an upper bound check on nr_nodes in
> process_hybrid_topology() to harden against malformed perf.data files by
> reusing the MAX_PMU_MAPPINGS limit.
>
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -3448,6 +3448,18 @@ static int process_hybrid_topology(struct feat_fd *ff,
> > if (do_read_u32(ff, &nr))
> > return -1;
> >
> > + if (nr > MAX_PMU_MAPPINGS) {
>
> This isn't a bug, but could this cause build failures if the functions are
> reordered in the future?
>
> MAX_PMU_MAPPINGS is currently defined locally inside process_pmu_mappings():
>
> tools/perf/util/header.c:process_pmu_mappings() {
> ...
> #define MAX_PMU_MAPPINGS 4096
> if (pmu_num > MAX_PMU_MAPPINGS) {
> ...
> }
>
> While the C preprocessor doesn't respect function block scope, allowing it
> to be used here, this relies on process_pmu_mappings() always appearing
> lexically before process_hybrid_topology() in this file.
>
> Should this macro be hoisted to file scope to prevent accidental breakage
> during future refactoring?

Yeah, I'll do it in v2.

- Arnaldo

> > + pr_err("Invalid HEADER_HYBRID_TOPOLOGY: nr_nodes (%u) > %u\n",
> > + nr, MAX_PMU_MAPPINGS);
> > + return -1;
> > + }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260410004000.148138-1-acme@kernel.org?part=10