On Mon, 07 Sep 2026 04:02:03 +0000 [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [High] Modifying a debug address register while the breakpoint is enabled > in DR7 violates the Intel SDM. This can occur due to a race condition with an > NMI because the `cpu_dr7` shadow state is not temporarily updated. > -- > > commit ca38cc4e9ed0533dfe16175c78dba60903420a8b > Author: Jinchao Wang <[email protected]> > > 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. > > Add arch_modify_local_hw_breakpoint_addr() as the architecture backend for > modify_local_hw_breakpoint_addr(). It validates the new address, finds the > installed local slot, and updates the hardware debug register and per-CPU > shadow register. > > Link: > https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao...@gmail.com/ > > > diff --git a/arch/x86/kernel/hw_breakpoint.c > > b/arch/x86/kernel/hw_breakpoint.c > > index 312550be3ad2b..99f0926298a85 100644 > > --- a/arch/x86/kernel/hw_breakpoint.c > > +++ b/arch/x86/kernel/hw_breakpoint.c > [ ... ] > > @@ -129,6 +129,62 @@ int arch_install_hw_breakpoint(struct perf_event *bp) > [ ... ] > > + do { > > + seq = this_cpu_inc_return(cpu_dr7_seq); > > + > > + /* Disable breakpoint in DR7 before updating address register */ > > + dr7 = this_cpu_read(cpu_dr7); > > + set_debugreg((dr7 & ~__encode_dr7(i, info->len, info->type)) | > > DR7_FIXED_1, 7); > > [Severity: High] > Does arch_modify_local_hw_breakpoint_addr() need to clear the breakpoint in > the shadow cpu_dr7 state as well?
Ah, OK, let's update cpu_dr7 too. > > Since the breakpoint isn't cleared in the cpu_dr7 shadow state, could an NMI > that interrupts execution here inadvertently re-enable the breakpoint > prematurely? > > If an NMI handler (such as kgdb or perf) modifies breakpoints, it increments > cpu_dr7_seq and invokes local_db_save() and local_db_restore(). When the NMI > completes, local_db_restore() detects the sequence mismatch and restores > hardware DR7 using the cpu_dr7 shadow cache. Because cpu_dr7 still has this > breakpoint enabled, the target breakpoint gets re-enabled in hardware DR7 > before the execution below resumes. > > > + barrier(); > > + > > + this_cpu_write(cpu_debugreg[i], addr); > > + barrier(); > > + set_debugreg(addr, i); > > [Severity: High] > If an NMI prematurely restored DR7 as described above, won't this modify the > debug address register while the breakpoint is enabled in DR7? This also needs to update cpu_dr7. Thank you, > > Modifying a debug address register while its corresponding DR7 enable bit is > set violates the Intel SDM and causes spurious #DB exceptions. > > While the sequence loop retry mechanism will catch the mismatch, it only does > so after this instruction has already executed, which is too late to prevent > the spurious exception. > > > + barrier(); > > + > > + /* Re-enable breakpoint in DR7 */ > > + set_debugreg(this_cpu_read(cpu_dr7) | DR7_FIXED_1, 7); > > + barrier(); > > + } while (seq != this_cpu_read(cpu_dr7_seq)); > > + > > + return 0; > > +} > > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/178875277830.93794.14247844688761142429.stgit@devnote2?part=3 -- Masami Hiramatsu (Google) <[email protected]>
