Re: [PATCH 09/17] platform/x86/intel/ifs: Select CPUs from topology_cluster_cpumask() for simultaneous IFS tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] platform/x86/intel/ifs: build_cpugroup_mask() called before cpu_online() check
--

commit 206d196a9743b86ac3f7ca97505586f73d04016c
Author: Jithu Joseph <jithu.joseph@intel.com>

platform/x86/intel/ifs: Select CPUs from topology_cluster_cpumask() for simultaneous IFS tests

This commit updates the intel_ifs driver to record the group of CPUs
tested simultaneously in ifsd->grp_cpumask using topology_cluster_cpumask()
and stop_cluster_cpuslocked() when ifsd->all_lp_join is set.

> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index c6a4d6dcccf1f..09e7a81e92204 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c

[ ... ]

> @@ -398,6 +407,24 @@ static void ifs_array_test_gen1(int cpu, struct device *dev)
>  		ifsd->status = SCAN_TEST_PASS;
>  }
>  
> +static void build_cpugroup_mask(struct device *dev, int cpu)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +
> +	cpumask_clear(&ifsd->grp_cpumask);
> +
> +	/*
> +	 * Some CPUs, such as Sierra Forest and Clearwater Forest, have multiple cores
> +	 * sharing the same SCAN test engine, but they don't require a rendezvous.
> +	 */
> +	if (!ifsd->all_lp_join) {
> +		cpumask_set_cpu(cpu, &ifsd->grp_cpumask);
> +		return;
> +	}
> +
> +	cpumask_copy(&ifsd->grp_cpumask, topology_cluster_cpumask(cpu));
> +}
> +
>  /*
>   * Initiate per core test. It wakes up work queue threads on the target CPU and
>   * its sibling CPU. Once all sibling threads wake up, the scan test gets executed and
> @@ -411,6 +438,7 @@ int do_core_test(int cpu, struct device *dev)
>  
>  	/* Prevent CPUs from being taken offline during the scan test */
>  	cpus_read_lock();
> +	build_cpugroup_mask(dev, cpu);
>  
>  	if (!cpu_online(cpu)) {
>  		dev_info(dev, "cannot test on the offline CPU %d\n", cpu);

[Severity: High]
Should build_cpugroup_mask(dev, cpu) be called after the !cpu_online(cpu)
check in do_core_test() rather than before it?

In drivers/platform/x86/intel/ifs/sysfs.c, run_test_store() only checks
that cpu < nr_cpu_ids before invoking do_core_test(cpu, dev):

drivers/platform/x86/intel/ifs/sysfs.c:run_test_store() {
	...
	rc = kstrtouint(buf, 0, &cpu);
	if (rc < 0 || cpu >= nr_cpu_ids)
		return -EINVAL;

	rc = mutex_lock_interruptible(&ifs_sem);
	if (rc)
		return rc;

	rc = do_core_test(cpu, dev);
	...
}

However, in arch/x86/kernel/smpboot.c, smp_prepare_cpus_common() only
allocates per_cpu(cpu_l2c_shared_map, cpu) for CPUs in cpu_possible_mask:

arch/x86/kernel/smpboot.c:smp_prepare_cpus_common() {
	...
	for_each_possible_cpu(cpu) {
		node = cpu_to_node(cpu);
		...
		zalloc_cpumask_var_node(&per_cpu(cpu_l2c_shared_map, cpu), GFP_KERNEL, node);
	}
}

If userspace writes a CPU index that is less than nr_cpu_ids but not in
cpu_possible_mask (for example when CONFIG_FORCE_NR_CPUS=y or when
cpu_possible_mask is sparse), calling build_cpugroup_mask() before
checking cpu_online(cpu) will invoke:

  build_cpugroup_mask()
    topology_cluster_cpumask(cpu)
      cpu_clustergroup_mask(cpu)
        cpu_l2c_shared_mask(cpu)
          per_cpu(cpu_l2c_shared_map, cpu)

Can this access an uninitialized per-CPU offset and dereference a NULL
cpumask_var_t pointer under CONFIG_CPUMASK_OFFSTACK=y (or set a
non-possible CPU bit in ifsd->grp_cpumask when ifsd->all_lp_join is false,
which later sysfs readers in this series pass to
per_cpu_ptr(ifsd->result_ptr, lcpu))?

Additionally, when a valid possible CPU is offline, remove_siblinginfo()
clears cpu_l2c_shared_mask(cpu). If userspace attempts to run a test on an
offline CPU and do_core_test() returns -EINVAL, won't calling
build_cpugroup_mask() beforehand overwrite ifsd->grp_cpumask with an empty
mask (when ifsd->all_lp_join is true) or with the untested offline CPU
(when ifsd->all_lp_join is false), clobbering the previous test's CPU
group state exposed by cpus_ran_list, status_list, details_list, and
addnl_details_list added in the subsequent patches?

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