Re: [RFC PATCH 04/13] HWBP: Add modify_wide_hw_breakpoint_local() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] In-place mutation of active hardware breakpoint state (`bp->hw.info`) without disabling the event causes NMI data races and error-path corruption.
- [High] The DRn debug address register is updated while the hardware breakpoint is still enabled in DR7, risking spurious debug exceptions.
- [Medium] `modify_wide_hw_breakpoint_local` fails to update `bp->attr` on successful modification, causing state desynchronization.
--

--- Patch [4]: [RFC PATCH 04/13] HWBP: Add modify_wide_hw_breakpoint_local() API ---
commit 861178eefb9a8774dffa3900f118da48cde078a9
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

    HWBP: Add modify_wide_hw_breakpoint_local() API
    
    Add modify_wide_hw_breakpoint_local() arch-wide interface which allows
    hwbp users to update watch address on-line. This is available if the
    arch supports CONFIG_HAVE_REINSTALL_HW_BREAKPOINT.

[ ... ]

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 789add0c185a0..20ca64f305083 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -888,6 +888,43 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)

[ ... ]

> +int modify_wide_hw_breakpoint_local(struct perf_event *bp,
> +				    struct perf_event_attr *attr)
> +{
> +	int ret;
> +
> +	if (find_slot_idx(bp->attr.bp_type) != find_slot_idx(attr->bp_type))
> +		return -EINVAL;
> +
> +	ret = hw_breakpoint_arch_parse(bp, attr, counter_arch_bp(bp));

[Severity: High]
Does this code call hw_breakpoint_arch_parse() on the active event's hardware
info without temporarily disabling the event?

Architecture implementations like x86's arch_build_bp_info() mutate the struct
incrementally:

arch/x86/kernel/hw_breakpoint.c:arch_build_bp_info() {
	hw->address = attr->bp_addr;
	hw->mask = 0;

	/* Type */
	switch (attr->bp_type) {
...
}

If validation fails midway, does bp->hw.info remain partially overwritten and
corrupted?

Additionally, could an NMI hitting the active breakpoint during this window
read torn state?

> +	if (ret)
> +		return ret;
> +
> +	return arch_reinstall_hw_breakpoint(bp);

[Severity: High]
Does arch_reinstall_hw_breakpoint() risk spurious debug exceptions here?

Because modify_wide_hw_breakpoint_local() does not temporarily disable the
breakpoint first, the slot is already enabled in the physical DR7.

Modifying DRn before the control register DR7 is cleared or updated can
violate architectural rules:

arch/x86/kernel/hw_breakpoint.c:setup_hwbp() {
	set_debugreg(info->address, slot);
	__this_cpu_write(cpu_debugreg[slot], info->address);

	dr7 = this_cpu_read(cpu_dr7);
	/*
	 * Clear the slot's stale len/type and enable bits first
...
}

Could this trigger spurious #DB debug exceptions if the CPU accesses the new
address during the window before DR7 is updated?

[Severity: Medium]
When this modification is successful, should bp->attr be updated with the new
user-provided attributes?

It looks like the newly parsed attr is never copied back to bp->attr.

Any subsequent read of the breakpoint properties or perf sample
initializations might silently use the old, stale address and length:

kernel/events/core.c:perf_bp_event() {
	perf_sample_data_init(&sample, bp->attr.bp_addr, 0);
...
}

Will this cause state desynchronization and silently corrupt trace data?

> +}
> +#else
> +int modify_wide_hw_breakpoint_local(struct perf_event *bp,
> +				    struct perf_event_attr *attr)
> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714182243.10687-1-wangjinchao600@gmail.com?part=4