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

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 <[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() 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.

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?

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