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? 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? 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
