Re: [RFC PATCH 02/13] x86/HWBP: Add arch_reinstall_hw_breakpoint() for atomic updates
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Date: 2025-09-04 01:02:35
Also in:
linux-mm, lkml
On Wed, 3 Sep 2025 15:58:44 +0800 Jinchao Wang [off-list ref] wrote:
On 9/2/25 22:11, Masami Hiramatsu (Google) wrote:quoted
On Mon, 1 Sep 2025 18:23:44 +0800 Jinchao Wang [off-list ref] wrote:quoted
On 9/1/25 15:06, Masami Hiramatsu (Google) wrote:quoted
Hi Jinchao,Hi Masami,quoted
On Mon, 18 Aug 2025 20:26:07 +0800 Jinchao Wang [off-list ref] wrote:quoted
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.quoted
quoted
quoted
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]:
Ah, that's a good idea!
quoted hunk ↗ jump to hunk
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/ (local)
Hmm, it seems that there is *user_hw_breakpoint() and *wide_hw_breakpoint() (maybe it means system-wide) so it is better to call modify_wide_hw_breakpoint(). (anyway it is only for kernel address space...) Thank you!
quoted
quoted
By the way, I have sent an updated series. https://lore.kernel.org/all/20250828073311.1116593-1-wangjinchao600@gmail.com/ (local)Yeah, OK, let me review the series. Thanks for update!quoted
Thank you again for your valuable review. -- Best regards, Jinchao-- Best regards, Jinchao
-- Masami Hiramatsu (Google) [off-list ref]