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

On Fri, Apr 10, 2026 at 12:58:37AM +0000, [email protected] 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 <[email protected]>
> 
> 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/[email protected]?part=10