Re: [RFC PATCH 6/6] riscv: ptrace: Add hw breakpoint support

[Deepak Gupta <debug@rivosinc.com>]

On Wed, Jul 23, 2025 at 09:55:25AM -0700, Jesse Taube wrote:
>On Tue, Jul 22, 2025 at 9:18 PM Deepak Gupta <debug@rivosinc.com> wrote:
>>
>> On Tue, Jul 22, 2025 at 10:38:29AM -0700, Jesse Taube wrote:
>> >Add ability to setup hw breakpoints to ptrace. Call defines a new
>> >structure of (ulong[3]){bp_addr, bp_len, bp_type} with
>> >bp_type being one of HW_BREAKPOINT_LEN_X and
>> >bp_len being one of HW_BREAKPOINT_X with a value of
>> >zero dissabling the breakpoint.
>> >
>> >Signed-off-by: Jesse Taube <jesse@rivosinc.com>
>> >---
>> > arch/riscv/include/asm/processor.h   |  4 ++
>> > arch/riscv/include/uapi/asm/ptrace.h |  3 +-
>> > arch/riscv/kernel/hw_breakpoint.c    | 14 ++++-
>> > arch/riscv/kernel/process.c          |  4 ++
>> > arch/riscv/kernel/ptrace.c           | 93 ++++++++++++++++++++++++++++
>> > 5 files changed, 116 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
>> >index 5f56eb9d114a..488d956a951f 100644
>> >--- a/arch/riscv/include/asm/processor.h
>> >+++ b/arch/riscv/include/asm/processor.h
>> >@@ -12,6 +12,7 @@
>> >
>> > #include <vdso/processor.h>
>> >
>> >+#include <asm/hw_breakpoint.h>
>> > #include <asm/ptrace.h>
>> >
>> > #define arch_get_mmap_end(addr, len, flags)                   \
>> >@@ -108,6 +109,9 @@ struct thread_struct {
>> >       struct __riscv_v_ext_state vstate;
>> >       unsigned long align_ctl;
>> >       struct __riscv_v_ext_state kernel_vstate;
>> >+#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> >+      struct perf_event *ptrace_bps[RV_MAX_TRIGGERS];
>> >+#endif
>> > #ifdef CONFIG_SMP
>> >       /* Flush the icache on migration */
>> >       bool force_icache_flush;
>> >diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
>> >index a38268b19c3d..a7998ed41913 100644
>> >--- a/arch/riscv/include/uapi/asm/ptrace.h
>> >+++ b/arch/riscv/include/uapi/asm/ptrace.h
>> >@@ -14,7 +14,8 @@
>> >
>> > #define PTRACE_GETFDPIC_EXEC  0
>> > #define PTRACE_GETFDPIC_INTERP        1
>> >-
>> >+#define PTRACE_GETHBPREGS     2
>> >+#define PTRACE_SETHBPREGS     3
>>
>> Why not use `PTRACE_GETREGSET` `PTRACE_SETREGSET` ?
>
>Because it was easier to implement this first, and REGSET will be
>another commit ontop of this one.
>Unless there is a reason to not have this version.
>

Yeah I don't have strong feelings on this. Whatever is best for the tools
(gdb, etc) who will consume this interface, lets do that.

>>
>> > /*
>> >  * User-mode register state for core dumps, ptrace, sigcontext
>> >  *
>> >diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
>> >index 437fd82b9590..c58145464539 100644
>> >--- a/arch/riscv/kernel/hw_breakpoint.c
>> >+++ b/arch/riscv/kernel/hw_breakpoint.c
>> >@@ -633,7 +633,19 @@ void arch_uninstall_hw_breakpoint(struct perf_event *event)
>> >               pr_warn("%s: Failed to uninstall trigger %d. error: %ld\n", __func__, i, ret.error);
>> > }
>> >
>> >-void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { }
>> >+/*
>> >+ * Release the user breakpoints used by ptrace
>> >+ */
>> >+void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>> >+{
>> >+      int i;
>> >+      struct thread_struct *t = &tsk->thread;
>> >+
>> >+      for (i = 0; i < dbtr_total_num; i++) {
>> >+              unregister_hw_breakpoint(t->ptrace_bps[i]);
>> >+              t->ptrace_bps[i] = NULL;
>> >+      }
>> >+}
>> >
>> > void hw_breakpoint_pmu_read(struct perf_event *bp) { }
>> >
>> >diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
>> >index 15d8f75902f8..9cf07ecfb523 100644
>> >--- a/arch/riscv/kernel/process.c
>> >+++ b/arch/riscv/kernel/process.c
>> >@@ -9,6 +9,7 @@
>> >
>> > #include <linux/bitfield.h>
>> > #include <linux/cpu.h>
>> >+#include <linux/hw_breakpoint.h>
>> > #include <linux/kernel.h>
>> > #include <linux/sched.h>
>> > #include <linux/sched/debug.h>
>> >@@ -164,6 +165,7 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
>> >
>> > void flush_thread(void)
>> > {
>> >+      flush_ptrace_hw_breakpoint(current);
>> > #ifdef CONFIG_FPU
>> >       /*
>> >        * Reset FPU state and context
>> >@@ -218,6 +220,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
>> >               set_bit(MM_CONTEXT_LOCK_PMLEN, &p->mm->context.flags);
>> >
>> >       memset(&p->thread.s, 0, sizeof(p->thread.s));
>> >+      if (IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT))
>> >+              memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
>> >
>> >       /* p->thread holds context to be restored by __switch_to() */
>> >       if (unlikely(args->fn)) {
>> >diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
>> >index ea67e9fb7a58..b78cfb0f1c0e 100644
>> >--- a/arch/riscv/kernel/ptrace.c
>> >+++ b/arch/riscv/kernel/ptrace.c
>> >@@ -9,11 +9,13 @@
>> >
>> > #include <asm/vector.h>
>> > #include <asm/ptrace.h>
>> >+#include <asm/hw_breakpoint.h>
>> > #include <asm/syscall.h>
>> > #include <asm/thread_info.h>
>> > #include <asm/switch_to.h>
>> > #include <linux/audit.h>
>> > #include <linux/compat.h>
>> >+#include <linux/hw_breakpoint.h>
>> > #include <linux/ptrace.h>
>> > #include <linux/elf.h>
>> > #include <linux/regset.h>
>> >@@ -336,12 +338,103 @@ void ptrace_disable(struct task_struct *child)
>> > {
>> > }
>> >
>> >+#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> >+static void ptrace_hbptriggered(struct perf_event *bp,
>> >+                              struct perf_sample_data *data,
>> >+                              struct pt_regs *regs)
>> >+{
>> >+      struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
>> >+      int num = 0;
>> >+
>> >+      force_sig_ptrace_errno_trap(num, (void __user *)bkpt->address);
>> >+}
>> >+
>> >+/*
>> >+ * idx selects the breakpoint index.
>> >+ * Both PTRACE_GETHBPREGS and PTRACE_SETHBPREGS transfer three 32-bit words:
>> >+ * address (0), length (1), type (2).
>> >+ * Instruction breakpoint length is one of HW_BREAKPOINT_LEN_X or 0. 0 will
>> >+ * disable the breakpoint.
>> >+ * Instruction breakpoint type is one of HW_BREAKPOINT_X.
>> >+ */
>> >+
>> >+static long ptrace_gethbpregs(struct task_struct *child, unsigned long idx,
>> >+                            unsigned long __user *datap)
>> >+{
>> >+      struct perf_event *bp;
>> >+      unsigned long user_data[3] = {0};
>> >+
>> >+      if (idx >= RV_MAX_TRIGGERS)
>> >+              return -EINVAL;
>> >+
>> >+      bp = child->thread.ptrace_bps[idx];
>> >+
>> >+      if (!IS_ERR_OR_NULL(bp)) {
>> >+              user_data[0] = bp->attr.bp_addr;
>> >+              user_data[1] = bp->attr.disabled ? 0 : bp->attr.bp_len;
>> >+              user_data[2] = bp->attr.bp_type;
>> >+      }
>> >+
>> >+      if (copy_to_user(datap, user_data, sizeof(user_data)))
>> >+              return -EFAULT;
>> >+
>> >+      return 0;
>> >+}
>> >+
>> >+static long ptrace_sethbpregs(struct task_struct *child, unsigned long idx,
>> >+                            unsigned long __user *datap)
>> >+{
>> >+      struct perf_event *bp;
>> >+      struct perf_event_attr attr;
>> >+      unsigned long user_data[3];
>> >+
>> >+      if (idx >= RV_MAX_TRIGGERS)
>> >+              return -EINVAL;
>> >+
>> >+      if (copy_from_user(user_data, datap, sizeof(user_data)))
>> >+              return -EFAULT;
>> >+
>> >+      bp = child->thread.ptrace_bps[idx];
>> >+      if (IS_ERR_OR_NULL(bp))
>>
>> Why not only check for NULL?
>> IS_ERR_VALUE will always expand to be true. right?
>
>Because im dumb and thought i was setting bp to an error code, but i'm not.
>Yes if (!bp) is right.
>
>>
>> >+              attr = bp->attr;
>> >+      else
>> >+              ptrace_breakpoint_init(&attr);
>> >+
>> >+      attr.bp_addr = user_data[0];
>> >+      attr.bp_len = user_data[1];
>> >+      attr.bp_type = user_data[2];
>> >+      attr.disabled = !attr.bp_len;
>>
>> Is it okay to not have any sanitization on inputs?
>>
>> Can these inputs be controlled by user to give kernel address and kernel
>> breakpoint?
>
>modify_user_hw_breakpoint calls modify_user_hw_breakpoint_check, which
>eventually checks if we have CAP_SYS_ADMIN.
>Same for register. type and len are also checked by the
>_user_hw_breakpoint_check functions and again in the riscv code.
>
>it would be nice if this could be double checked, but it does seem
>other architectures don't check addr aswell.

I see. That's interesting.

>
>Thanks,
>Jesse Taube
>
>>
>> >+
>> >+      if (IS_ERR_OR_NULL(bp)) {
>> >+              bp = register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL,
>> >+                                         child);
>> >+              if (IS_ERR(bp))
>> >+                      return PTR_ERR(bp);
>> >+
>> >+              child->thread.ptrace_bps[idx] = bp;
>> >+              return 0;
>> >+      } else {
>> >+              return modify_user_hw_breakpoint(bp, &attr);
>> >+      }
>> >+}
>> >+#endif
>> >+
>> > long arch_ptrace(struct task_struct *child, long request,
>> >                unsigned long addr, unsigned long data)
>> > {
>> >       long ret = -EIO;
>> >+      unsigned long __user *datap = (unsigned long __user *) data;
>> >
>> >       switch (request) {
>> >+#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> >+      case PTRACE_GETHBPREGS:
>> >+              ret = ptrace_gethbpregs(child, addr, datap);
>> >+              break;
>> >+
>> >+      case PTRACE_SETHBPREGS:
>> >+              ret = ptrace_sethbpregs(child, addr, datap);
>> >+              break;
>> >+#endif
>> >       default:
>> >               ret = ptrace_request(child, request, addr, data);
>> >               break;
>> >--
>> >2.43.0
>> >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv