Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] platform/x86/intel/ifs: premature grp_cpumask update in do_core_test() - [Medium] platform/x86/intel/ifs: unlocked grp_cpumask reads in status_list_show() --
commit bf9eae2e7ab2791a6f3b06b483c7daa541ce8c99 Author: Jithu Joseph <[email protected]> platform/x86/intel/ifs: Add new sysfs interfaces {status, details, cpus_ran}_list This commit adds the status_list, details_list, and cpus_ran_list sysfs attributes to report per-CPU scan statuses, details, and the group of CPUs tested together during an In-Field Scan run. > diff --git a/drivers/platform/x86/intel/ifs/sysfs.c > b/drivers/platform/x86/intel/ifs/sysfs.c > index 287f96bb033ea..82a603cd1fd34 100644 > --- a/drivers/platform/x86/intel/ifs/sysfs.c > +++ b/drivers/platform/x86/intel/ifs/sysfs.c > @@ -135,6 +135,59 @@ static ssize_t image_version_show(struct device *dev, > > static DEVICE_ATTR_RO(image_version); > > +static ssize_t status_list_show(struct device *dev, struct device_attribute > *attr, char *buf) > +{ > + struct ifs_data *ifsd = ifs_get_data(dev); > + struct ifs_test_output *pcpu_state; > + int lcpu, len = 0; > + > + if (cpumask_weight(&ifsd->grp_cpumask) == 0) > + return sysfs_emit(buf, "%s\n", "none"); > + > + for_each_cpu(lcpu, &ifsd->grp_cpumask) { > + pcpu_state = per_cpu_ptr(ifsd->result_ptr, lcpu); > + len += sysfs_emit_at(buf, len, "%s%s", (len > 0) ? " " : "", > + status_msg[pcpu_state->test_result]); > + } > + len += sysfs_emit_at(buf, len, "\n"); > + > + return len; > +} [Severity: High] Can status_list_show(), details_list_show(), and cpus_ran_list_show() report bogus state or dereference invalid per-CPU pointers when a write to run_test fails validation in do_core_test()? In drivers/platform/x86/intel/ifs/runtest.c, run_test_store() only checks cpu < nr_cpu_ids before calling do_core_test(), which unconditionally calls build_cpugroup_mask() before checking cpu_online(cpu) and ifsd->loaded: drivers/platform/x86/intel/ifs/runtest.c:do_core_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); ret = -EINVAL; goto out; } switch (test->test_num) { case IFS_TYPE_SAF: if (!ifsd->loaded) ret = -EPERM; else ifs_test_core(cpu, dev); break; } drivers/platform/x86/intel/ifs/runtest.c:build_cpugroup_mask() { cpumask_clear(&ifsd->grp_cpumask); ... if (!ifsd->all_lp_join) { cpumask_set_cpu(cpu, &ifsd->grp_cpumask); return; } cpumask_copy(&ifsd->grp_cpumask, topology_cluster_cpumask(cpu)); } When do_core_test() fails with -EPERM because !ifsd->loaded, build_cpugroup_mask() has already populated ifsd->grp_cpumask. Reading cpus_ran_list, status_list, and details_list afterwards reports that group as having run (or overwrites the group mask from a prior valid test while legacy status and details keep the old results). Similarly, when an offline or non-possible CPU < nr_cpu_ids is written to run_test: - If ifsd->all_lp_join is true and cpu is offline, topology_cluster_cpumask() is empty, wiping out ifsd->grp_cpumask from any previous valid test run. - If ifsd->all_lp_join is true and cpu is not in cpu_possible_mask on a system with CONFIG_CPUMASK_OFFSTACK=y, topology_cluster_cpumask(cpu) is NULL, causing a NULL pointer dereference in cpumask_copy(). - If ifsd->all_lp_join is false, build_cpugroup_mask() sets the offline or non-possible CPU in ifsd->grp_cpumask, causing status_list_show() and details_list_show() to dereference per_cpu_ptr(ifsd->result_ptr, lcpu) for that CPU. Should do_core_test() validate cpu_online(cpu) and ifsd->loaded before updating ifsd->grp_cpumask in build_cpugroup_mask()? > + > +static DEVICE_ATTR_RO(status_list); > + > +static ssize_t details_list_show(struct device *dev, struct device_attribute > *attr, char *buf) > +{ > + struct ifs_data *ifsd = ifs_get_data(dev); > + struct ifs_test_output *pcpu_state; > + int lcpu, len = 0; > + > + if (cpumask_weight(&ifsd->grp_cpumask) == 0) > + return sysfs_emit(buf, "%s\n", "none"); > + > + for_each_cpu(lcpu, &ifsd->grp_cpumask) { > + pcpu_state = per_cpu_ptr(ifsd->result_ptr, lcpu); > + len += sysfs_emit_at(buf, len, "%s%#llx", (len > 0) ? " " : "", > + pcpu_state->test_details); > + } > + len += sysfs_emit_at(buf, len, "\n"); > + > + return len; > +} > + > +static DEVICE_ATTR_RO(details_list); > + > +static ssize_t cpus_ran_list_show(struct device *dev, struct > device_attribute *attr, char *buf) > +{ > + struct ifs_data *ifsd = ifs_get_data(dev); > + > + if (cpumask_weight(&ifsd->grp_cpumask) == 0) > + return sysfs_emit(buf, "%s\n", "none"); > + > + return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(&ifsd->grp_cpumask)); > +} [Severity: Medium] Should status_list_show(), details_list_show(), and cpus_ran_list_show() serialize with ifs_sem when reading ifsd->grp_cpumask and the per-CPU results? In run_test_store(), test execution is serialized under ifs_sem while do_core_test() -> build_cpugroup_mask() clears and updates ifsd->grp_cpumask and ifs_test_core() updates per-CPU test_details and test_result across chunk retries. Because status_list_show(), details_list_show(), and cpus_ran_list_show() read ifsd->grp_cpumask twice without holding ifs_sem (first in cpumask_weight() and then in for_each_cpu() or cpumask_pr_args()), a concurrent build_cpugroup_mask() clearing ifsd->grp_cpumask between the two reads causes these attributes to emit an empty line ("\n") instead of "none\n". In addition, reading status_list or details_list while a test is in progress can pair the newly updated ifsd->grp_cpumask with stale test_result values from a previous run and intermediate per-chunk test_details. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
