Re: [RFC PATCH v3 07/13] s390/topology: Initialize sbm topology during topology_init_early()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] s390/topology: OOB write in SBM due to underestimating max_threads_per_instance
- [High] s390/topology: arch_sbm_cpu_instance_id() fails to map secondary CPUs
- [Low] s390/topology: __configure_sbm_topology() counts dummy head node
--

commit 7e4b5901a7b8d62cc7a0551c478b49d8ad5ae92b
Author: K Prateek Nayak <kprateek.nayak@amd.com>

s390/topology: Initialize sbm topology during topology_init_early()

This commit configures sparsebitmask (sbm) properties for the system using
socket_info when the topology mode is TOPOLOGY_MODE_HW. It also adds
arch_sbm_cpu_instance_id() to map CPUs to socket instance IDs during
CPU hotplug events.

> diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
> index 1377c6f3f6709..80dc51f5ce6f5 100644
> --- a/arch/s390/kernel/topology.c
> +++ b/arch/s390/kernel/topology.c

[ ... ]

> @@ -561,6 +562,55 @@ static int __init detect_polarization(union topology_entry *tle)
>  	return tl_core->pp != POLARIZATION_HRZ;
>  }
>  
> +int arch_sbm_cpu_instance_id(int cpu)
> +{
> +	struct mask_info *info = &socket_info;
> +
> +	/*
> +	 * sbm core should not call this if sbm_set_topology() was
> +	 * skipped below due to lack of topology information.
> +	 */
> +	if (WARN_ON_ONCE(topology_mode != TOPOLOGY_MODE_HW))
> +		return -1;
> +
> +	while (info) {
> +		if (cpumask_test_cpu(cpu, &info->mask))
> +			return info->id;

[Severity: High]
Can this fail to map newly hotplugged CPUs?

Since hardware topology masks on s390 are updated asynchronously via a
workqueue, the mask might not be populated before the CPU online callback
executes during CPU hotplug.

> +		info = info->next;
> +	}
> +
> +	pr_warn_once("socket mapping for CPU%d not found! Mapping to ID 0", cpu);
> +	return 0;

[Severity: High]
If the mask check above fails due to the asynchronous update race, does
falling back to instance 0 break SBM topology isolation by grouping all
secondary CPUs into a single instance?

> +}
> +
> +static void __configure_sbm_topology(void)
> +{
> +	int num_sbm_instances = 0, max_threads_per_instance = -1;
> +	struct mask_info *info = &socket_info;

[Severity: Low]
Should this traversal start at info->next instead of the dummy head node?

socket_info serves as a static dummy head, and alloc_masks() attaches newly
allocated instances starting at socket_info.next.

> +
> +	/*
> +	 * Consider single package if topology
> +	 * information is unavailable.
> +	 *
> +	 * sbm core will handle the initialization.
> +	 */
> +	if (topology_mode != TOPOLOGY_MODE_HW)
> +		return;
> +
> +	while (info) {
> +		num_sbm_instances += 1;

[Severity: Low]
Counting the dummy head node as a valid instance appears to pass an inflated
count to sbm_set_topology(), which would overallocate the SBM instance
metadata arrays by exactly one element.

> +		max_threads_per_instance = max_t(int,
> +						 cpumask_weight(&info->mask),
> +						 max_threads_per_instance);

[Severity: High]
Does calculating the max threads per instance here underestimate the
capacity?

Because this is called during topology_init_early() when only the boot CPU
is marked as present, info->mask will only contain the boot CPU's threads.
This results in a drastically undersized max_threads_per_instance.

When secondary CPUs come online later, alloc_sbm_index() assigns them
local indices. Will returning an index that exceeds this undersized limit
trigger an out-of-bounds write in the SBM core when accessing its internal
arrays?

> +		info = info->next;
> +	}
> +
> +	if (WARN_ON_ONCE(!num_sbm_instances || max_threads_per_instance < 1))
> +		return;
> +
> +	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=7