Re: [PATCH v12 02/11] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 07 Aug 2026 15:50:50 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] arch_modify_local_hw_breakpoint_addr updates the hardware and shadow registers but fails to update the perf_event's canonical software state, leading to silent hardware reverts and incorrect event samples.
> --
> 
> --- Patch [2]: [PATCH v12 02/11] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API ---
> commit bbc277e839d1fd31bb4bfa9d695721e5b6899bf6
> 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.
>     
> Add arch_modify_local_hw_breakpoint_addr() to find the installed local
> slot and update only its address shadow and hardware debug address
> register. Publish the shadow first so hw_breakpoint_restore() observes
> the new address if an NMI interrupts the update.
> 
> 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 9ef24b55737f8..c89f6e71c3a91 100644
> > --- a/arch/x86/kernel/hw_breakpoint.c
> > +++ b/arch/x86/kernel/hw_breakpoint.c
> 
> [ ... ]
> 
> > @@ -128,6 +128,39 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
> >  	return 0;
> >  }
> >  
> > +int arch_modify_local_hw_breakpoint_addr(struct perf_event *bp,
> > +					  unsigned long addr)
> > +{
> > +	struct arch_hw_breakpoint hw;
> > +	struct perf_event_attr attr = bp->attr;
> > +	unsigned int seq;
> > +	int i, ret;
> > +
> > +	lockdep_assert_irqs_disabled();
> > +
> > +	attr.bp_addr = addr;
> > +	ret = hw_breakpoint_arch_parse(bp, &attr, &hw);
> > +	if (ret)
> > +		return ret;
> 
> [Severity: High]
> Does this discard the validated software state?
> 
> The new address configuration is successfully validated into the local 'hw'
> struct, but neither the architecture-specific state (counter_arch_bp(bp))
> nor the core attribute (bp->attr.bp_addr) are updated.

Yes, if it is sololy used. But in the 3rd patch, this function is only
called from modify_local_hw_breakpoint_addr(), which updates
counter_arch_bp(bp)->address. I will add this as a comment.

> 
> If the breakpoint is later re-installed (e.g., during CPU offline/online or
> PMU scheduling), arch_install_hw_breakpoint() will read the stale address:
> 
>     struct arch_hw_breakpoint *info = counter_arch_bp(bp);
>     ...
>     this_cpu_write(cpu_debugreg[i], info->address);
>     barrier();
>     set_debugreg(info->address, i);
> 
> Could this silently revert the hardware register back to the old address?
> 
> Also, if a watchpoint fires, perf_bp_event() initializes the sample record
> using the unmodified bp->attr.bp_addr. Could this result in reporting
> incorrect addresses in the trace output?

trace_wprobe ensures the hit address and modified bp->attr.bp_addr are the
same. So there should be no problem.

Thanks,

> 
> > +
> > +	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 -ENOENT;
> > +
> > +	do {
> > +		seq = this_cpu_inc_return(cpu_dr7_seq);
> > +		this_cpu_write(cpu_debugreg[i], addr);
> > +		barrier();
> > +		set_debugreg(addr, i);
> > +	} while (seq != this_cpu_read(cpu_dr7_seq));
> > +
> > +	return 0;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/178611679753.237811.10190793347982785419.stgit@devnote2?part=2


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>