Re: [PATCH v14 05/14] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Modifying a debug address register without first disabling the corresponding breakpoint in DR7 violates the x86 hardware architecture specification and causes unpredictable behavior.
--

commit 35b07811f48c348c13ac1d9de7fa44d25fc7f36a
Author: Jinchao Wang <wangjinchao600@gmail.com>

x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API

This commit adds an architecture backend to modify the address of an active
per-CPU watchpoint without releasing its hardware slot. It validates the new
address, finds the installed local slot, and updates the hardware debug
register and per-CPU shadow state.

Link: https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao600@gmail.com/

> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 3204e75dc4546..2227918b0163c 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c

[ ... ]

> +int arch_modify_local_hw_breakpoint_addr(struct perf_event *bp,
> +					 unsigned long addr)
> +{

[ ... ]

> +	do {
> +		seq = this_cpu_inc_return(cpu_dr7_seq);
> +		this_cpu_write(cpu_debugreg[i], addr);
> +		barrier();
> +		set_debugreg(addr, i);

[Severity: High]
Does this code risk violating the x86 hardware architecture specification?

Looking at arch_modify_local_hw_breakpoint_addr() in
arch/x86/kernel/hw_breakpoint.c, set_debugreg() directly updates the debug
address register (DR0-DR3) while its corresponding enable bit in DR7 might
still be set.

According to hardware specs, modifying a debug address register without first
disabling the breakpoint in DR7 can lead to spurious debug exceptions or
unpredictable processor behavior if an NMI occurs or if execution boundaries
trigger a match on the transient state.

Should the DR7 enable bit be cleared before updating the debug register?

> +		barrier();
> +	} while (seq != this_cpu_read(cpu_dr7_seq));
> +
> +	return 0;
> +}

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