Re: [PATCH v14 06/14] HWBP: Add modify_local_hw_breakpoint_addr() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Lockless updates to `bp->attr.bp_addr` and `counter_arch_bp(bp)->address` lack `WRITE_ONCE()`, constituting a formal data race under the Linux Kernel Memory Model.
--

commit 0c79300d09e43eac52a027f6cb480808e7f3902f
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

HWBP: Add modify_local_hw_breakpoint_addr() API

This commit adds the modify_local_hw_breakpoint_addr() API to update the
watched address of an installed hardware breakpoint on the local CPU without
releasing and reserving its hardware slot.

Link: https://lore.kernel.org/all/f9c49dfa49bdc57ba8c0574bc9981c1e581acf92.1785067572.git.wangjinchao600@gmail.com/

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 789add0c185a0..124074c3d3bc2 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
[ ... ]
> +int modify_local_hw_breakpoint_addr(struct perf_event *bp,
> +				    unsigned long addr)
> +{
> +	unsigned long old_addr;
> +	int ret;
> +
> +	lockdep_assert_irqs_disabled();
> +
> +	old_addr = bp->attr.bp_addr;
> +	counter_arch_bp(bp)->address = addr;
> +	bp->attr.bp_addr = addr;

[Severity: Low]
The function intentionally updates bp->attr.bp_addr locklessly so NMI handlers
can observe the new address during the transition. However, without WRITE_ONCE()
annotations, does this constitute a formal data race under the Linux Kernel
Memory Model? 

While a torn store on 32-bit architectures might only overwrite the upper 32
bits with zero (leaving the address functionally intact), could this still
trigger KCSAN warnings when an NMI handler concurrently reads bp->attr.bp_addr
via perf_bp_event()?

> +
> +	ret = arch_modify_local_hw_breakpoint_addr(bp, addr);
> +	if (ret) {
> +		counter_arch_bp(bp)->address = old_addr;
> +		bp->attr.bp_addr = old_addr;

[Severity: Low]
Similarly, should the rollback path also use WRITE_ONCE() to ensure concurrent
accesses by NMI handlers avoid data race warnings from KCSAN?

> +		return ret;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178810001186.64882.2161016469449127450.stgit@devnote2?part=6