Re: [PATCH v12 11/11] selftests/riscv: add perf user-stack SSE copy regression test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/riscv: sse_perf_ustack fails with EINVAL instead of skipping without PMU IRQ/SSE
- [Medium] selftests/riscv: stress_worker() overflows default pthread stack on musl in burn_stack()
- [Medium] selftests/riscv: sse_perf_ustack accepts 0-byte ustack copy as truncated
--

commit 5a3dc623890401f5832b1d92fb8db20380606649
Author: Zhanpeng Zhang <zhangzhanpeng.jasper@bytedance.com>

selftests/riscv: add perf user-stack SSE copy regression test

This commit adds a RISC-V SSE selftest exercising PERF_SAMPLE_STACK_USER
raw user-stack copying under PMU overflow sampling. It checks page-boundary
truncation against a PROT_NONE guard page and runs a multi-CPU stress
workload combining unix sockets and deep stack recursion.

> diff --git a/tools/testing/selftests/riscv/sse/sse_perf_ustack.c b/tools/testing/selftests/riscv/sse/sse_perf_ustack.c
> new file mode 100644
> index 0000000000000..9535b6d7ba4c1
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/sse/sse_perf_ustack.c
[ ... ]
> +static struct perf_event_attr sampling_attr(unsigned long freq)
> +{
> +	struct perf_event_attr attr = {
> +		.type		= PERF_TYPE_HARDWARE,
> +		.size		= sizeof(attr),
> +		.config		= PERF_COUNT_HW_INSTRUCTIONS,
> +		.sample_type	= PERF_SAMPLE_STACK_USER,
> +		.sample_stack_user = STACK_DUMP_SIZE,
> +		.freq		= 1,
> +		.sample_freq	= freq,
> +		.disabled	= 1,
> +		.exclude_kernel	= 1,
> +		.exclude_hv	= 1,
> +	};
> +
> +	return attr;
> +}
> +
> +static bool open_skip_reason(int err, const char **why)
> +{
> +	switch (err) {
> +	case EACCES:
> +	case EPERM:
> +		*why = "insufficient privilege for PMU sampling (perf_event_paranoid)";
> +		return true;
> +	case ENOENT:
> +	case ENODEV:
> +	case EOPNOTSUPP:
> +		*why = "hardware PMU sampling event not available";
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

[Severity: Medium]
When running on a RISC-V system where neither SBI SSE nor Sscofpmf IRQ
overflow delivery is supported, will test_ustack_records_wellformed() fail
with EINVAL instead of skipping?

In that configuration, no riscv-pmu interrupt is registered in
/proc/interrupts, so pmu_sse_route_testable() returns true, and
pmu_sbi_device_probe() sets both PERF_PMU_CAP_NO_INTERRUPT and
PERF_PMU_CAP_NO_EXCLUDE:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_device_probe() {
    ...
	ret = pmu_sbi_setup_irqs(pmu, pdev);
	if (ret < 0) {
		pr_info("Perf sampling/filtering is not supported as sscof extension is not available\n");
		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE;
	}
    ...
}

Because sampling_attr() sets exclude_kernel and exclude_hv,
perf_event_open() -> perf_event_alloc() -> perf_init_event() ->
perf_try_init_event() rejects the event with -EINVAL before
perf_event_open() reaches the PERF_PMU_CAP_NO_INTERRUPT check that returns
-EOPNOTSUPP:

kernel/events/core.c:perf_try_init_event() {
    ...
	if (pmu->capabilities & PERF_PMU_CAP_NO_EXCLUDE &&
	    event_has_any_exclude_flag(event)) {
		ret = -EINVAL;
		goto err_destroy;
	}
    ...
}

Since open_skip_reason() does not match EINVAL, test_ustack_records_wellformed()
calls ksft_test_result_fail() rather than ksft_test_result_skip().

[ ... ]
> +static int truncation_child(void *arg)
> +{
> +	int ready_fd = (intptr_t)arg;
> +	char ready = 1;
> +
> +	if (write(ready_fd, &ready, sizeof(ready)) != 1)
> +		return 1;
> +
> +	for (;;)
> +		__asm__ __volatile__("" : : : "memory");
> +}
[ ... ]
> +static void test_ustack_records_wellformed(void)
> +{
[ ... ]
> +					if (dyn_size > dump_size) {
> +						ok = false;
> +						break;
> +					}
> +					if (dyn_size < dump_size)
> +						truncated++;
> +				}
> +			}

[Severity: Medium]
Can test_ustack_records_wellformed() pass when every sample copies 0 bytes
(dyn_size == 0) instead of performing a partial copy up to the guard page?

start_truncation_child() passes stack + page_size to clone(), where the page
at stack + page_size is mapped PROT_NONE. Inside truncation_child(), the
stack frame only holds ready_fd and ready before entering the spin loop,
leaving the user stack pointer within 16 to 48 bytes of the PROT_NONE page
boundary.

When perf_output_sample_ustack() copies the user stack via
__output_copy_user() -> riscv_perf_out_copy_user() ->
__copy_from_user_inatomic(), fallback_scalar_usercopy_sum_enabled() enters
.Lword_copy and issues 8 unrolled word loads (64 bytes on RV64) before
storing any bytes or advancing dst (a0):

arch/riscv/lib/uaccess.S:fallback_scalar_usercopy_sum_enabled() {
    ...
2:
	fixup REG_L   a4,        0(a1), 10f
	fixup REG_L   a5,    SZREG(a1), 10f
	fixup REG_L   a6,  2*SZREG(a1), 10f
	fixup REG_L   a7,  3*SZREG(a1), 10f
	fixup REG_L   t1,  4*SZREG(a1), 10f
	fixup REG_L   t2,  5*SZREG(a1), 10f
	fixup REG_L   t3,  6*SZREG(a1), 10f
	fixup REG_L   t4,  7*SZREG(a1), 10f
    ...
}

Because the stack pointer is closer than 64 bytes to the PROT_NONE page,
one of the initial loads faults and jumps to the fixup label before a0
advances, returning the full requested size as uncopied and producing
dyn_size == 0.

Since test_ustack_records_wellformed() only checks dyn_size < dump_size,
dyn_size == 0 increments truncated and passes the test without verifying
that a non-zero partial stack copy succeeded up to the guard page. Should
truncation_child() keep a live stack frame larger than 64 bytes and require
0 < dyn_size && dyn_size < dump_size?

[ ... ]
> +static void test_sse_stress_no_crash(void)
> +{
[ ... ]
> +		CPU_ZERO(&set);
> +		CPU_SET(cpu, &set);
> +		pthread_attr_init(&thread_attr);
> +		ret = pthread_attr_setaffinity_np(&thread_attr, sizeof(set), &set);
> +		if (!ret)
> +			ret = pthread_create(&st->tid, &thread_attr,
> +					     stress_worker, st);
> +		pthread_attr_destroy(&thread_attr);

[Severity: Medium]
Does stress_worker() overflow the default pthread stack on musl libc when
calling burn_stack(RECURSE_DEPTH, &sink)?

In burn_stack(), each of the 513 active frames (from RECURSE_DEPTH = 512
down to 0) allocates unsigned long frame[32] (256 bytes on RV64) plus
saved registers and keeps the frame live across the recursive call via
keep_alive(frame):

static noinline unsigned long burn_stack(int depth, unsigned long *sink)
{
	unsigned long frame[32];
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(frame); i++)
		frame[i] = (unsigned long)depth * i + *sink;

	if (depth > 0)
		frame[depth & 31] += burn_stack(depth - 1, sink);

This consumes roughly 148 KiB of stack space, which exceeds musl libc's
default pthread stack size of 128 KiB unless pthread_attr_setstacksize() is
used to request a larger stack for thread_attr.

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