[PATCH v2 0/2] cpupower: fix topology array handling

[PATCH v2 0/2] cpupower: fix topology array handling

[Ali Ahmet Memis]

v1 did two things in one patch. Shuah asked for them to be split, so here
they are as two.

Patch 1 is the uninitialized read: get_cpu_topology() allocates core_info
with malloc(), several paths never write core_cpu_list, and the sort
comparator then hands that buffer to strcmp(). calloc() fixes it.

Patch 2 is separate and only about the physical core count. The counting
loop seeds cores at 1 from entry 0 without checking whether that entry has
usable topology data, so an incomplete entry can be counted as a core.
Patch 2 depends on patch 1, since it uses an empty core_cpu_list to
recognise an entry that was never filled in.

Tested against a fake sysfs tree with the configured CPU count pinned to 2,
where cpu0 has complete topology and cpu1's topology attributes are absent.
Same harness, one commit apart:

  unpatched     cores=2   valgrind: errors
  patch 1       cores=2   valgrind: clean
  patch 1+2     cores=1   valgrind: clean

Unpatched, valgrind traces it to the allocation:

  Conditional jump or move depends on uninitialised value(s)
     at strcmp (vg_replace_strmem.c:941)
     by __compare_core_cpu_list (cpupower.c:159)
     by qsort_r (qsort.c:409)
     by get_cpu_topology (cpupower.c:214)
   Uninitialised value was created by a heap allocation
     at malloc (vg_replace_malloc.c:446)
     by get_cpu_topology (cpupower.c:174)

I have no machine where a topology attribute actually disappears during
enumeration, so the fake tree is as close as I could get. If you would
rather see this exercised some other way, say so and I will do that.

Link to the v1 review:
https://lore.kernel.org/all/c746110c-6ac4-4500-a4f0-491a06838173@kernel.org/

Ali Ahmet Memis (2):
  cpupower: zero the topology array to avoid uninitialized reads
  cpupower: do not count incomplete topology entries as physical cores

 tools/power/cpupower/lib/cpupower.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
2.55.0

[PATCH v2 1/2] cpupower: zero the topology array to avoid uninitialized reads

[Ali Ahmet Memis]

get_cpu_topology() allocates core_info with malloc() and then fills it in
per CPU. Three paths leave core_cpu_list untouched: a failed
physical_package_id read, a failed core_id read, and a core_cpus_list read
that comes back empty, which only prints a warning.

The array is then sorted with __compare_core_cpu_list(), which passes
core_cpu_list to strcmp(). For the entries above that buffer still holds
whatever malloc() returned, so strcmp() reads uninitialized memory, and if
the buffer happens to contain no NUL byte it reads past the end of it.

Allocate with calloc() so an entry that is never filled in compares as an
empty string.

Fixes: f89cb9cba7a2 ("cpupower: Implement CPU physical core querying")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 tools/power/cpupower/lib/cpupower.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
index d7f7ec6f151c..559b04f4387e 100644
--- a/tools/power/cpupower/lib/cpupower.c
+++ b/tools/power/cpupower/lib/cpupower.c
@@ -171,7 +171,7 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 	char path[SYSFS_PATH_MAX];
 	char *last_cpu_list;
 
-	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
+	cpu_top->core_info = calloc(cpus, sizeof(struct cpuid_core_info));
 	if (cpu_top->core_info == NULL)
 		return -ENOMEM;
 	cpu_top->pkgs = cpu_top->cores = 0;
-- 
2.55.0

[PATCH v2 2/2] cpupower: do not count incomplete topology entries as physical cores

[Ali Ahmet Memis]

The physical core count is derived by sorting core_info by core_cpu_list
and counting how many distinct lists there are. The loop seeds the count
with entry 0 unconditionally:

	last_cpu_list = cpu_top->core_info[0].core_cpu_list;
	cpu_top->cores = 1;

An entry whose topology could not be read is still a member of the array,
carrying pkg and core of -1 and either an empty core_cpu_list or the
literal "-1". After the sort such an entry can land at index 0, where it is
counted as a physical core even though the pkg check inside the loop was
meant to exclude it. The check is also applied only to the entries that
follow, so the seed is never validated.

Skip entries without complete topology data and count from zero, so only
CPUs with a package, a core and a core list contribute.

Fixes: f89cb9cba7a2 ("cpupower: Implement CPU physical core querying")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 tools/power/cpupower/lib/cpupower.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
index 559b04f4387e..a8ee304bdcc0 100644
--- a/tools/power/cpupower/lib/cpupower.c
+++ b/tools/power/cpupower/lib/cpupower.c
@@ -214,11 +214,17 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 	qsort(cpu_top->core_info, cpus, sizeof(struct cpuid_core_info),
 	      __compare_core_cpu_list);
 
-	last_cpu_list = cpu_top->core_info[0].core_cpu_list;
-	cpu_top->cores = 1;
-	for (cpu = 1; cpu < cpus; cpu++) {
-		if (strcmp(cpu_top->core_info[cpu].core_cpu_list, last_cpu_list) != 0 &&
-		    cpu_top->core_info[cpu].pkg != -1) {
+	last_cpu_list = NULL;
+	cpu_top->cores = 0;
+	for (cpu = 0; cpu < cpus; cpu++) {
+		if (cpu_top->core_info[cpu].pkg == -1 ||
+		    cpu_top->core_info[cpu].core == -1 ||
+		    cpu_top->core_info[cpu].core_cpu_list[0] == '\0')
+			continue;
+
+		if (!last_cpu_list ||
+		    strcmp(cpu_top->core_info[cpu].core_cpu_list,
+			   last_cpu_list) != 0) {
 			last_cpu_list = cpu_top->core_info[cpu].core_cpu_list;
 			cpu_top->cores++;
 		}
-- 
2.55.0

Re: [PATCH v2 0/2] cpupower: fix topology array handling

[Shuah Khan]

On 8/3/26 11:52, Ali Ahmet Memis wrote:
> v1 did two things in one patch. Shuah asked for them to be split, so here
> they are as two.
> 
> Patch 1 is the uninitialized read: get_cpu_topology() allocates core_info
> with malloc(), several paths never write core_cpu_list, and the sort
> comparator then hands that buffer to strcmp(). calloc() fixes it.

I agree that core_cpu_list isn't initialized and that needs fixing.
It can be done with your first patch that replaces malloc() with
calloc().

This code path can be improved to initialize core_cpu_list.

Did you think about a scenario when the following check will
be tru - i.e core == -1 is trur?

if (cpu_top->core_info[cpu].core == -1) {
                         strncpy(cpu_top->core_info[cpu].core_cpu_list, "-1", CPULIST_BUFFER);
                         continue;
                 }

> 
> Patch 2 is separate and only about the physical core count. The counting
> loop seeds cores at 1 from entry 0 without checking whether that entry has
> usable topology data, so an incomplete entry can be counted as a core.
> Patch 2 depends on patch 1, since it uses an empty core_cpu_list to
> recognise an entry that was never filled in.

Can you elaborate on a real scenario where this could happen after
replacing malloc() with calloc() and making sure core_cpu_list is
initialized to "-1" like in the above conditional?

thanks,
-- Shuah

Re: [PATCH v2 0/2] cpupower: fix topology array handling

[Ali Ahmet Memis]

On Tue, 4 Aug 2026 14:45:48 -0600 Shuah Khan wrote:
> Did you think about a scenario when the following check will be tru - i.e
> core == -1 is trur?

I went looking for one and could not find it, so that branch may well be
dead. What I checked:

On the architectures using drivers/base/arch_topology.c, reset_cpu_topology()
does start every possible CPU at core_id = -1, but store_cpu_topology()
overwrites it for any CPU that comes up without firmware topology:

	if (cpuid_topo->package_id != -1)
		goto topology_populated;
	cpuid_topo->thread_id = -1;
	cpuid_topo->core_id = cpuid;
	cpuid_topo->package_id = cpu_to_node(cpuid);

and it is called from the bring-up paths, arch/arm64/kernel/smp.c and
arch/riscv/kernel/smpboot.c. On x86 core_id is either derived from the apic
id in arch/x86/kernel/cpu/topology_common.c or set to 0 in smpboot.c, so it
is never negative either.

A CPU with no topology at all does not show up as -1 either. The topology
attribute group is created from a CPU hotplug prepare callback in
drivers/base/topology.c, so a CPU that never comes up has no topology
directory and the read fails outright rather than returning -1.

That last case is the one that matters here, and it takes one of the two
earlier continue branches rather than the one you quoted.

> Can you elaborate on a real scenario where this could happen after
> replacing malloc() with calloc() and making sure core_cpu_list is
> initialized to "-1" like in the above conditional?

Those two branches set pkg and core to -1 and leave core_cpu_list untouched,
so under calloc it stays empty, and the count is still wrong. I ran this
against a fake sysfs tree with the CPU count pinned, cpu0 with real topology
and cpu1 with no topology files at all:

  unpatched            cores=2
  patch 1 only         cores=2
  patch 1 and 2        cores=1

and with three CPUs, cpu0 and cpu1 real and cpu2 unreadable:

  patch 1 only         cores=3
  patch 1 and 2        cores=2

The reason is the seed, not the buffer contents:

	last_cpu_list = cpu_top->core_info[0].core_cpu_list;
	cpu_top->cores = 1;

An empty string and "-1" both sort ahead of a real cpu list, so after the
qsort entry 0 is an incomplete one, and cores is seeded to 1 from it without
ever looking at pkg. The pkg != -1 check inside the loop only guards the
entries that follow, never the one the count started from. That is why
initializing the buffer to "-1" does not help: it changes what entry 0
contains, not the fact that it is counted.

One consequence worth stating rather than leaving for you to find. If no CPU
has usable topology at all, the count changes:

  patch 1 only         cores=1
  patch 1 and 2        cores=0

That direction looks like the consistent one rather than a regression, since
pkgs already reports 0 in that case today, so the current code prints
"Packages: 0 - Cores: 1" and after patch 2 it prints "Packages: 0 -
Cores: 0". The only in-tree consumer of cores is the dprint() in
cpupower-monitor.c, so nothing there divides by it or sizes an allocation
with it, but cores is in the installed cpupower.h so I cannot speak for
out-of-tree users of the library.

Re: [PATCH v2 0/2] cpupower: fix topology array handling

[Ali Ahmet Memis]

> but cores is in the installed cpupower.h so I cannot speak for
> out-of-tree users of the library.

That last clause is wrong and I should not have written it without looking
cpupower.h is not installed. install-lib in tools/power/cpupower/Makefile
installs cpufreq.h, cpuidle.h and powercap.h only:

	$(INSTALL_DATA) lib/cpufreq.h $(DESTDIR)${includedir}/cpufreq.h
	$(INSTALL_DATA) lib/cpuidle.h $(DESTDIR)${includedir}/cpuidle.h
	$(INSTALL_DATA) lib/powercap.h $(DESTDIR)${includedir}/powercap.h

cpupower.h appears in the LIB_HEADERS build variable, which is what I saw,
but that is a build dependency list and not the install list.
get_cpu_topology() and struct cpupower_topology are declared only in that
uninstalled header, so the caveat I attached does not apply.

The rest of the message stands.

Re: [PATCH v2 0/2] cpupower: fix topology array handling

[Shuah Khan]

On 8/5/26 05:43, Ali Ahmet Memis wrote:
> On Tue, 4 Aug 2026 14:45:48 -0600 Shuah Khan wrote:
>> Did you think about a scenario when the following check will be tru - i.e
>> core == -1 is trur?
> 
> I went looking for one and could not find it, so that branch may well be
> dead. What I checked:

That is really the questions - the branch isn't dead, it is in the wrong
place.

Sounds like you don't have a real scenario to test this change. This why
I am not eager to take either of these patches.

However, did you consider simplifying the logic in these conditionals?


if(sysfs_topology_read_file(
                         cpu,
                         "physical_package_id",
                         &(cpu_top->core_info[cpu].pkg)) < 0) {
                         cpu_top->core_info[cpu].pkg = -1;
                         cpu_top->core_info[cpu].core = -1;
                         continue;

-- Is this continue necessary here?

                 }
                 if(sysfs_topology_read_file(
                         cpu,
                         "core_id",
                         &(cpu_top->core_info[cpu].core)) < 0) {
                         cpu_top->core_info[cpu].pkg = -1;
                         cpu_top->core_info[cpu].core = -1;
                         continue;

-- Is this continue necessary here?

                 }

I think the following logic makes sense without the continue(s)

                 if (cpu_top->core_info[cpu].core == -1) {
                         strncpy(cpu_top->core_info[cpu].core_cpu_list, "-1", CPULIST_BUFFER);
                         continue;
                 }

thanks,
-- Shuah

[PATCH v3 0/3] cpupower: fix topology array handling

[Ali Ahmet Memis]

First, a correction. The v2 cover letter said I had no machine where a
topology attribute actually disappears during enumeration. That was wrong,
and it is the answer to your question about a real scenario: an offline CPU
is enough. The topology attribute group is added and removed by a CPU
hotplug callback in drivers/base/topology.c, so while a CPU is offline it
has no topology directory at all and both reads fail. chcpu -d, a write to
cpuN/online, or turning SMT off all get there.

Measured on a 4 CPU machine against its real sysfs, no fake tree this time,
calling get_cpu_topology() and printing what it decided:

  all four CPUs online
    unpatched                        cores=4
    v3 series                        cores=4

  cpu2 and cpu3 offlined
    unpatched                        cores=3
    with both continues removed      cores=3
    v3 series                        cores=2

Two online CPUs, one core each, so 3 is the wrong answer and 4 is
unaffected by the series.

> However, did you consider simplifying the logic in these conditionals?
> -- Is this continue necessary here?

No, they are not necessary, and removing them is the right thing. That is
patch 2. Without them the core == -1 check runs for the entries it was
written for and gives them a defined core_cpu_list of "-1", which is what
you meant by the branch being in the wrong place rather than dead.

It does not change the count on its own though, which is the third row
above. The count is seeded before anything is checked:

	last_cpu_list = cpu_top->core_info[0].core_cpu_list;
	cpu_top->cores = 1;

and "-1" sorts ahead of a real cpu list, so entry 0 after the qsort is a
placeholder and the count starts by counting it. Patch 3 is about that
seed, so the two changes are complementary rather than alternatives.

Patch 1 is unchanged from v2. Patch 2 makes the demonstrable uninitialized
read go away by itself, but calloc() is still what covers the remaining
path, a core_cpus_list read that fails and only warns, and it is the
smaller change for stable.

v2: https://lore.kernel.org/all/20260803175215.117518-1-ali@iusegentoo.com/

Ali Ahmet Memis (3):
  cpupower: zero the topology array to avoid uninitialized reads
  cpupower: let the core == -1 check handle failed topology reads
  cpupower: do not count incomplete topology entries as physical cores

 tools/power/cpupower/lib/cpupower.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)


base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
-- 
2.55.0

[PATCH v3 1/3] cpupower: zero the topology array to avoid uninitialized reads

[Ali Ahmet Memis]

get_cpu_topology() allocates core_info with malloc() and then fills it in
per CPU. Three paths leave core_cpu_list untouched: a failed
physical_package_id read, a failed core_id read, and a core_cpus_list read
that comes back empty, which only prints a warning.

The array is then sorted with __compare_core_cpu_list(), which passes
core_cpu_list to strcmp(). For the entries above that buffer still holds
whatever malloc() returned, so strcmp() reads uninitialized memory, and if
the buffer happens to contain no NUL byte it reads past the end of it.

Allocate with calloc() so an entry that is never filled in compares as an
empty string.

Fixes: f89cb9cba7a2 ("cpupower: Implement CPU physical core querying")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 tools/power/cpupower/lib/cpupower.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
index d7f7ec6f151c..559b04f4387e 100644
--- a/tools/power/cpupower/lib/cpupower.c
+++ b/tools/power/cpupower/lib/cpupower.c
@@ -171,7 +171,7 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 	char path[SYSFS_PATH_MAX];
 	char *last_cpu_list;
 
-	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
+	cpu_top->core_info = calloc(cpus, sizeof(struct cpuid_core_info));
 	if (cpu_top->core_info == NULL)
 		return -ENOMEM;
 	cpu_top->pkgs = cpu_top->cores = 0;
-- 
2.55.0

[PATCH v3 2/3] cpupower: let the core == -1 check handle failed topology reads

[Ali Ahmet Memis]

When physical_package_id or core_id cannot be read, get_cpu_topology()
sets pkg and core to -1 and skips the rest of the iteration. The check
below them exists to give such an entry a defined core_cpu_list:

	if (cpu_top->core_info[cpu].core == -1) {
		strncpy(cpu_top->core_info[cpu].core_cpu_list, "-1", CPULIST_BUFFER);
		continue;
	}

The two continue statements above it mean it never runs for the entries it
was written for, since a CPU whose topology cannot be read is exactly the
case it describes. Nothing between the reads and that check needs skipping,
so drop the continue statements and let it do its job.

Suggested-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 tools/power/cpupower/lib/cpupower.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
index 559b04f4387e..3d7e22ed1fd7 100644
--- a/tools/power/cpupower/lib/cpupower.c
+++ b/tools/power/cpupower/lib/cpupower.c
@@ -184,7 +184,6 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 			&(cpu_top->core_info[cpu].pkg)) < 0) {
 			cpu_top->core_info[cpu].pkg = -1;
 			cpu_top->core_info[cpu].core = -1;
-			continue;
 		}
 		if(sysfs_topology_read_file(
 			cpu,
@@ -192,7 +191,6 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 			&(cpu_top->core_info[cpu].core)) < 0) {
 			cpu_top->core_info[cpu].pkg = -1;
 			cpu_top->core_info[cpu].core = -1;
-			continue;
 		}
 		if (cpu_top->core_info[cpu].core == -1) {
 			strncpy(cpu_top->core_info[cpu].core_cpu_list, "-1", CPULIST_BUFFER);
-- 
2.55.0

[PATCH v3 3/3] cpupower: do not count incomplete topology entries as physical cores

[Ali Ahmet Memis]

The physical core count is derived by sorting core_info by core_cpu_list
and counting how many distinct lists there are. The loop seeds the count
with entry 0 unconditionally:

	last_cpu_list = cpu_top->core_info[0].core_cpu_list;
	cpu_top->cores = 1;

A CPU whose topology could not be read is still a member of the array,
carrying pkg and core of -1 and a core_cpu_list of "-1". That sorts ahead
of any real cpu list, so after the qsort it is entry 0 and it seeds the
count as if it were a core. The pkg check inside the loop only guards the
entries that follow, never the one the count started from.

An offline CPU is enough to reach this. The topology attribute group is
added and removed by a CPU hotplug callback in drivers/base/topology.c, so
physical_package_id and core_id are absent while a CPU is offline and both
reads fail.

Skip entries without complete topology data and count from zero, so only
CPUs with a package and a core contribute.

Fixes: f89cb9cba7a2 ("cpupower: Implement CPU physical core querying")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 tools/power/cpupower/lib/cpupower.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
index 3d7e22ed1fd7..7784eb499435 100644
--- a/tools/power/cpupower/lib/cpupower.c
+++ b/tools/power/cpupower/lib/cpupower.c
@@ -212,11 +212,16 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 	qsort(cpu_top->core_info, cpus, sizeof(struct cpuid_core_info),
 	      __compare_core_cpu_list);
 
-	last_cpu_list = cpu_top->core_info[0].core_cpu_list;
-	cpu_top->cores = 1;
-	for (cpu = 1; cpu < cpus; cpu++) {
-		if (strcmp(cpu_top->core_info[cpu].core_cpu_list, last_cpu_list) != 0 &&
-		    cpu_top->core_info[cpu].pkg != -1) {
+	last_cpu_list = NULL;
+	cpu_top->cores = 0;
+	for (cpu = 0; cpu < cpus; cpu++) {
+		if (cpu_top->core_info[cpu].pkg == -1 ||
+		    cpu_top->core_info[cpu].core == -1)
+			continue;
+
+		if (!last_cpu_list ||
+		    strcmp(cpu_top->core_info[cpu].core_cpu_list,
+			   last_cpu_list) != 0) {
 			last_cpu_list = cpu_top->core_info[cpu].core_cpu_list;
 			cpu_top->cores++;
 		}
-- 
2.55.0