Re: [RFC PATCH 02/13] x86/HWBP: Add arch_reinstall_hw_breakpoint() for atomic updates

[Jinchao Wang <wangjinchao600@gmail.com>]

On 9/2/25 22:11, Masami Hiramatsu (Google) wrote:
> On Mon, 1 Sep 2025 18:23:44 +0800
> Jinchao Wang <wangjinchao600@gmail.com> wrote:
> 
>> On 9/1/25 15:06, Masami Hiramatsu (Google) wrote:
>>> Hi Jinchao,
>>>
>> Hi Masami,
>>
>>> On Mon, 18 Aug 2025 20:26:07 +0800
>>> Jinchao Wang <wangjinchao600@gmail.com> wrote:
>>>
>>>> Add arch_reinstall_hw_breakpoint() to enable atomic context modification
>>>> of hardware breakpoint parameters without deallocating and reallocating
>>>> the breakpoint slot.
>>>>
>>>> The existing arch_install_hw_breakpoint() allocates a new debug register
>>>> slot, while arch_uninstall_hw_breakpoint() deallocates it. However, some
>>>> use cases require modifying breakpoint parameters (address, length, type)
>>>> atomically without losing the allocated slot, particularly when operating
>>>> in atomic contexts where allocation might fail or be unavailable.
>>>>
>>>> This is particularly useful for debugging tools like kstackwatch that
>>>> need to dynamically update breakpoint targets in atomic contexts while
>>>> maintaining consistent hardware state.
>>>>
>>>
>>> I'm also trying to find this interface for my wprobe. So the idea is good.
>>> But this looks hacky and only for x86. I think the interface should be
>>> more generic and do not use this arch internal function directly.
>>>
>>
>> I agree with your point about the architectural dependency. I have been
>> considering this problem not only for the hardware breakpoint
>> reinstallation,
>> but also for other related parts of the series, such as canary finding and
>> stack address resolving. These parts also rely on arch-specific code.
> 
> Yes, even though, the hw-breakpoint is an independent feature.
> Directly using arch_*() functions (which are expected to be used
> internally) introduces a hidden dependency between these two
> components and looses maintainability.

Yes, I am trying to improve this in the v3 series.

> 
>>> It seems that the slot is allocated by "type", thus, if this reinstall
>>> hwbp without deallocate/allocate slot, it must NOT change the type.
>>> See __modify_bp_slot. Also, provide CONFIG_HAVE_... option for checking
>>> whether the architecture support that interface.
>>>
>> Regarding the slot allocation, I would like to clarify my point. I
>> believe the
>> event->attr.type should not be changed when reinstalling a hardware
>> breakpoint, as this defines the fundamental nature of the event. The type
>> must always be PERF_TYPE_BREAKPOINT.
>>
>> The event->attr.bp_type, however, can be changed. For example, from a
>> HW_BREAKPOINT_W to a HW_BREAKPOINT_RW without needing to deallocate and
>> reallocate the slot. This is useful for future applications, even though the
>> current use case for KStackWatch only requires HW_BREAKPOINT_W.
> 
> I understand your point, so it also needs another wrapper which checks
> the type is compatible on the architecture.
> 

I think the wrapper should handle the type by type_slot, something like[1]:

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 1db2c5e24d0e..6fed9521baf2 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -752,6 +752,7 @@ modify_user_hw_breakpoint_check(struct perf_event 
*bp, struct perf_event_attr *a
  {
  	struct arch_hw_breakpoint hw = { };
  	int err;
+	enum bp_type_idx old_type_idx, new_type_idx;

  	err = hw_breakpoint_parse(bp, attr, &hw);
  	if (err)
@@ -766,7 +767,9 @@ modify_user_hw_breakpoint_check(struct perf_event 
*bp, struct perf_event_attr *a
  			return -EINVAL;
  	}

-	if (bp->attr.bp_type != attr->bp_type) {
+	old_type_idx = find_slot_idx(bp->attr.bp_type);
+	new_type_idx = find_slot_idx(attr->bp_type);
+	if (old_type_idx != new_type_idx) {
  		err = modify_bp_slot(bp, bp->attr.bp_type, attr->bp_type);
  		if (err)
  			return err;

For kernel breakpoints, we might also consider introducing a
modify_kernel_hw_breakpoint() helper, similar to
modify_user_hw_breakpoint(), to encapsulate the kernel-specific case.

[1]https://lore.kernel.org/all/20250903075144.3722848-3-wangjinchao600@gmail.com/

>>
>> By the way, I have sent an updated series.
>> https://lore.kernel.org/all/20250828073311.1116593-1-wangjinchao600@gmail.com/
> 
> Yeah, OK, let me review the series. Thanks for update!
> 
>>
>> Thank you again for your valuable review.
>> -- 
>> Best regards,
>> Jinchao
> 
> 


-- 
Best regards,
Jinchao