linux-kernel - Re: [PATCH] libperf: Add back guard on MAX_NR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fWVHaVpJbDf=afn5MhZ972uEq=sGEmsULoD=LRff2Vouw@mail.gmail.com>
Date: Mon, 6 Jan 2025 12:05:51 -0800
From: Ian Rogers <irogers@...gle.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, "Liang, Kan" <kan.liang@...ux.intel.com>, 
	Leo Yan <leo.yan@....com>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, 
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH] libperf: Add back guard on MAX_NR_CPUS

On Mon, Jan 6, 2025 at 11:38 AM Christophe Leroy
<christophe.leroy@...roup.eu> wrote:
>
> Building perf with EXTRA_CFLAGS="-DMAX_NR_CPUS=1" fails:
>
>           CC      /home/chleroy/linux-powerpc/tools/perf/libperf/cpumap.o
>         cpumap.c:16: error: "MAX_NR_CPUS" redefined [-Werror]
>            16 | #define MAX_NR_CPUS 4096
>               |
>         <command-line>: note: this is the location of the previous definition
>
> Commit e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
> moved definition of MAX_NR_CPUS from lib/perf/include/internal/cpumap.h
> to lib/perf/cpumap.c but the guard surrounding that definition got lost
> in the move.
>
> See commit 21b8732eb447 ("perf tools: Allow overriding MAX_NR_CPUS at
> compile time") to see why it is needed.
>
> Note that MAX_NR_CPUS was initialy defined in perf/perf.h and a
> redundant definition was added by commit 9c3516d1b850 ("libperf:
> Add perf_cpu_map__new()/perf_cpu_map__read() functions").
>
> A cleaner fix would be to remove that duplicate but for the time
> being fix the problem by bringing back the guard for when MAX_NR_CPUS
> is already defined.
>
> Fixes: e8399d34d568 ("libperf cpumap: Hide/reduce scope of MAX_NR_CPUS")
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@...hat.com>

Hello,

I believe this change might be unnecessary. The only use of
MAX_NR_CPUS is in a warning message within perf_cpu_map__new, which
takes a string and produces a perf_cpu_map. Other similar functions
like cpu_map__new_sysconf don't check MAX_NR_CPUS. Therefore,
specifying a -DMAX_NR_CPUS value on the build command line has little
effect—it only impacts a warning message for certain kinds of
perf_cpu_map creation. It's also unclear what the intended outcome is
on the build command line.

Given that specifying the value doesn't seem to have a clear purpose,
allowing the build to break might be the best option. This would alert
the person building perf that they are doing something that doesn't
make sense.

Thanks,
Ian