Re: [PATCH v17 03/13] x86/hw_breakpoints: Make DR7 updates NMI safe

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Sep 22, 2026 at 01:25:19PM +0900, Masami Hiramatsu (Google) wrote:
> From: Jinchao Wang <wangjinchao600@gmail.com>
> 
> 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? 


> 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;


>  #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)

>  {

>  }
>  
> -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

> 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

> @@ -106,32 +108,25 @@ int arch_install_hw_breakpoint(struct perf_event *bp)

> +	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)

> +	do {
> +		seq = this_cpu_inc_return(cpu_dr7_seq);
> +		dr7 = this_cpu_read(cpu_dr7);

You're inconsistent with the leading barrier(). 

> +		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?


> @@ -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().

> +		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?