Re: [PATCH v8 5/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
issues to consider:
- [Medium] selftests/breakpoints: 8-byte write into unsigned long var on
  32-bit riscv
- [Medium] selftests/breakpoints: icount_branch_result modified only via
  raw asm symbol
- [Low] selftests/breakpoints: format string mismatch for __u64 in
  breakpoint_test_riscv
- [Low] selftests/breakpoints: missing kselftest TAP reporting, skip
  handling, and changelog mismatch
- [Low] selftests/breakpoints: commit message out of sync with diff
  changes
- [Low] selftests/breakpoints: icount_branch_result invisible to C
  optimizer
- [Low] selftests/breakpoints: close(gfd) before wait_event() in
  trigger_wp() and trigger_bp()
--

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

> selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints
>
> Cover the new ptrace-based hardware breakpoint/watchpoint support
> added for riscv: exercise the PTRACE_GETREGSET/SETREGSET regset path
> as well as the raw PTRACE_GETHBPREGS/SETHBPREGS interface, alongside
> the existing perf_event-based tests.
>
> Also drop a leftover #if 0 block duplicating the HWDEBUG_* control
> field macros already defined in uapi/asm/ptrace.h.
>
> Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>

[Severity: Low]
Is this commit message out of sync with the changes in the diff?  The parent
commit "riscv: Add breakpoint and watchpoint test for riscv" has no #if 0
block with duplicate HWDEBUG_* macros, while the new perf_event taken-branch
PC test (trigger_icount_branch()) and the close(gfd) fix in trigger_wp() are
not mentioned.

> diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> index a8a94a788142d..b07f6770c380e 100644
> --- a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> +++ b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c

[ ... ]

> +		if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_HW_WATCH, &iov) == 0) {
> +			ksft_print_msg("ptrace(PTRACE_GETREGSET): Number of watchpoints: %u\n",
> +				       dreg_state.info);
> +			ksft_print_msg("ptrace(PTRACE_GETREGSET): addr: 0x%lx control: 0x%x\n",
> +				       dreg_state.dbg_regs[0].addr,
> +				       dreg_state.dbg_regs[0].control);
> +			if (!dreg_state.info ||
> +			    dreg_state.dbg_regs[0].addr != (uintptr_t)addr ||
> +			    HWDEBUG_TYPE(dreg_state.dbg_regs[0].control) != HW_BREAKPOINT_W ||
> +			    HWDEBUG_SIZE(dreg_state.dbg_regs[0].control) != size) {
> +				ksft_print_msg("Bad wp readback: info=%u addr=0x%lx ctrl=0x%x\n",
> +					       dreg_state.info,
> +					       dreg_state.dbg_regs[0].addr,
> +					       dreg_state.dbg_regs[0].control);
> +				return false;
> +			}

[Severity: Low]
Should these ksft_print_msg() calls use %llx and %llu instead of %lx and
%lu?  In arch/riscv/include/uapi/asm/ptrace.h,
struct user_hwdebug_state::dbg_regs[0].addr and
struct __riscv_hwdebug_state::{addr, type, len, ctrl} (printed in
set_hbpregs_watchpoint() and set_hbpregs_breakpoint()) are __u64, which is
unsigned long long on both 32-bit and 64-bit RISC-V, causing -Wformat
warnings and corrupting variadic argument reads on 32-bit RISC-V.

[ ... ]

> +static bool run_ptrace_wp_test(void)
> +{
> +	pid_t pid = fork();
> +	bool ret = false;
> +
> +	if (pid < 0) {
> +		ksft_print_msg("fork() failed: %s\n", strerror(errno));
> +		return false;
> +	}
> +
> +	if (pid == 0)
> +		do_wp_child(&var, 8);
> +
> +	if (!wait_for_child_stop(pid, SIGSTOP, "ptrace watchpoint"))
> +		goto out;
> +
> +	if (!set_watchpoint(pid, 8))
> +		goto out;

[Severity: Medium]
Can do_wp_child(&var, 8) write past the end of var on 32-bit RISC-V?

var is declared as unsigned long (4 bytes on RV32), and
tools/testing/selftests/breakpoints/Makefile builds breakpoint_test_riscv for
both riscv and riscv64.  Hardcoding 8 here and in set_hbpregs_watchpoint() /
run_hbpregs_wp_test() instead of sizeof(var) executes *(uint64_t *)addr = 47
in do_wp_child(), overflowing var in .bss and requesting HW_BREAKPOINT_LEN_8
on 32-bit kernels where only lengths up to 4 bytes are supported.

[ ... ]

> +static int icount_branch_result;
> +static unsigned long icount_branch_pc;

[ ... ]

> +static __attribute_noinline__ void run_icount_taken_branch(void)
> +{
> +	asm volatile(".option push\n"
> +		".option norvc\n"
> +		".globl icount_branch_insn\n"
> +		".globl icount_branch_fallthrough\n"
> +		".globl icount_branch_target\n"
> +		"li t0, 1\n"
> +		"li t1, 1\n"
> +		".balign 4\n"
> +		"icount_branch_insn:\n"
> +		"beq t0, t1, icount_branch_target\n"
> +		"icount_branch_fallthrough:\n"
> +		"la t2, icount_branch_result\n"
> +		"li t3, 2\n"
> +		"sw t3, 0(t2)\n"
> +		"j icount_branch_done\n"
> +		".balign 4\n"
> +		"icount_branch_target:\n"
> +		"la t2, icount_branch_result\n"
> +		"li t3, 1\n"
> +		"sw t3, 0(t2)\n"
> +		"icount_branch_done:\n"
> +		".option pop\n"
> +		:
> +		:
> +		: "t0", "t1", "t2", "t3", "memory");
> +}

[Severity: Low]
Can the compiler optimize away or constant-fold icount_branch_result when
building with optimizations enabled?  Because icount_branch_result is a
static variable whose address is never taken in C and that is only written via
a raw symbol name in the inline assembly rather than an asm output operand,
interprocedural optimization can fold reads of icount_branch_result to 0 and
elide the symbol from .bss.

[ ... ]

> +	if (icount_branch_result != 1) {
> +		ksft_print_msg("Icount branch did not take the branch\n");
> +		return -1;
> +	}
> +
> +	if (icount_branch_pc != (unsigned long)target_addr) {
> +		ksft_print_msg("Icount branch PC 0x%lx does not match target %p\n",
> +			       icount_branch_pc, target_addr);
> +		return -1;
> +	}

[Severity: Medium]
Does trigger_icount_branch() fail at this point in the series and prevent
run_ptrace_tests() and run_hbpregs_tests() from running?

Before the later commit "riscv: Add software supported single stepping with
mc/mc6 triggers" in this series, hw_breakpoint_handler() delivers the perf
breakpoint event at insn_addr without single-stepping to target_addr, so
icount_branch_pc != (unsigned long)target_addr fails and main() returns -1
before reaching the new ptrace tests (in addition to icount_branch_result
being folded to 0 under -O1/-O2 unless marked volatile/__used or passed as an
asm operand).

[ ... ]

>  	ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
>  	test_data = 0xdeadbeef;
>  	ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
>  
> +	close(gfd);
> +
>  	return 0;
>  }

[Severity: Low]
Can closing gfd in trigger_wp() (and trigger_bp()) before main() calls
wait_event() drop the perf fasync signal?

__perf_event_overflow() queues pending_irq via irq_work_queue() without
IRQ_WORK_HARD_IRQ, so on !CONFIG_SMP or CONFIG_PREEMPT_RT kernels kill_fasync()
is deferred until after the breakpoint trap returns to userspace:

trigger_wp()
  test_data = 0xdeadbeef
    <breakpoint trap>
      ...
        __perf_event_overflow()
          irq_work_queue(&event->pending_irq)
  close(gfd)
    __fput()
      perf_fasync(-1, ...)  <-- clears event->fasync before kill_fasync()
main()
  wait_event(&wp_mtx, "Watchpoint")  <-- times out after 5s

Closing gfd after wait_event() returns, as trigger_icount_branch() does,
avoids tearing down fasync while the signal is still pending.

[ ... ]

>  	if (wp_triggered)
> -		printf("Watchpoint test passed!\n");
> +		ksft_print_msg("[perf_event]: Watchpoint test passed!\n");
> +
> +	sem_init(&icount_branch_mtx, 0, 0);
> +	if (trigger_icount_branch() < 0)
> +		return -1;
> +
> +	if (!run_ptrace_tests())
> +		return -1;
> +	if (!run_hbpregs_tests())
> +		return -1;
>  
>  	return 0;
>  }

[Severity: Low]
Should main() report results through the kselftest harness and skip when
hardware breakpoints are unsupported?

Although this patch includes kselftest.h and converts several prints to
ksft_print_msg(), main() never calls ksft_print_header(), ksft_set_plan(),
ksft_test_result_*(), or ksft_exit_*(), and exits with a bare return -1 (255).
In addition, when set_watchpoint() or set_breakpoint() sees errno == EIO ("not
supported") on platforms without hardware debug triggers, the test fails
instead of reporting a kselftest skip.

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