Re: [PATCH v17 03/13] x86/hw_breakpoints: Make DR7 updates NMI safe
flat view
From: Peter Zijlstra <peterz@infradead.org>
Date: 2026-09-23 09:13:31
Also in:
kvm, linux-doc, linux-perf-users, lkml
On Tue, Sep 22, 2026 at 01:25:19PM +0900, Masami Hiramatsu (Google) wrote:
From: Jinchao Wang <redacted> Hardware breakpoint installation and removal run with IRQs disabled, but an NMI can still enter the same code through KGDB. The interrupted operation and the NMI can consequently claim the same slot or overwrite each other's DR7 state. Claim and release per-CPU slots with cmpxchg. Update cpu_dr7 with single-instruction per-CPU operations, and preserve hardware-first disable and hardware-last enable ordering. Add a per-CPU sequence number so interrupted DR7 writers and restore paths detect an NMI update and retry from the latest shadow state.
Bah, KGDB.. Aren't there far more problems with that thing?
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h index 854d82b88ff4..515d2d313d0c 100644 --- a/arch/x86/include/asm/debugreg.h +++ b/arch/x86/include/asm/debugreg.h@@ -18,6 +18,7 @@ #define DR7_FIXED_1 0x00000400 DECLARE_PER_CPU(unsigned long, cpu_dr7); +DECLARE_PER_CPU(unsigned int, cpu_dr7_seq);
Would it make sense to:
typedef struct {
unsigned long dr7;
unsigned int seq;
} dr7_save_t;
quoted hunk ↗ jump to hunk
#ifndef CONFIG_PARAVIRT_XXL /*@@ -125,40 +126,69 @@ static __always_inline bool hw_breakpoint_active(void) extern void hw_breakpoint_restore(void); -static __always_inline unsigned long local_db_save(void) +static __always_inline void local_db_save(unsigned long *dr7, + unsigned int *dr7_seq)
static __always_inline dr7_save_t local_db_save(void)
{quoted hunk ↗ jump to hunk
} -static __always_inline void local_db_restore(unsigned long dr7) +static __always_inline void local_db_restore(unsigned long dr7, + unsigned int dr7_seq)
static __always_inline void local_db_restore(dr7_save_t dr7)
{} #ifdef CONFIG_CPU_SUP_AMD
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c index 0473a5c95856..901323ae7d6a 100644 --- a/arch/x86/kernel/hw_breakpoint.c +++ b/arch/x86/kernel/hw_breakpoint.c
quoted hunk ↗ jump to hunk
@@ -106,32 +108,25 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
quoted hunk ↗ jump to hunk
+ do { + seq = this_cpu_inc_return(cpu_dr7_seq); + this_cpu_write(cpu_debugreg[i], info->address); + barrier(); + set_debugreg(info->address, i); + if (info->mask) + amd_set_dr_addr_mask(info->mask, i); + this_cpu_or(cpu_dr7, encode_dr7(i, info->len, info->type)); + barrier(); + set_debugreg(this_cpu_read(cpu_dr7) | DR7_FIXED_1, 7); + barrier(); + } while (seq != this_cpu_read(cpu_dr7_seq)); return 0; }@@ -149,36 +144,34 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
quoted hunk ↗ jump to hunk
+ do { + seq = this_cpu_inc_return(cpu_dr7_seq); + dr7 = this_cpu_read(cpu_dr7);
You're inconsistent with the leading barrier().
quoted hunk ↗ jump to hunk
+ dr7 &= ~__encode_dr7(i, info->len, info->type); + set_debugreg(dr7 | DR7_FIXED_1, 7); + if (info->mask) + amd_set_dr_addr_mask(0, i); + barrier(); + this_cpu_and(cpu_dr7, + ~__encode_dr7(i, info->len, info->type)); + barrier(); + } while (seq != this_cpu_read(cpu_dr7_seq)); + + WARN_ONCE(this_cpu_cmpxchg(bp_per_reg[i], bp, NULL) != bp, + "Can't release breakpoint slot"); }
These loops should be far more similar. Note how the top one does: this_cpu_or(cpu_dr7, encode_dr7(...)); set_debugreg(this_cpu_read(cpu_dr7) | ..., 7); while the bottom one does: dr7 &= ~encode_dr7(...) set_debugreg(dr7 | ...); this_cpu_and(cpu_dr7, ~encode_dr7(...)); Why can't they both have the same shape and only one encode_dr7() instance?
quoted hunk ↗ jump to hunk
@@ -486,12 +480,18 @@ void flush_ptrace_hw_breakpoint(struct task_struct *tsk) void hw_breakpoint_restore(void) { + unsigned int seq; + + do { + seq = this_cpu_inc_return(cpu_dr7_seq);
no barrier().
quoted hunk ↗ jump to hunk
+ set_debugreg(this_cpu_read(cpu_debugreg[0]), 0); + set_debugreg(this_cpu_read(cpu_debugreg[1]), 1); + set_debugreg(this_cpu_read(cpu_debugreg[2]), 2); + set_debugreg(this_cpu_read(cpu_debugreg[3]), 3); + set_debugreg(DR6_RESERVED, 6); + set_debugreg(this_cpu_read(cpu_dr7) | DR7_FIXED_1, 7); + barrier(); + } while (seq != this_cpu_read(cpu_dr7_seq)); } EXPORT_SYMBOL_FOR_KVM(hw_breakpoint_restore);
I really can't say I'm a fan of this. Is KGDB really a thing?