Re: [PATCH 10/17] platform/x86/intel/ifs: Validate TID_BIT_SHIFT matches topology_cluster_cpumask()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform/x86/intel/ifs: ifs_init() fails on SAF_GEN=0 CPUs
- [Medium] platform/x86/intel/ifs: broken cluster validation in topology_cluster_matched()
--

commit d52108059abf4fca7e3a463651949851cf65de59
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

platform/x86/intel/ifs: Validate TID_BIT_SHIFT matches topology_cluster_cpumask()

This commit validates during driver initialization that the logical
processors indicated by TID_BIT_SHIFT in MSR_INTEGRITY_CAPS match
topology_cluster_cpumask(), failing driver load if they do not match.

> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index f2d2caab04797..c830a02fe7b33 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
[ ... ]
> @@ -69,6 +71,45 @@ static struct ifs_device ifs_devices[] = {
>  	},
>  };
>  
> +/*
> + * Test whether logical processors indicated by TID_BIT_SHIFT that share the
> + * same SCAN test engine match the cpumask from topology_cluster_cpumask().
> + */
> +static bool topology_cluster_matched(u32 msrval)
> +{
> +	u32 dist, shift = FIELD_GET(MSR_INTEGRITY_CAPS_TID_BIT_SHIFT_MASK, msrval);
> +	const cpumask_t *cpumask = topology_cluster_cpumask(0);
> +	u32 delta, step = 0, first = cpu_data(0).topo.apicid;
> +	unsigned int cpu, cpus;
> +
> +	/* Get the minimal APIC ID step among the CPUs in the same cluster. */
> +	for_each_cpu_andnot(cpu, cpumask, cpumask_of(0)) {
> +		delta = abs(cpu_data(cpu).topo.apicid - first);
> +
> +		if (!step) {
> +			step = delta;
> +			continue;
> +		}
> +
> +		if (step > delta)
> +			step = delta;
> +	}
> +
> +	/* A Hyper-Threading (HT) system with HT disabled. */
> +	if (!step)
> +		step = 2;
> +
> +	cpus = cpumask_weight(cpumask);
> +	dist = step * cpus;
> +
> +	if (dist != (1 << shift)) {
> +		pr_err_once("APICID step %u, group cpus %u, TID_BIT_SHIFT %u\n", step, cpus, shift);
> +		return false;
> +	}
> +
> +	return true;
> +}

[Severity: Medium]
Can topology_cluster_matched() fail when CPUs in cluster 0 are offline or
undergoing hotplug during module initialization?

topology_cluster_cpumask(0) returns a pointer to per_cpu(cpu_l2c_shared_map,
0), which only tracks currently online CPUs and is updated in-place by
set_cpu_sibling_map() and remove_siblinginfo() under the CPU hotplug lock:

arch/x86/kernel/smpboot.c:remove_siblinginfo() {
	...
	for_each_cpu(sibling, cpu_l2c_shared_mask(cpu))
		cpumask_clear_cpu(cpu, cpu_l2c_shared_mask(sibling));
	...
}

Because CONFIG_INTEL_IFS is tristate, ifs_init() can run at module load time
without holding cpus_read_lock(). topology_cluster_matched() iterates over
cpumask in for_each_cpu_andnot() to compute step and then re-reads cpumask
in cpumask_weight(cpumask) to compute cpus, so concurrent CPU hotplug on a
cluster-0 sibling can mutate the mask between the two reads.

Furthermore, if any CPU in cluster 0 is offline when the module is loaded
(for example, when a core or thread in a Dual-Core Module is offline, or
when booted with maxcpus=1), cpumask_weight(cpumask) and step will not
reflect the full hardware cluster, causing dist != (1 << shift) and failing
driver initialization with -EINVAL.

In addition, when cpumask contains only CPU 0 (cpus == 1), the fallback:

	if (!step)
		step = 2;

forces dist to 2, while when cpus >= 2, step >= 1 also makes dist >= 2. Can
dist ever match (1 << shift) on a non-SMT single-CPU cluster where
TID_BIT_SHIFT is 0?

> +
>  #define IFS_NUMTESTS ARRAY_SIZE(ifs_devices)
>  
>  static void ifs_cleanup(void)
> @@ -122,6 +163,13 @@ static int __init ifs_init(void)
>  		 */
>  		ifs_devices[i].rw_data.all_lp_join = ifs_devices[i].rw_data.generation ?
>  						(msrval & MSR_INTEGRITY_CAPS_ALL_LP_JOIN) : true;
> +
> +		if (ifs_devices[i].rw_data.all_lp_join && !topology_cluster_matched(msrval)) {
> +			pr_err_once("TID_BIT_SHIFT mismatched topology_cluster_cpumask().\n");
> +			ret = -EINVAL;
> +			goto err_exit;
> +		}

[Severity: High]
Won't this check cause ifs_init() to unconditionally fail with -EINVAL on
Sapphire Rapids and Emerald Rapids (SAF_GEN=0) processors?

On SAF_GEN=0 CPUs, ifs_devices[i].rw_data.generation is 0, so
ifs_devices[i].rw_data.all_lp_join is forced to true. However, bits 20:16
(TID_BIT_SHIFT) of MSR_INTEGRITY_CAPS were only introduced in SAF_GEN=2 and
read as 0 on SAF_GEN=0 platforms.

When ifs_init() calls topology_cluster_matched(msrval) with shift == 0,
(1 << shift) is 1, whereas dist = step * cpus evaluates to 2 (both with
Hyper-Threading enabled where cpus == 2 and step == 1, and with
Hyper-Threading disabled where cpus == 1 and step is forced to 2). As a
result, dist != (1 << shift) is always true and topology_cluster_matched()
always returns false on SAF_GEN=0 hardware.

Should the topology_cluster_matched() check also be gated on
ifs_devices[i].rw_data.generation (or only performed when ALL_LP_JOIN is
enumerated by hardware)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026183820.129459-1-ehab.ababneh@intel.com?part=10