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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Non-atomic updates to active hardware breakpoint state can cause torn reads in NMI/#DB handlers.
- [High] The function alters the event's internal software state before verifying if the hardware update succeeds, leaving the state permanently corrupted if the hardware update fails.
--

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

HWBP: Add modify_wide_hw_breakpoint_local() API

This commit adds modify_wide_hw_breakpoint_local(), an arch-wide interface
allowing hardware breakpoint users to update watch addresses on-line for the
local CPU. It updates the configuration by reusing the current hardware slot
without releasing and reserving it.

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 789add0c185a0..f4709c892d674 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c

[ ... ]

> +int modify_wide_hw_breakpoint_local(struct perf_event *bp,
> +				    struct perf_event_attr *attr)
> +{
> +	struct arch_hw_breakpoint info;
> +	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, &info);
> +	if (ret)
> +		return ret;
> +
> +	*counter_arch_bp(bp) = info;
> +	bp->attr.bp_addr = attr->bp_addr;
> +	bp->attr.bp_type = attr->bp_type;
> +	bp->attr.bp_len = attr->bp_len;

[Severity: High]
Can this cause a torn read if an NMI/#DB exception occurs while we are
non-atomically updating the breakpoint state?

If an NMI triggers a #DB exception before arch_reinstall_hw_breakpoint()
completes, the hw_breakpoint_handler might read the partially updated
bp->hw.info and bp->attr while the hardware DR7 register is still armed with
the old configuration.

On architectures like x86, reading a torn bp->hw.info.type might cause the
handler to misidentify the breakpoint type and fail to set the RF flag,
leading to an infinite #DB loop.

> +
> +	return arch_reinstall_hw_breakpoint(bp);

[Severity: High]
Does this leave the software state permanently corrupted if the hardware
update fails?

If the PMU rotates the event out just before this function is called,
arch_reinstall_hw_breakpoint() might fail and return an error like -EINVAL.
This error is propagated to the caller, so the caller assumes the
modification failed.

However, the software state (bp->attr and bp->hw.info) was already
overwritten above and is not rolled back. When the PMU later rotates the
breakpoint back in, will it silently use this uncommitted new configuration?

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

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