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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] Physical address truncation when passing `attr_buf` to SBI on RV32.
- [Medium] Memory allocation inside a busy-wait loop in `sse_test_wait_not_running()`.
- [Medium] `sse_test_injection_fast()` ignores injection errors and can hang the test suite for over 16 minutes.
- [Medium] Background cleanup worker loops infinitely and permanently leaks the module refcount if a queued event is already disabled.
- [Medium] A delayed SSE event handler can overwrite `fast_test_arg.completion` in a subsequent test iteration, causing false passes.
--

commit c7a4c7b5432c658881e0fc3e7c6fa179158e9592
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
> index 0000000000000..cc5c2e46f2fdb 100644
> --- /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: Medium]
Will this cleanup worker loop infinitely if a queued event is already
disabled? Since sse_event_disable() will return an error (SBI_ERR_INVALID_STATE)
when called on an already disabled event, the unregister call is skipped,
retry is set to true, and this background worker reschedules itself
permanently, leaking the module refcount.

> +
> +		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: Medium]
Is the physical address safely cast here? On RV32 with >4GB physical RAM
(CONFIG_PHYS_ADDR_T_64BIT enabled), virt_to_phys() returns a 64-bit value,
which gets truncated to 32 bits by the (unsigned long) cast. The firmware
will receive a truncated address, potentially causing memory corruption.

> +	if (sret.error)
> +		return sbi_err_map_linux_errno(sret.error);
> +
> +	return 0;
> +}
> +
> +static int sse_event_attr_get(u32 evt, unsigned long attr_id,
> +			      unsigned long *val)
> +{
> +	unsigned long *attr_buf;
> +	int ret;
> +
> +	attr_buf = kmalloc_obj(*attr_buf, GFP_KERNEL);
> +	if (!attr_buf)
> +		return -ENOMEM;
> +
> +	ret = sse_event_attr_read(evt, attr_id, attr_buf);
> +	if (!ret)
> +		*val = *attr_buf;
> +	kfree(attr_buf);
> +
> +	return ret;
> +}

[ ... ]

> +static int sse_test_wait_not_running(u32 evt)
> +{
> +	unsigned long timeout = jiffies + HZ;
> +	unsigned long status;
> +	int ret;
> +
> +	do {
> +		ret = sse_event_attr_get(evt, SBI_SSE_ATTR_STATUS, &status);

[Severity: Medium]
Does this code thrash the allocator? sse_event_attr_get() calls kmalloc_obj()
and kfree() on each invocation. This loop repeatedly allocates and frees
memory while busy-waiting in a tight spin loop until the status changes
or a timeout is reached, which could cause massive memory traffic.

> +		if (ret) {
> +			sse_err("Failed to get status for evt %x, error %d\n", evt, ret);
> +			return ret;
> +		}
> +		status &= SBI_SSE_ATTR_STATUS_STATE_MASK;
> +		cpu_relax();
> +	} while (status == SBI_SSE_STATE_RUNNING && time_before(jiffies, timeout));
> +
> +	if (status == SBI_SSE_STATE_RUNNING) {
> +		sse_err("Timed out waiting for event %x to leave RUNNING state\n", evt);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int sse_test_handler(u32 evt, void *arg, struct pt_regs *regs)
> +{
> +	int ret = 0;
> +	struct fast_test_arg *targ = arg;
> +	u32 test_evt;
> +	int cpu;
> +
> +	atomic_inc(&sse_test_handler_count);
> +
> +	/* Pairs with the argument publication in sse_run_fast_test_cpu(). */
> +	if (!smp_load_acquire(&targ->args_ready)) {
> +		sse_err("Received SSE event %x before its test arguments were published\n",
> +			evt);
> +		ret = -EINVAL;
> +		goto complete;
> +	}
> +
> +	test_evt = READ_ONCE(targ->evt);
> +	cpu = READ_ONCE(targ->cpu);
> +
> +	if (evt != test_evt) {
> +		sse_err("Received SSE event id %x instead of %x\n", test_evt, evt);
> +		ret = -EINVAL;
> +	}
> +
> +	if (!sse_event_is_global(evt) && cpu != smp_processor_id()) {
> +		sse_err("Received SSE event %d on CPU %d instead of %d\n", evt, smp_processor_id(),
> +			cpu);
> +		ret = -EINVAL;
> +	}
> +
> +complete:
> +	WRITE_ONCE(targ->args_ready, false);
> +	/* Publish handler-side checks before waking the waiting CPU. */
> +	smp_store_release(&targ->completion, true);

[Severity: Medium]
Could a delayed handler from a previous timed-out test iteration execute
here and overwrite the completion flag for a newly started test iteration?
When sse_run_fast_test_cpu() times out, it abandons the pending hardware
event. If it fires later, it can cause the next loop iteration to falsely
report success.

> +
> +	return ret;
> +}
> +
> +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;
> +	/* 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;
> +	}
> +}

[ ... ]

> +		for (j = 0; j < RUN_LOOP_COUNT; j++)
> +			sse_run_fast_test(&fast_test_arg, event, evt);

[Severity: Medium]
Will this loop unconditionally execute 1000 times regardless of whether
the internal test execution timed out or failed? Because sse_run_fast_test()
returns void, any errors like -ETIMEDOUT from sse_run_fast_test_cpu()
are dropped, and the test suite could hang for over 16 minutes blindly
looping if an event delivery consistently fails.

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

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