Re: [PATCH v7 08/10] HWBP: Add modify_wide_hw_breakpoint_local() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] State Corruption on Error Path during breakpoint modification
- [High] Hardware Register Corruption (DR7 missing bit clear) on Reinstall
- [Medium] Inconsistent Software State after breakpoint modification
- [Medium] Race Condition with Debug Exception Handlers during in-place modification
--

--- Patch [8]: [PATCH v7 08/10] HWBP: Add modify_wide_hw_breakpoint_local() API ---
commit e82221932a08cae24063fb7cb3fe49f250391aa5
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

HWBP: Add modify_wide_hw_breakpoint_local() API

This commit adds the modify_wide_hw_breakpoint_local() arch-wide interface
which allows hwbp users to update watch addresses on-line. It reuses the
current hardware breakpoint slot without releasing and reserving it, meaning
type changes are only allowed between compatible types.

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

[ ... ]

> @@ -888,6 +888,45 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)
>  }
>  EXPORT_SYMBOL_GPL(unregister_wide_hw_breakpoint);
>  
> +/**
> + * modify_wide_hw_breakpoint_local - update breakpoint config for local CPU
> + * @bp: the hwbp perf event for this CPU
> + * @attr: the new attribute for @bp
> + *
> + * This does not release and reserve the slot of a HWBP; it just reuses the
> + * current slot on local CPU. So the users must update the other CPUs by
> + * themselves.
> + * Also, since this does not release/reserve the slot, this can not change the
> + * type to incompatible type of the HWBP.
> + * Return err if attr is invalid or the CPU fails to update debug register
> + * for new @attr.
> + */
> +#ifdef CONFIG_HAVE_REINSTALL_HW_BREAKPOINT
> +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: Medium]
Is it safe to mutate the active hardware state structure in-place while the
hardware breakpoint is still active? If a debug exception (#DB) triggers on
the same CPU during this non-atomic update, could hw_breakpoint_handler()
read a partially updated bp->hw.info and generate flawed samples?

[Severity: High]
Could this corrupt the active state on error paths? By passing the active
hardware configuration pointer via counter_arch_bp(bp) directly, the
architecture parser mutates it in place.

For example, in arch_build_bp_info() on x86, if length or type is updated
but a subsequent alignment check fails, we return an error but leave the
active struct partially modified. When the breakpoint is later uninstalled,
could this use the corrupted length/type to mask out DR7, leaving residual
bits that permanently corrupt the slot?

> +	if (ret)
> +		return ret;
> +
> +	bp->attr.bp_addr = attr->bp_addr;

[Severity: Medium]
Does this leave the logical perf_event attributes disconnected from the
hardware configuration? The parser updates the hardware state to reflect the
new type and length, but we only copy attr->bp_addr here. Should this also
update bp->attr.bp_type and bp->attr.bp_len?

> +
> +	return arch_reinstall_hw_breakpoint(bp);

[Severity: High]
Can reinstalling the breakpoint without clearing the old hardware state
corrupt the debug registers? When arch_reinstall_hw_breakpoint() invokes
setup_hwbp() on x86, it ORs the new length and type bits into DR7:

arch/x86/kernel/hw_breakpoint.c:setup_hwbp()
    if (enable)
        dr7 |= encode_dr7(slot, info->len, info->type);

Since we did not disable the breakpoint first, won't a bitwise OR of the
old and new encoded values produce an invalid configuration?

Additionally, if the new configuration sets info->mask to 0, setup_hwbp()
skips resetting the hardware mask MSR because it evaluates if (info->mask).
Could this leave the old mask permanently active?

> +}
> +#else

[ ... ]

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