Re: [PATCH v7 4/8] selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] selftests: missing _exit(0) causes child to execute parent test logic
- [High] selftests: test failures are silently ignored causing false positive passes
- [Medium] selftests: orphaned stopped child processes on test error paths
- [Medium] selftests: unhandled SIGALRM causes abrupt test termination leaving orphaned tracees
- [Low] selftests: hardware breakpoint readback lacks programmatic assertions
- [Low] selftests: commit message claims removal of code not present in diff
- [Low] selftests: size parameter in set_watchpoint is ignored
--

commit dad9f82e0eab9aaebe66d40b3104c52fdbe939dc
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>

selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints

This commit extends the RISC-V breakpoint selftests to cover the new
ptrace-based hardware breakpoint and watchpoint support. It exercises
both the PTRACE_GETREGSET/SETREGSET and PTRACE_GETHBPREGS/SETHBPREGS
interfaces.

> Also drop a leftover #if 0 block duplicating the HWDEBUG_* control
> field macros already defined in uapi/asm/ptrace.h.

[Severity: Low]
This isn't a bug in the code, but does the provided patch actually
include this removal? Looking at the diff, it doesn't seem to contain
any modifications removing an #if 0 block, so the patch contents might
have desynced from the commit message.

> diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> index 0649940b709e0..13d6b3ff1b6ee 100644
> --- a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> +++ b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
[ ... ]
> +static int do_bp_child(void (*bp_func)(void))
> +{
> +	if (ptrace(PTRACE_TRACEME, 0, NULL, NULL) != 0) {
> +		ksft_print_msg(
> +			       "ptrace(PTRACE_TRACEME) failed: %s\n",
> +			       strerror(errno));
> +		_exit(1);
> +	}
> +
> +	if (raise(SIGSTOP) != 0) {
> +		ksft_print_msg(
> +			       "raise(SIGSTOP) failed: %s\n", strerror(errno));
> +		_exit(1);
> +	}
> +
> +	bp_func();
> +}

[Severity: High]
Does this child process need to terminate with _exit(0) here? Since it
doesn't exit, when the child returns from bp_func(), it will break out
of the child context and begin executing the parent's logic in
run_ptrace_bp_test(). This causes the child to wrongly issue waitpid()
and ptrace() commands on itself.

[ ... ]
> +static bool set_watchpoint(pid_t pid, int size)
> +{
> +	uint8_t *addr = (uint8_t *)&var;
> +	unsigned int control = 0;
> +	struct user_hwdebug_state dreg_state;
> +	struct iovec iov;
> +
> +	/* Write watchpoint */
> +	control = (HW_BREAKPOINT_W << 14) & ((0x7 << 14));
> +	control |= (HW_BREAKPOINT_LEN_8 << 4) & ((0x1f << 4));

[Severity: Low]
Is it intentional that the size parameter passed to set_watchpoint() is
ignored here? The code hardcodes HW_BREAKPOINT_LEN_8 into the control bits,
unconditionally forcing the watchpoint size to 8 bytes regardless of the
caller's request.

[ ... ]
> +static int run_ptrace_wp_test(void)
> +{
[ ... ]
> +	if (!set_watchpoint(pid, 8))
> +		return false;
> +
> +	if (ptrace(PTRACE_CONT, pid, NULL, NULL) < 0) {
> +		ksft_print_msg(
> +			"ptrace(PTRACE_CONT) failed: %s\n",
> +			strerror(errno));
> +		return false;
> +	}

[Severity: Medium]
If the test returns false early here, does it leak the stopped child
process? The child process is already blocked in raise(SIGSTOP), and by
returning early, the test runner bypasses the kill(pid, SIGKILL) cleanup
block at the end of the function, which could leave orphaned tracees
consuming system resources.

> +
> +	alarm(3);
> +	wpid = waitpid(pid, &status, __WALL);

[Severity: Medium]
If waitpid() blocks for 3 seconds (e.g., if the child never hits the
breakpoint), will the resulting SIGALRM abruptly terminate the test
suite? Because no signal handler is registered for SIGALRM, the default
action would kill the test process outright, bypassing test framework
reporting and leaving any traced children orphaned.

[ ... ]
> +static bool set_hbpregs_watchpoint(pid_t pid)
> +{
[ ... ]
> +	/* Read back and verify */
> +	memset(&state, 0, sizeof(state));
> +	if (ptrace(PTRACE_GETHBPREGS, pid, 0, &state) != 0) {
> +		ksft_print_msg(
> +			"ptrace(PTRACE_GETHBPREGS) failed: %s\n",
> +			strerror(errno));
> +		return false;
> +	}
> +
> +	ksft_print_msg(
> +		"[hbpregs] watchpoint readback: addr=0x%lx type=%lu len=%lu ctrl=%lu\n",
> +		state.addr, state.type, state.len, state.ctrl);
> +
> +	return true;
> +}

[Severity: Low]
Should there be a programmatic assertion to validate the readback state?
The code currently prints the state to stdout, but doesn't check if the
returned addr, type, len, and ctrl actually match what was written.
Without a check, regressions in breakpoint state persistence might go
unnoticed by automated tests.

[ ... ]
> +static void run_ptrace_tests(void)
> +{
> +	run_ptrace_bp_test();
> +	run_ptrace_wp_test();
> +}

[Severity: High]
Since run_ptrace_bp_test() and run_ptrace_wp_test() return boolean
success values, does run_ptrace_tests() need to propagate these
results? Dropping them here masks test failures.

> +
> +/*
> + * BREAKPOINT TEST USING PTRACE - END
> + */
> +
[ ... ]
> @@ -213,7 +760,10 @@ int main(int argc, char *argv[])
>  		return -1;
>  
>  	if (wp_triggered)
> -		printf("Watchpoint test passed!\n");
> +		ksft_print_msg("[perf_event]: Watchpoint test passed!\n");
> +
> +	run_ptrace_tests();
> +	run_hbpregs_tests();
>  
>  	return 0;
>  }

[Severity: High]
Does main() need to return a non-zero exit code if the new tests fail?
Currently it unconditionally returns 0 at the end of execution, which
means CI systems will report a successful pass even if the new ptrace or
hbpregs tests completely fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930063919.608911-1-himanshu.chauhan@oss.qualcomm.com?part=4