Re: [RFC PATCH 02/13] x86/hw_breakpoint: Unify breakpoint install/uninstall

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing compiler barrier during hardware breakpoint uninstall reintroduces an NMI race condition.
- [Medium] Redundant hardware debug address register write during breakpoint uninstall degrades performance.
--

--- Patch [2]: [RFC PATCH 02/13] x86/hw_breakpoint: Unify breakpoint install/uninstall ---
commit 856f1186c52b7f5043f0e5e6063055a66d542dc1
Author: Jinchao Wang <wangjinchao600@gmail.com>

x86/hw_breakpoint: Unify breakpoint install/uninstall

Consolidate breakpoint management to reduce code duplication and shrink
stripped code size. This refactoring paves the way for reinstalling
breakpoints and adds a missing types dependency.

> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c

[ ... ]

> +static void setup_hwbp(struct arch_hw_breakpoint *info, int slot, bool enable)
> +{
> +	unsigned long dr7;
> +
> +	set_debugreg(info->address, slot);
> +	__this_cpu_write(cpu_debugreg[slot], info->address);

[Severity: Medium]
Does this code unconditionally write the hardware debug address register
even on the uninstall path?

Writing to DR0-DR3 is typically a slow serializing operation, which can
also trigger expensive VMExits in virtualized environments. Since the
breakpoint is being disabled via DR7 anyway when enable is false, is
this address register write necessary here? Could this introduce
unnecessary overhead on the hot path during perf event scheduling?

> +
> +	dr7 = this_cpu_read(cpu_dr7);
> +	if (enable)
> +		dr7 |= encode_dr7(slot, info->len, info->type);
> +	else
> +		dr7 &= ~__encode_dr7(slot, info->len, info->type);
> +
> +	/*
> +	 * Enabling:
> +	 *   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.
> +	 */
> +	if (enable)
> +		this_cpu_write(cpu_dr7, dr7);
> +
> +	barrier();
> +
> +	set_debugreg(dr7, 7);
> +
> +	if (info->mask)
> +		amd_set_dr_addr_mask(enable ? info->mask : 0, slot);
> +
> +	/*
> +	 * Disabling:
> +	 *   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.
> +	 */
> +	if (!enable)
> +		this_cpu_write(cpu_dr7, dr7);

[Severity: Medium]
Could the compiler reorder this write to cpu_dr7 before the DR7 register
is updated on the disable path?

The original uninstall code included an explicit compiler barrier before
updating cpu_dr7, but it appears to be missing here. If the compiler
reorders this memory access, could an NMI hit in the middle, observe a 
cleared cpu_dr7, and incorrectly assume no breakpoints are active? 

If the NMI handler then touches watched memory, could this lead to a fatal
nested hardware breakpoint exception?

> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714182243.10687-1-wangjinchao600@gmail.com?part=2