Re: [PATCH v8 7/9] 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] State Corruption on Error Path
- [High] Race Condition / Unprotected State Modification with NMI
- [Medium] Locking Context / Lockdep Splat Due to Missing IRQ Disabling
--

--- Patch [7]: [PATCH v8 7/9] HWBP: Add modify_wide_hw_breakpoint_local() API ---
commit 12f28341e1b86eda783641bab63a14b074ec1ca3
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Subject: HWBP: Add modify_wide_hw_breakpoint_local() API

This patch adds an architecture-wide interface to allow hardware breakpoint
users to update watch addresses online. It supports modifying compatible
breakpoint types without releasing and reserving the hardware slot.

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
[ ... ]
> @@ -888,6 +888,49 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)
[ ... ]
> +#ifdef CONFIG_HAVE_REINSTALL_HW_BREAKPOINT
> +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]
Is it safe to modify these software attributes non-atomically while the
hardware breakpoint is still fully enabled?

If a concurrent hardware debug exception fires and acts as a non-maskable
interrupt during this modification, could the exception handler read partially
updated or torn attributes and report incorrect trace data?

> +
> +	return arch_reinstall_hw_breakpoint(bp);

[Severity: High]
If arch_reinstall_hw_breakpoint() fails, this returns the error code directly,
but the internal state of the event has already been permanently modified.

Could this leave the breakpoint object in an inconsistent state where the
caller receives an error but the object was mutated?

Should we restore the original state if the architecture update fails?

[Severity: Medium]
Do callers of modify_wide_hw_breakpoint_local() need to have interrupts
disabled?

Calling arch_reinstall_hw_breakpoint() on x86 eventually reaches
arch_manage_bp(), which contains an unconditional
lockdep_assert_irqs_disabled():

modify_wide_hw_breakpoint_local()
  arch_reinstall_hw_breakpoint()
    arch_manage_bp()
      lockdep_assert_irqs_disabled()

Since this exported function doesn't disable interrupts itself or document
the requirement, will callers from standard contexts trigger a lockdep splat?

> +}
> +#else
[ ... ]

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