Re: [PATCH v12 01/11] x86/hw_breakpoints: Make DR7 updates NMI safe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing compiler barrier before the sequence number check in per-CPU seqlock loops allows the compiler to hoist the sequence check, leading to lost hardware state updates during NMI interruption.
--

--- Patch [1]: [PATCH v12 01/11] x86/hw_breakpoints: Make DR7 updates NMI safe ---
commit 585f6ee61d495eb16d59ff031d378a9d8f727fd9
Author: Jinchao Wang <wangjinchao600@gmail.com>
x86/hw_breakpoints: Make DR7 updates NMI safe

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.

Link: https://lore.kernel.org/all/4ee0a2efc9e8387af83286b8495b7d490247e165.1785067572.git.wangjinchao600@gmail.com/

> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index a2c1f2d24b645..b1fe1c47978d1 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h

[ ... ]

> @@ -145,20 +148,33 @@ static __always_inline unsigned long local_db_save(void)
>  	 * be good.
>  	 */
>  	barrier();
> -
> -	return dr7;
>  }
>  
> -static __always_inline void local_db_restore(unsigned long dr7)
> +static __always_inline void local_db_restore(unsigned long dr7,
> +					     unsigned int dr7_seq)
>  {
> +	unsigned int seq;
> +
>  	/*
>  	 * Ensure the compiler doesn't raise this statement into
>  	 * the critical section; enabling breakpoints early would
>  	 * not be good.
>  	 */
>  	barrier();
> -	if (dr7)
> +
> +	do {
> +		seq = this_cpu_read(cpu_dr7_seq);
> +		if (seq == dr7_seq) {
> +			if (!dr7)
> +				return;
> +		} else {
> +			dr7 = this_cpu_read(cpu_dr7);
> +			if (!dr7)
> +				dr7 = DR7_FIXED_1;
> +		}
> +
>  		set_debugreg(dr7, 7);
> +	} while (unlikely(seq != this_cpu_read(cpu_dr7_seq)));

[Severity: High]
Does this sequence check need a barrier() before it?

Without a compiler barrier, could the compiler hoist the
this_cpu_read(cpu_dr7_seq) check above the set_debugreg() update? If an NMI
fires after the hoisted read but before the update completes, the loop might
validate the pre-NMI sequence number and terminate prematurely without retrying,
which could desynchronize the hardware DR7 state and per-CPU shadow state.

>  }
>  
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index f846c15f21ca1..9ef24b55737f8 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c

[ ... ]

> @@ -97,38 +100,30 @@ int decode_dr7(unsigned long dr7, int bpnum, unsigned *len, unsigned *type)
>  int arch_install_hw_breakpoint(struct perf_event *bp)
>  {
>  	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> -	unsigned long *dr7;
> +	unsigned int seq;
>  	int i;
>  
>  	lockdep_assert_irqs_disabled();
>  
>  	for (i = 0; i < HBP_NUM; i++) {
> -		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> -
> -		if (!*slot) {
> -			*slot = bp;
> +		if (!this_cpu_cmpxchg(bp_per_reg[i], NULL, bp))
>  			break;
> -		}
>  	}
>  
>  	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
>  		return -EBUSY;
>  
> -	set_debugreg(info->address, i);
> -	__this_cpu_write(cpu_debugreg[i], info->address);
> -
> -	dr7 = this_cpu_ptr(&cpu_dr7);
> -	*dr7 |= encode_dr7(i, info->len, info->type);
> -
> -	/*
> -	 * Ensure we first write cpu_dr7 before we set the DR7 register.
> -	 * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> -	 */
> -	barrier();
> -
> -	set_debugreg(*dr7, 7);
> -	if (info->mask)
> -		amd_set_dr_addr_mask(info->mask, i);
> +	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), 7);
> +	} while (seq != this_cpu_read(cpu_dr7_seq));

[Severity: High]
Is a barrier() also missing here before the sequence check, similar to
local_db_restore(), to prevent the compiler from reordering the sequence
check before the DR7 register update?

>  
>  	return 0;
>  }
> @@ -146,36 +141,33 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>  {
>  	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
>  	unsigned long dr7;
> +	unsigned int seq;
>  	int i;
>  
>  	lockdep_assert_irqs_disabled();
>  
>  	for (i = 0; i < HBP_NUM; i++) {
> -		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> -
> -		if (*slot == bp) {
> -			*slot = NULL;
> +		if (this_cpu_read(bp_per_reg[i]) == bp)
>  			break;
> -		}
>  	}
>  
>  	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
>  		return;
>  
> -	dr7 = this_cpu_read(cpu_dr7);
> -	dr7 &= ~__encode_dr7(i, info->len, info->type);
> -
> -	set_debugreg(dr7, 7);
> -	if (info->mask)
> -		amd_set_dr_addr_mask(0, i);
> -
> -	/*
> -	 * Ensure the write to cpu_dr7 is after we've set the DR7 register.
> -	 * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> -	 */
> -	barrier();
> -
> -	this_cpu_write(cpu_dr7, dr7);
> +	do {
> +		seq = this_cpu_inc_return(cpu_dr7_seq);
> +		dr7 = this_cpu_read(cpu_dr7);
> +		dr7 &= ~__encode_dr7(i, info->len, info->type);
> +		set_debugreg(dr7, 7);
> +		if (info->mask)
> +			amd_set_dr_addr_mask(0, i);
> +		barrier();
> +		this_cpu_and(cpu_dr7,
> +			     ~__encode_dr7(i, info->len, info->type));
> +	} while (seq != this_cpu_read(cpu_dr7_seq));

[Severity: High]
Could the lack of a barrier() here allow the compiler to hoist the sequence
check before the shadow state update (this_cpu_and)?

> +
> +	WARN_ONCE(this_cpu_cmpxchg(bp_per_reg[i], bp, NULL) != bp,
> +		  "Can't release breakpoint slot");
>  }

[ ... ]

> @@ -483,12 +476,17 @@ void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>  
>  void hw_breakpoint_restore(void)
>  {
> -	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), 7);
> +	unsigned int seq;
> +
> +	do {
> +		seq = this_cpu_inc_return(cpu_dr7_seq);
> +		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), 7);
> +	} while (seq != this_cpu_read(cpu_dr7_seq));

[Severity: High]
Does this loop also require a barrier() before the sequence check to prevent
premature termination if an NMI interrupts the register restores?

>  }
>  EXPORT_SYMBOL_FOR_KVM(hw_breakpoint_restore);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178611679753.237811.10190793347982785419.stgit@devnote2?part=1