Re: [RFC PATCH v3 09/13] x86/cpu/topology: Initialize sbm topology after topology parsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/topology: UB and incorrect topology IDs due to out-of-bounds subtract in dom_shifts
--

commit c4055a2c9b6199a420263b5bc2ed2a987d466f5a
Author: Peter Zijlstra <peterz@infradead.org>

x86/cpu/topology: Initialize sbm topology after topology parsing

This commit initializes the sparsebitmap (sbm) topology on x86 after parsing
the CPU topology. It uses the maximum APICID and topology domain shifts to
determine the maximum number of threads per LLC instance and the maximum
number of instances.

> diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
> index 4913b64ec592d..150e098f0ef8f 100644
> --- a/arch/x86/kernel/cpu/topology.c
> +++ b/arch/x86/kernel/cpu/topology.c

[ ... ]

> @@ -449,13 +450,45 @@ static __init bool restrict_to_up(void)
>  	return apic_is_disabled;
>  }
>  
> +int arch_sbm_cpu_instance_id(int cpu)
> +{
> +	u32 sbm_shift = x86_topo_system.dom_shifts[TOPO_DIE_DOMAIN] - 1;

[Severity: High]
Does this code compute the wrong shift value by subtracting 1 from the
shift amount instead of the domain index?

If x86_topo_system.dom_shifts[TOPO_DIE_DOMAIN] happens to be 0 (such as on
CPUs without SMT or cores), this evaluates to 0xFFFFFFFF since sbm_shift is
an unsigned 32-bit integer. This would lead to undefined behavior when
evaluating apicid >> sbm_shift later in the function.

Even if the shift value is non-zero, this subtracts 1 from the bit shift 
value itself, which calculates the wrong shift, inappropriately halving the 
max threads per instance and generating incorrect SBM instance IDs.

Should this instead be x86_topo_system.dom_shifts[TOPO_DIE_DOMAIN - 1]?

Please note that this bug appears to remain unfixed at the end of the patch
series in commit 0b0c43c6e4d9b31b812d8407b146c5dc13783dc8.

> +	u32 apicid = cpuid_to_apicid[cpu];
> +
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> +	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> +		sbm_shift = x86_topo_system.dom_shifts[TOPO_TILE_DOMAIN] - 1;

[Severity: High]
Is the subtract from the value rather than the index also an issue here
for AMD and Hygon systems?

> +
> +	return (apicid >> sbm_shift);
> +}
> +
> +static __init void init_sbm_topology(u32 max_apicid)
> +{
> +	u32 sbm_shift = x86_topo_system.dom_shifts[TOPO_DIE_DOMAIN] - 1;

[Severity: High]
Could this also lead to undefined behavior or incorrect shift calculations
during early boot initialization, for the same reasons as above?

> +	int num_sbm_instances, max_threads_per_instance;
> +
> +	/*
> +	 * On Intel systems, memory controllers are present at TOPO_DIE_DOMAIN.
> +	 * On newer AMD and Hygon systems, LLC is at TOPO_TILE_DOMAIN so use
> +	 * that instead.
> +	 */
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> +	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> +		sbm_shift = x86_topo_system.dom_shifts[TOPO_TILE_DOMAIN] - 1;

[Severity: High]
Does this code also have the same issue with subtracting 1 from the array
value instead of the array index during boot initialization?

> +
> +	num_sbm_instances = 1 + (max_apicid >> sbm_shift);
> +	max_threads_per_instance = (1 << sbm_shift);
> +
> +	sbm_set_topology(num_sbm_instances, max_threads_per_instance);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001192849.74788-1-kprateek.nayak@amd.com?part=9