Re: [PATCH v10 RESEND 8/9] selftests/riscv: add SSE test module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] riscv/sse: truncating physical address in sse_event_attr_read() causes memory corruption on RV32
- [High] riscv/sse: unhandled timeout in sse_run_fast_test() leaves asynchronous firmware event pending
- [High] riscv/sse: cleanup work loop deadlocks because sse_event_disable() on disabled event fails
- [Medium] riscv/sse: HZ/100 timeout calculation causes guaranteed immediate timeout when HZ < 100
--

commit a979d907b51d143e2d5c8ad22d308f8c21385e6f
Author: Clément Léger <cleger@rivosinc.com>

    selftests/riscv: add SSE test module
    
    Add an SSE selftest module and runner. Loading the module executes
    smoke tests for the SSE framework, and the runner reports any error
    emitted by the module.

> diff --git a/tools/testing/selftests/riscv/sse/module/riscv_sse_test.c b/tools/testing/selftests/riscv/sse/module/riscv_sse_test.c
> new file mode 100644
> index 0000000000000..cc5c2e46f2fdb
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/sse/module/riscv_sse_test.c
[ ... ]
> +static void sse_test_cleanup_workfn(struct work_struct *work)
> +{
> +	bool retry = false;
> +	int i, ret;
> +
> +	mutex_lock(&sse_test_cleanup_lock);
> +	for (i = 0; i < ARRAY_SIZE(sse_test_cleanup_events); i++) {
> +		struct sse_event *event = sse_test_cleanup_events[i];
> +
> +		if (!event)
> +			continue;
> +
> +		ret = sse_event_disable(event);
> +		if (!ret)
> +			ret = sse_event_unregister(event);
> +		if (ret) {
> +			retry = true;
> +			continue;
> +		}

[Severity: High]
Does this code introduce an infinite loop during cleanup? If sse_event_disable()
is called on an already-disabled event, it may return an error like
SBI_ERR_INVALID_STATE. If ret is set, this check will skip
sse_event_unregister(), set retry = true, and permanently reschedule the work,
deadlocking the cleanup work queue.

> +
> +		sse_test_cleanup_events[i] = NULL;
> +		module_put(THIS_MODULE);
> +	}
> +	mutex_unlock(&sse_test_cleanup_lock);
> +
> +	if (retry)
> +		mod_delayed_work(system_wq, &sse_test_cleanup_work,
> +				 msecs_to_jiffies(100));
> +}
[ ... ]
> +static int sse_event_attr_read(u32 evt, unsigned long attr_id,
> +			       unsigned long *attr_buf)
> +{
> +	struct sbiret sret;
> +	phys_addr_t phys;
> +
> +	phys = virt_to_phys(attr_buf);
> +
> +	sret = sbi_ecall(SBI_EXT_SSE, SBI_SSE_EVENT_ATTR_READ, evt, attr_id, 1,
> +			 (unsigned long)phys, 0, 0);

[Severity: High]
Can this corrupt memory on RV32 systems? The function casts phys_addr_t phys
to unsigned long, which truncates it to 32 bits on RV32. Since 0 is passed for
arg4 (phys_hi), the SBI firmware will write attributes to the truncated physical
address, potentially corrupting arbitrary memory below 4GB instead of
modifying the intended buffer.

> +	if (sret.error)
> +		return sbi_err_map_linux_errno(sret.error);
> +
> +	return 0;
> +}
[ ... ]
> +static int sse_run_fast_test_cpu(struct fast_test_arg *test_arg,
> +				 struct sse_event *event, u32 evt, int cpu)
> +{
> +	unsigned long timeout;
> +	int ret;
> +
> +	WRITE_ONCE(test_arg->completion, false);
> +	WRITE_ONCE(test_arg->args_ready, false);
> +	WRITE_ONCE(test_arg->evt, evt);
> +	WRITE_ONCE(test_arg->cpu, cpu);
> +	/* Publish all arguments before firmware can inject on another hart. */
> +	smp_store_release(&test_arg->args_ready, true);
> +
> +	ret = sse_test_inject_event(event, evt, cpu);
> +	if (ret) {
> +		sse_err("event %s injection failed, err %d\n",
> +			sse_evt_name(evt), ret);
> +		return ret;
> +	}
> +
> +	timeout = jiffies + HZ / 100;

[Severity: Medium]
Will this evaluate to 0 if HZ < 100? If the kernel is configured with a low
HZ, HZ / 100 is 0. This causes the subsequent time_before() check to
immediately fail before the event can be handled, resulting in false test
failures.

> +	/* We can not use <linux/completion.h> since they are not NMI safe */
> +	/* Pairs with the handler's completion publication. */
> +	while (!smp_load_acquire(&test_arg->completion) &&
> +	       time_before(jiffies, timeout))
> +		cpu_relax();
> +	/* Acquire the handler's checks even if the loop observed a timeout. */
> +	if (!smp_load_acquire(&test_arg->completion)) {
> +		sse_err("Failed to wait for event %s completion on CPU %d\n",
> +			sse_evt_name(evt), cpu);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return sse_test_wait_not_running_on_cpu(evt, cpu);
> +}
> +
> +static void sse_run_fast_test(struct fast_test_arg *test_arg,
> +			      struct sse_event *event, u32 evt)
> +{
> +	int cpu;
> +
> +	if (sse_event_is_global(evt)) {
> +		/* Keep the selected target online through injection and completion. */
> +		cpu_hotplug_disable();
> +		for_each_online_cpu(cpu) {
> +			if (sse_run_fast_test_cpu(test_arg, event, evt, cpu))
> +				break;
> +		}
> +		cpu_hotplug_enable();
> +		return;
> +	}
> +
> +	guard(cpus_read_lock)();
> +	for_each_online_cpu(cpu) {
> +		if (sse_run_fast_test_cpu(test_arg, event, evt, cpu))
> +			return;
> +	}
> +}
> +
> +static void sse_test_injection_fast(void)
> +{
[ ... ]
> +		pr_info("Starting testing event %s\n", sse_evt_name(evt));
> +
> +		for (j = 0; j < RUN_LOOP_COUNT; j++)
> +			sse_run_fast_test(&fast_test_arg, event, evt);

[Severity: High]
Does this code ignore unhandled timeouts? If sse_run_fast_test_cpu() times out
and returns -ETIMEDOUT, the error is swallowed because sse_run_fast_test()
returns void. This loop immediately fires the next iteration, overwriting
fast_test_arg while an asynchronous firmware event may still be pending from
the previous loop, potentially causing a data race on the global test state.

> +		pr_info("Finished testing event %s\n", sse_evt_name(evt));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789974241.git.zhangzhanpeng.jasper@bytedance.com?part=8