Re: [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] riscv: ptrace: PTRACE_GETHBPREGS/SETHBPREGS collide with
  generic SUD requests
- [High] riscv: hw_breakpoint: duplicate flush_ptrace_hw_breakpoint()
  stub breaks build
- [Medium] riscv: ptrace: switching slot type in riscv_ptrace_bp_set()
  fails with -ENOSPC
- [Medium] riscv: ptrace: sleeping lock in riscv_ptrace_hbptriggered()
  with IRQs off
- [Low] riscv: unused clear_ptrace_hw_breakpoint() prototype and
  debug_info fields
--

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

riscv: add ptrace requests for hardware breakpoints

This commit adds support for the PTRACE_GETHBPREGS and PTRACE_SETHBPREGS
ptrace requests on RISC-V, storing per-task perf hardware breakpoint events
and reporting hits via SIGTRAP/TRAP_HWBKPT.

> diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
> index dcce20ba3faab..6e8913d69df23 100644
> --- a/arch/riscv/include/asm/hw_breakpoint.h
> +++ b/arch/riscv/include/asm/hw_breakpoint.h
[ ... ]
> @@ -42,8 +49,14 @@ void arch_disable_hw_breakpoint(struct perf_event *bp);
>  int arch_install_hw_breakpoint(struct perf_event *bp);
>  void arch_uninstall_hw_breakpoint(struct perf_event *bp);
>  void hw_breakpoint_pmu_read(struct perf_event *bp);
> +void clear_ptrace_hw_breakpoint(struct task_struct *tsk);
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
> +void ptrace_hw_copy_thread(struct task_struct *task);
>  
>  #else
>  
> +static inline void flush_ptrace_hw_breakpoint(struct task_struct *task) { }
> +static inline void ptrace_hw_copy_thread(struct task_struct *task) { }
> +
>  #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  #endif /* __RISCV_HW_BREAKPOINT_H */

[Severity: High]
Does defining flush_ptrace_hw_breakpoint() here break the build when
CONFIG_HAVE_HW_BREAKPOINT is disabled?

include/linux/hw_breakpoint.h already defines a static inline stub for
flush_ptrace_hw_breakpoint() when CONFIG_HAVE_HW_BREAKPOINT is not set
(and declares the prototype when it is set).  Since asm/processor.h now
includes asm/hw_breakpoint.h, any translation unit including both
linux/sched.h and linux/hw_breakpoint.h (such as arch/riscv/kernel/ptrace.c)
fails to compile due to the redefinition.

> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index e9d339bcb7e9f..a124ea48d8058 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
[ ... ]
> @@ -111,6 +112,19 @@ struct pt_regs;
>  #define RISCV_PREEMPT_V_NEED_RESTORE	0x40000000
>  #define RISCV_PREEMPT_V_IN_SCHEDULE	0x20000000
>  
> +struct debug_info {
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	/* Have we suspended stepping by a debugger? */
> +	int			suspended_step;
> +	/* Allow breakpoints and watchpoints to be disabled for this thread. */
> +	int			bp_disabled;
> +	int			wp_disabled;
> +	/* Hardware breakpoints pinned to this task. */
> +	struct perf_event	*hbp_break[RISCV_MAX_BP];
> +	struct perf_event	*hbp_watch[RISCV_MAX_BP];
> +#endif
> +};

[Severity: Low]
This isn't a bug, but suspended_step, bp_disabled, and wp_disabled in
struct debug_info, along with the clear_ptrace_hw_breakpoint() prototype in
asm/hw_breakpoint.h, are never used in this patch or later in the series.

> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
> index 3de2b7124aff3..373a1c14f2073 100644
> --- a/arch/riscv/include/uapi/asm/ptrace.h
> +++ b/arch/riscv/include/uapi/asm/ptrace.h
> @@ -15,6 +15,8 @@
>  
>  #define PTRACE_GETFDPIC_EXEC	0
>  #define PTRACE_GETFDPIC_INTERP	1
> +#define PTRACE_GETHBPREGS	0x4210
> +#define PTRACE_SETHBPREGS	0x4211

[Severity: High]
As reported by Rui Qi on v6 [1], do these values collide with the generic
PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG (0x4210) and
PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG (0x4211) requests in
include/uapi/linux/ptrace.h, where 0x4200-0x4300 is reserved for
architecture-independent additions?

Because arch_ptrace() and compat_arch_ptrace() intercept 0x4210 and 0x4211
before calling ptrace_request(), a tracer calling
PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG when CONFIG_HAVE_HW_BREAKPOINT=y is
routed to riscv_ptrace_gethbpregs(), which overwrites the caller's input
buffer via copy_to_user(), while PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG is
routed to riscv_ptrace_sethbpregs().

> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index a0055743d3ba5..be076865d02d0 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
[ ... ]
> @@ -380,6 +382,152 @@ static int riscv_cfi_set(struct task_struct *target,
>  }
>  #endif
>  
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +enum riscv_hbp_type {
> +	RISCV_HBP_BREAK,
> +	RISCV_HBP_WATCH,
> +};
> +
> +/*
> + * Handle hitting a HW-breakpoint.
> + */
> +static void riscv_ptrace_hbptriggered(struct perf_event *bp,
> +				      struct perf_sample_data *data,
> +				      struct pt_regs *regs)
> +{
> +	struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
> +
> +	force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)bkpt->address);
> +}

[Severity: Medium]
Can calling force_sig_fault() from riscv_ptrace_hbptriggered() sleep with
interrupts disabled on PREEMPT_RT?

When a breakpoint triggers, hw_breakpoint_handler() -> perf_bp_event() ->
perf_swevent_event() disables hardware interrupts via guard(irqsave)()
before invoking the overflow handler:

  perf_swevent_event()              [guard(irqsave)()]
    -> perf_swevent_overflow()
      -> __perf_event_overflow()
        -> riscv_ptrace_hbptriggered()
          -> force_sig_fault()
            -> force_sig_info_to_task()
              -> spin_lock_irqsave(&t->sighand->siglock, flags)

On PREEMPT_RT (RISC-V selects ARCH_SUPPORTS_RT), sighand->siglock is a
sleeping rt_spin_lock, so acquiring it with interrupts disabled triggers a
sleeping-in-atomic warning and blocks with IRQs off if contended.

[ ... ]

> @@ -549,12 +697,173 @@ void ptrace_disable(struct task_struct *child)
[ ... ]
> +static int riscv_ptrace_bp_set(struct task_struct *child, unsigned long idx,
> +			       struct __riscv_hwdebug_state *state)
> +{
> +	struct perf_event *bp;
> +	struct perf_event_attr attr;
> +	enum riscv_hbp_type hbp_type, other_hbp_type;
> +	int ret;
> +	bool created = false;
> +
> +	if (idx >= RISCV_MAX_BP)
> +		return -EINVAL;
> +
> +	idx = array_index_nospec(idx, RISCV_MAX_BP);
> +
> +	ret = riscv_ptrace_bp_type_to_hbp(state->type, &hbp_type);
> +	if (ret)
> +		return ret;
> +
> +	/* The legacy request exposes ctrl as a disabled/enabled boolean. */
> +	if (state->ctrl > 1)
> +		return -EINVAL;
> +
> +	other_hbp_type = hbp_type == RISCV_HBP_BREAK ?
> +			 RISCV_HBP_WATCH : RISCV_HBP_BREAK;
> +
> +	bp = ptrace_hbp_get_event(hbp_type, child, idx);
> +	if (IS_ERR(bp))
> +		return PTR_ERR(bp);
> +	if (!bp) {
> +		bp = ptrace_hbp_create(hbp_type, child, idx);
> +		if (IS_ERR(bp))
> +			return PTR_ERR(bp);
> +		created = true;
> +	}
> +
> +	attr = bp->attr;
> +
> +	attr.bp_addr = state->addr;
> +	attr.bp_len  = state->len;
> +	attr.bp_type = state->type;
> +	/* Enable or disable as requested by ctrl (0 = enabled, 1 = disabled) */
> +	attr.disabled = state->ctrl == 1;
> +	ret = modify_user_hw_breakpoint(bp, &attr);
> +	if (ret) {
> +		/* Drop only events allocated by this failed request. */
> +		if (created)
> +			riscv_ptrace_bp_clear_event(child, hbp_type, idx);
> +		return ret;
> +	}
> +
> +	/*
> +	 * If the same index changed from breakpoint to watchpoint, or back,
> +	 * clear the old opposite-type slot after the new event is valid so a
> +	 * failed update does not destroy the previous state.
> +	 */
> +	return riscv_ptrace_bp_clear_event(child, other_hbp_type, idx);
> +}

[Severity: Medium]
Can switching an existing slot between a breakpoint and a watchpoint fail
with -ENOSPC when all hardware triggers are already in use?

RISC-V selects CONFIG_HAVE_MIXED_BREAKPOINTS_REGS, so breakpoints and
watchpoints share a single pool of dbtr_total_num slots in
reserve_bp_slot().  Because debug_info splits storage into hbp_break[] and
hbp_watch[], changing the type at index idx causes ptrace_hbp_get_event()
to return NULL and ptrace_hbp_create() to allocate a second perf_event
from the shared pool before riscv_ptrace_bp_clear_event() frees the
existing event at the same index.  Modifying the existing perf_event in
place via modify_user_hw_breakpoint() (or storing unified trigger slots in
a single array) would avoid needing a spare hardware slot.

[1] https://lore.kernel.org/r/5a7c08bd-4313-4424-b649-a19ffe5242e4@bytedance.com

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