Re: [PATCH 2/3] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arch_modify_local_hw_breakpoint_addr() fails to increment cpu_dr7_seq, causing a race condition that can desynchronize hardware debug registers from their shadow state if interrupted by an NMI.
- [High] arch_modify_local_hw_breakpoint_addr() fails to update the primary breakpoint shadow address (info->address), causing future updates to silently revert the address.
--

--- Patch [2]: [PATCH 2/3] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API ---
commit 91337a1d81005389f6b66cba9728d9f95a16cec3
Author: Jinchao Wang <wangjinchao600@gmail.com>

x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API

Wprobe needs to move an active per-CPU watchpoint without releasing and
reserving its hardware slot. This adds a new API to find the installed
local slot and update only its address shadow and hardware debug address
register.

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

[ ... ]

> @@ -128,6 +128,26 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>  
>  void arch_modify_local_hw_breakpoint_addr(struct perf_event *bp,
>  					  unsigned long addr)
>  {
>  	int i;
>  
>  	lockdep_assert_irqs_disabled();
>  
>  	for (i = 0; i < HBP_NUM; i++) {
>  		if (this_cpu_read(bp_per_reg[i]) == bp)
>  			break;
>  	}
>  
>  	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
>  		return;
>  

[Severity: High]
Does this missing cpu_dr7_seq increment leave a race condition if interrupted
by an NMI?

If hw_breakpoint_restore() is running, reads the old shadow value, and is
interrupted by an NMI that calls this function to update the address, the
lack of a sequence bump means hw_breakpoint_restore() will not detect the
interruption. It would exit its retry loop and overwrite the hardware
register with the stale value.

[Severity: High]
Could this also cause the watchpoint to silently revert to its previous
value when rescheduled?

Since the perf_event's internal info->address (retrieved via
counter_arch_bp(bp)) is not updated here, any future call to
arch_install_hw_breakpoint() during PMU multiplexing or CPU hotplug
would reinstall the breakpoint using that stale address.

>  	this_cpu_write(cpu_debugreg[i], addr);
>  	barrier();
>  	set_debugreg(addr, i);
>  }

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