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

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

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

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.

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

> 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.