tracing: wprobe: x86: Add wprobe for watchpoint
Subsystems (7)
Masami Hiramatsu (Google)
maintainerv10 cover tracing: wprobe: x86: Add wprobe for watchpoint
Hi,
Here is the 10th version of the series for adding new wprobe (watch probe) which provides memory access tracing event. Moreover, this can be used via event trigger. Thus it can trace memory access on a dynamically allocated objects too.
The previous version is here:
https://lore.kernel.org/all/178429796992.157981.3393977217853767915.stgit@devnote2/
This version add BTF support in wprobe trigger command[11/11]. With this feature, user do not need to decode the offset of structure fields. (Since the previous version lacked base-commit tag, Sashiko failed to
apply it for review. Thus this is just add a BTF support.)
To support kprobe events, we need to check the previous NMI context or need to use an atomic refcount etc. But that should be another patch for review.
To support arm64, we need to avoid major pagefault on single-stepping issue. I think we can solve it with checking whether the address is the kernel (which must not cause a major page fault), or not.
Public branch
-------------
I will push this branch as topic/wprobe-v2 to my tree so that it can be easily tested.
https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/log/?h=topic/wprobe-v2
This is based on linux-trace tree's probes/for-next.
Usage
-----
The basic usage of this wprobe is similar to other probes;
w:[GRP/][EVENT] [r|w|rw]@<ADDRESS|SYMBOL[+OFFS]> [FETCHARGS]
This defines a new wprobe event. For example, to trace jiffies update, you can do;
echo 'w:my_jiffies w@jiffies:8 value=+0($addr)' >> dynamic_events echo 1 > events/wprobes/my_jiffies/enable
Moreover, this can be combined with event trigger to trace the memory accecss on slab objects. The trigger syntax is;
set_wprobe:WPROBE_EVENT:FIELD[+OFFSET] [if FILTER] clear_wprobe:WPROBE_EVENT[:FIELD[+OFFSET]] [if FILTER]
set_wprobe sets WPROBE_EVENT's watch address on FIELD[+OFFSET]. clear_wprobe clears WPROBE_EVENT's watch address if it is set to FIELD[+OFFSET]. If FIELD is omitted, forcibly clear the watch address when trigger event is hit.
For example, trace the first 8 byte of the dentry data structure passed to do_truncate() until it is deleted by dentry_kill().
(Note: all tracefs setup uses '>>' so that it does not kick do_truncate())
# echo 'w:watch rw@0:8 address=$addr value=+0($addr)' > dynamic_events
# echo 'f:truncate do_truncate dentry=$arg2' >> dynamic_events # echo 'set_wprobe:watch:dentry' >> events/fprobes/truncate/trigger
# echo 'f:dentry_kill dentry_kill dentry=$arg1' >> dynamic_events # echo 'clear_wprobe:watch:dentry' >> events/fprobes/dentry_kill/trigger
# echo 1 >> events/fprobes/truncate/enable # echo 1 >> events/fprobes/dentry_kill/enable
# echo aaa > /tmp/hoge # echo bbb > /tmp/hoge # echo ccc > /tmp/hoge # rm /tmp/hoge
Then, the trace data will show;
# tracer: nop
#
# entries-in-buffer/entries-written: 32/32 #P:8
#
# _-----=> irqs-off/BH-disabled
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / _-=> migrate-disable
# |||| / delay
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
sh-107 [004] ...1. 9.990418: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004ad6618
sh-107 [004] ...1. 9.990914: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b3de78
sh-107 [004] ...1. 9.993175: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049ddd40
sh-107 [004] ..... 9.995198: truncate: (do_truncate+0x4/0x120) dentry=0xffff8880048083a8
sh-107 [004] ...1. 9.995389: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db998
sh-107 [004] ..Zff 9.997503: watch: (lookup_fast+0xaa/0x150) address=0xffff8880048083a8 value=0x8200080
sh-107 [004] ..Zff 9.997509: watch: (path_openat+0x211/0xda0) address=0xffff8880048083a8 value=0x8200080
sh-107 [004] ..Zff 9.997514: watch: (path_openat+0xa56/0xda0) address=0xffff8880048083a8 value=0x8200080
sh-107 [004] ..Zff 9.997518: watch: (path_openat+0xae2/0xda0) address=0xffff8880048083a8 value=0x8200080
sh-107 [004] ..... 9.997521: truncate: (do_truncate+0x4/0x120) dentry=0xffff8880048083a8
sh-107 [004] ...1. 9.997582: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004808270
sh-107 [004] ...1. 9.999365: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db728
sh-107 [004] ...1. 9.999388: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b1c000
rm-113 [005] ..Zff 10.000965: watch: (lookup_fast+0xaa/0x150) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.000971: watch: (path_lookupat+0x97/0x1e0) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.000984: watch: (lookup_fast+0xaa/0x150) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.000988: watch: (path_lookupat+0x97/0x1e0) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.001010: watch: (lookup_one_qstr_excl+0x28/0x140) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.001014: watch: (lookup_one_qstr_excl+0xd1/0x140) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.001018: watch: (may_delete_dentry+0x1c/0x200) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.001021: watch: (may_delete_dentry+0x195/0x200) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] ..Zff 10.001031: watch: (vfs_unlink+0x5e/0x260) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] d.Z.. 10.001067: watch: (d_make_discardable+0x1b/0x40) address=0xffff8880048083a8 value=0x8200080
rm-113 [005] d.Z.. 10.001071: watch: (d_make_discardable+0x29/0x40) address=0xffff8880048083a8 value=0x200080
rm-113 [005] ...1. 10.001072: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880048083a8
rm-113 [005] ...1. 10.001218: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880048083a8
sh-107 [004] ...1. 10.001416: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db110
sh-107 [004] ...1. 10.001444: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db248
sh-107 [004] ...1. 10.001500: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004ad6618
sh-107 [004] ...1. 10.002067: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b41e78
sh-107 [004] ...1. 10.904920: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b41e78
sh-107 [004] ...1. 10.905129: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004ad6618Thank you,
---
base-commit: 1a416ae446afa42d2d8500ce25bd61c564508721
Jinchao Wang (2):
x86/hw_breakpoint: Unify breakpoint install/uninstall x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint
Masami Hiramatsu (Google) (9):
tracing: wprobe: Add watchpoint probe event based on hardware breakpoint
x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires
selftests: tracing: Add a basic testcase for wprobe
selftests: tracing: Add syntax testcase for wprobe
HWBP: Add modify_wide_hw_breakpoint_local() API
tracing: wprobe: Add wprobe event trigger
selftests: ftrace: Add wprobe trigger testcase tracing/wprobe: Support BTF typecast in fetchargs tracing/wprobe: Support BTF typecast in wprobe trigger command
Documentation/trace/index.rst | 1 Documentation/trace/wprobetrace.rst | 163 +++ arch/Kconfig | 20 arch/x86/Kconfig | 2 arch/x86/include/asm/hw_breakpoint.h | 8 arch/x86/kernel/hw_breakpoint.c | 164 ++- include/linux/hw_breakpoint.h | 6 include/linux/trace_events.h | 3 kernel/events/hw_breakpoint.c | 61 + kernel/trace/Kconfig | 24 kernel/trace/Makefile | 1 kernel/trace/trace.c | 9 kernel/trace/trace.h | 7 kernel/trace/trace_events_trigger.c | 12 kernel/trace/trace_probe.c | 24 kernel/trace/trace_probe.h | 14 kernel/trace/trace_wprobe.c | 1300 ++++++++++++++++++++ tools/testing/selftests/ftrace/config | 2 .../ftrace/test.d/dynevent/add_remove_wprobe.tc | 63 + .../test.d/dynevent/wprobes_syntax_errors.tc | 20 .../test.d/trigger/trigger-wprobe-btf-offset.tc | 73 + .../test.d/trigger/trigger-wprobe-btf-typecast.tc | 80 + .../ftrace/test.d/trigger/trigger-wprobe.tc | 70 + 23 files changed, 2056 insertions(+), 71 deletions(-) create mode 100644 Documentation/trace/wprobetrace.rst create mode 100644 kernel/trace/trace_wprobe.c create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/add_remove_wprobe.tc create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/wprobes_syntax_errors.tc create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-btf-offset.tc create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-btf-typecast.tc create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc
--
Masami Hiramatsu (Google) <[email protected]>
Masami Hiramatsu (Google)
maintainerPATCH v10 1/11 tracing: wprobe: Add watchpoint probe event based on hardware breakpoint
From: Masami Hiramatsu (Google) <[email protected]>
Add a new probe event for the hardware breakpoint called wprobe-event. This wprobe allows user to trace (watch) the memory access at the specified memory address.
The new syntax is;
w[:[GROUP/]EVENT] [r|w|rw]@[ADDR|SYM][:SIZE] [FETCH_ARGs]
User also can use $addr to fetch the accessed address and $value to fetch the accessed memory value (shorthand for '+0($addr)'). No other variables are supported.
For example, tracing updates of the jiffies;
/sys/kernel/tracing # echo 'w:my_jiffies w@jiffies' >> dynamic_events /sys/kernel/tracing # cat dynamic_events w:wprobes/my_jiffies w@jiffies:4
/sys/kernel/tracing # echo 1 > events/wprobes/my_jiffies/enable /sys/kernel/tracing # head -n 20 trace | tail -n 5
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
<idle>-0 [000] d.Z1. 206.547317: my_jiffies: (tick_do_update_jiffies64+0xbe/0x130)
<idle>-0 [000] d.Z1. 206.548341: my_jiffies: (tick_do_update_jiffies64+0xbe/0x130)
<idle>-0 [000] d.Z1. 206.549346: my_jiffies: (tick_do_update_jiffies64+0xbe/0x130)---
Changes in v10:
- Use zalloc_flex() in alloc_trace_wprobe().
- Support matching command arguments in trace_wprobe_match().
Changes in v9:
- Rebased on probes/for-next branch.
- Add WPROBE_NO_SIBLING error log to explicitly reject sibling probes since event triggers identify the target wprobe by event name.
- Use traceprobe_parse_event_name() to properly validate group/event names instead of using the raw command string directly.
- Generate unique event name (w_0x<addr>) for anonymous address-based watchpoints to avoid naming collisions.
- Call traceprobe_update_arg() in __register_trace_wprobe() to resolve @symbol fetch arguments, consistent with kprobe and fprobe.
Changes in v8:
- Include required header files.
- Use READ_ONCE(tw->addr) in trace handler to safely check dynamically updated addresses.
- Prohibit unsafe perf support by returning -EOPNOTSUPP in wprobe_register().
- Add rollback logic to unregister already-enabled sibling probes if registration fails mid-loop.
- Resolve symbol offsets dynamically in trace_wprobe_show() using kallsyms_lookup_name().
- Fix memory leak of parse_address_spec()'s symbol output in
__trace_wprobe_create().
- Print "rw" instead of "x" for read-write type breakpoints in
trace_wprobe_show().
- Document the $value fetcharg in wprobetrace.rst.
Changes in v7:
- Include IS_ERR_PCPU fix.
- use seq_print_ip_sym_offset().
- fix checkpatch warning on DEFINE_FREE()
- Use bp->attr.bp_addr instead of tw->addr because it can be updated from another CPU.
---
Documentation/trace/index.rst | 1
Documentation/trace/wprobetrace.rst | 70 +++
include/linux/trace_events.h | 2
kernel/trace/Kconfig | 13 +
kernel/trace/Makefile | 1
kernel/trace/trace.c | 9
kernel/trace/trace.h | 5
kernel/trace/trace_probe.c | 21 +
kernel/trace/trace_probe.h | 9
kernel/trace/trace_wprobe.c | 746 +++++++++++++++++++++++++++++++++++
10 files changed, 873 insertions(+), 4 deletions(-)
create mode 100644 Documentation/trace/wprobetrace.rst
create mode 100644 kernel/trace/trace_wprobe.cDocumentation/trace/index.rst +1 -0
@@ -36,6 +36,7 @@ the Linux kernel. kprobes kprobetrace fprobetrace + wprobetrace eprobetrace fprobe ring-buffer-design
Documentation/trace/wprobetrace.rst new file +70 -0
70 lines not on this page · raw message
include/linux/trace_events.h +2 -0
@@ -328,6 +328,7 @@ enum {TRACE_EVENT_FL_UPROBE_BIT ,TRACE_EVENT_FL_EPROBE_BIT ,TRACE_EVENT_FL_FPROBE_BIT , +TRACE_EVENT_FL_WPROBE_BIT ,TRACE_EVENT_FL_CUSTOM_BIT , TRACE_EVENT_FL_TEST_STR_BIT, };
@@ -358,6 +359,7 @@ enum {TRACE_EVENT_FL_UPROBE = (1 <<TRACE_EVENT_FL_UPROBE_BIT ),TRACE_EVENT_FL_EPROBE = (1 <<TRACE_EVENT_FL_EPROBE_BIT ),TRACE_EVENT_FL_FPROBE = (1 <<TRACE_EVENT_FL_FPROBE_BIT ), +TRACE_EVENT_FL_WPROBE = (1 <<TRACE_EVENT_FL_WPROBE_BIT ),TRACE_EVENT_FL_CUSTOM = (1 <<TRACE_EVENT_FL_CUSTOM_BIT ), TRACE_EVENT_FL_TEST_STR = (1 << TRACE_EVENT_FL_TEST_STR_BIT), };
kernel/trace/Kconfig +13 -0
@@ -862,6 +862,19 @@ config EPROBE_EVENTS convert the type of an event field. For example, turn an address into a string. +config WPROBE_EVENTS + bool "Enable wprobe-based dynamic events" + depends on TRACING + depends on HAVE_HW_BREAKPOINT + select PROBE_EVENTS + select DYNAMIC_EVENTS + help + This allows the user to add watchpoint tracing events based on + hardware breakpoints on the fly via the ftrace interface. + + Those events can be inserted wherever hardware breakpoints can be + set, and record accessed memory address and values. + config BPF_EVENTS depends on BPF_SYSCALL depends on (KPROBE_EVENTS || UPROBE_EVENTS) && PERF_EVENTS
kernel/trace/Makefile +1 -0
@@ -126,6 +126,7 @@ obj-$(CONFIG_FTRACE_RECORD_RECURSION) += trace_recursion_record.o obj-$(CONFIG_FPROBE) += fprobe.o obj-$(CONFIG_RETHOOK) += rethook.o obj-$(CONFIG_FPROBE_EVENTS) += trace_fprobe.o +obj-$(CONFIG_WPROBE_EVENTS) += trace_wprobe.o obj-$(CONFIG_TRACEPOINT_BENCHMARK) += trace_benchmark.o obj-$(CONFIG_RV) += rv/
kernel/trace/trace.c +8 -1
@@ -4294,8 +4294,12 @@ static const char readme_msg[] =" uprobe_events\t\t- Create/append/remove/show the userspace dynamic events\n" "\t\t\t Write into this file to define/undefine new trace events.\n"#endif +#ifdef CONFIG_WPROBE_EVENTS +" wprobe_events\t\t- Create/append/remove/show the hardware breakpoint dynamic events\n" + "\t\t\t Write into this file to define/undefine new trace events.\n" +#endif #if defined(CONFIG_KPROBE_EVENTS ) || defined(CONFIG_UPROBE_EVENTS ) || \ - defined(CONFIG_FPROBE_EVENTS ) + defined(CONFIG_FPROBE_EVENTS ) || defined(CONFIG_WPROBE_EVENTS ) "\t accepts: event-definitions (one definition per line)\n"#if defined(CONFIG_KPROBE_EVENTS ) || defined(CONFIG_UPROBE_EVENTS )"\t Format: p[:[<group>/][<event>]] <place> [<args>]\n"
@@ -4305,6 +4309,9 @@ static const char readme_msg[] ="\t f[:[<group>/][<event>]] <func-name>[%return] [<args>]\n" "\t t[:[<group>/][<event>]] <tracepoint> [<args>]\n"#endif +#ifdef CONFIG_WPROBE_EVENTS + "\t w[:[<group>/][<event>]] [r|w|rw]@<addr>[:<len>]\n" +#endif #ifdef CONFIG_HIST_TRIGGERS "\t s:[synthetic/]<event> <field> [<field>]\n"#endif
kernel/trace/trace.h +5 -0
@@ -179,6 +179,11 @@ struct fexit_trace_entry_head {unsigned long ret_ip; }; +struct wprobe_trace_entry_head { +struct trace_entry ent; +unsigned long ip; +}; +#define TRACE_BUF_SIZE 1024struct trace_array ;
kernel/trace/trace_probe.c +19 -2
@@ -1405,6 +1405,23 @@ static int parse_probe_vars(char *orig_arg, const struct fetch_type *t,return 0 ; } +/* wprobe only support "$addr" and "$value" variable */ +if (ctx->flags &TPARG_FL_WPROBE ) { +if (!strcmp (arg,"addr" )) { + code->op =FETCH_OP_BADDR ; +return 0 ; + } +if (!strcmp (arg,"value" )) { + code->op =FETCH_OP_BADDR ; + code++; + code->op =FETCH_OP_DEREF ; + code->offset =0 ; + *pcode = code; +return 0 ; + } + goto inval; + } +if (strcmp (arg,"comm" ) ==0 ||strcmp (arg,"COMM" ) ==0 ) { code->op =FETCH_OP_COMM ;return 0 ;
@@ -1464,8 +1481,8 @@ static int parse_probe_arg_register(char *arg, struct fetch_insn *code, {int ret; -if (ctx->flags & (TPARG_FL_TEVENT |TPARG_FL_FPROBE )) { -/* eprobe and fprobe do not handle registers */ +if (ctx->flags & (TPARG_FL_TEVENT |TPARG_FL_FPROBE |TPARG_FL_WPROBE )) { +/* eprobe, fprobe and wprobe do not handle registers */ trace_probe_log_err (ctx->offset,BAD_VAR );return -EINVAL ; }
kernel/trace/trace_probe.h +8 -1
@@ -91,6 +91,7 @@ typedef int (*print_type_func_t)(struct trace_seq *, void *, void *);FETCH_OP (STACK , param),/* Stack: .param = index */ \FETCH_OP (STACKP , none),/* Stack pointer */ \FETCH_OP (RETVAL , none),/* Return value */ \ +FETCH_OP (BADDR , none),/* Break address */ \FETCH_OP (IMM , imm),/* Immediate: .immediate */ \FETCH_OP (COMM , none),/* Current comm */ \ FETCH_OP(CURRENT, none), /* Current task_struct address */\
@@ -420,6 +421,7 @@ static inline int traceprobe_get_entry_data_size(struct trace_probe *tp)#define TPARG_FL_USER BIT(4)#define TPARG_FL_FPROBE BIT(5)#define TPARG_FL_TPOINT BIT(6) +#define TPARG_FL_WPROBE BIT(7)#define TPARG_FL_LOC_MASK GENMASK(4, 0)static inline bool tparg_is_function_entry (unsigned int flags)
@@ -546,6 +548,10 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,C (ARG_TOO_LONG ,"Argument expression is too long" ), \C (ARRAY_NO_CLOSE ,"Array is not closed" ), \C (ARRAY_TOO_BIG ,"Array number is too big" ), \ +C (BAD_ACCESS_ADDR ,"Invalid access memory address" ), \ +C (BAD_ACCESS_FMT ,"Access memory address requires @" ), \ +C (BAD_ACCESS_LEN ,"This memory access length is not supported" ), \ +C (BAD_ACCESS_TYPE ,"Bad memory access type" ), \C (BAD_ADDR_SUFFIX ,"Invalid probed address suffix" ), \C (BAD_ARG_NAME ,"Argument name must follow the same rules as C identifiers" ), \ C(BAD_ARG_NUM, "Invalid argument number"), \
@@ -628,7 +634,8 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,C (TYPECAST_NOT_EVENT ,"Typecasts are only for eprobe fields" ), \C (TYPECAST_REQ_FIELD ,"Typecast requires a field access" ), \C (TYPECAST_SYM_OFFSET ,"@SYM+/-OFFSET with typecast needs parentheses" ), \ -C (USED_ARG_NAME ,"This argument name is already used" ), +C (USED_ARG_NAME ,"This argument name is already used" ), \ +C (WPROBE_NO_SIBLING ,"Watchpoint probe does not support sibling probes" ), #undef C #define C(a, b) TP_ERR_##a
kernel/trace/trace_wprobe.c new file +746 -0
746 lines not on this page · raw message
[email protected]
AI reviewRe: [PATCH v10 01/11] tracing: wprobe: Add watchpoint probe event based on hardware breakpoint
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] Missing infrastructure for dynamic address updates leaves checks as dead code. - [Low]…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing infrastructure for dynamic address updates leaves checks as dead code.
- [Low] Misleading error message returned for malformed wprobe definitions.
- [Low] Comment refers to non-existent 'set_wprobe/clear_wprobe' triggers. --
--- Patch [1]: [PATCH v10 01/11] tracing: wprobe: Add watchpoint probe event based on hardware breakpoint --- commit d872c73e586c2b750b29132ee2dfc4a1052ce6d8
Author: Masami Hiramatsu (Google) <[email protected]>
tracing: wprobe: Add watchpoint probe event based on hardware breakpoint
Add a new probe event for the hardware breakpoint called wprobe-event. This wprobe allows user to trace (watch) the memory access at the specified memory address.
Quoted 5 lines
kernel/trace/trace_wprobe.c new file +0
-0@@ -0,0 +1,746 @@
[ ... ]
Quoted 8 lines
kernel/trace/trace_wprobe.c new file +8
-0+static void wprobe_trace_handler (struct trace_wprobe *tw, +unsigned long addr, +struct pt_regs *regs, +struct trace_event_file *trace_file) +{ +struct wprobe_trace_entry_head *entry; +struct trace_event_call *call =trace_probe_event_call (&tw->tp); +struct trace_event_buffer fbuffer;
kernel/trace/trace_wprobe.c line 140
Quoted 10 lines
kernel/trace/trace_wprobe.c new file +10
-0+int dsize; + +if (WARN_ON_ONCE (call != trace_file->event_call)) +return ; + +if (trace_trigger_soft_disabled (trace_file)) +return ; + +if (READ_ONCE (tw->addr) != addr) +return ;
[Severity: Medium]
The changelog states this uses READ_ONCE(tw->addr) to safely check dynamically updated addresses. Does this patch actually include the infrastructure to update tw->addr dynamically?
It appears tw->addr is only set during initialization, and the SMP/IRQ work update functions are absent. Does this make the check against addr effectively constant?
Quoted 2 lines
kernel/trace/trace_wprobe.c new file +2
-0+ + dsize =__get_data_size (&tw->tp, (void *)addr,NULL );
[ ... ]
Quoted 6 lines
kernel/trace/trace_wprobe.c new file +6
-0+static int register_trace_wprobe_event (struct trace_wprobe *tw) +{ +struct trace_wprobe *old_tw; +int ret; + +guard (mutex)(&event_mutex);
kernel/trace/trace_wprobe.c line 351
Quoted 10 lines
kernel/trace/trace_wprobe.c new file +10
-0+ + old_tw =find_trace_wprobe (trace_probe_name (&tw->tp), +trace_probe_group_name (&tw->tp)); +if (old_tw) { +/* +* Wprobe does not support sibling probes because the event +* trigger (set_wprobe/clear_wprobe) identifies the target +* wprobe by its event name. Having multiple wprobes sharing +* the same event name would make the target ambiguous. +*/
[Severity: Low]
This isn't a bug, but the comment references 'set_wprobe/clear_wprobe' triggers. Do these triggers actually exist in the tracing subsystem, or should this refer to standard triggers like enable_event/disable_event?
Quoted 4 lines
kernel/trace/trace_wprobe.c new file +4
-0+trace_probe_log_set_index (0 ); +trace_probe_log_err (0 ,WPROBE_NO_SIBLING ); +return -EBUSY ; + }
[ ... ]
Quoted 2 lines
kernel/trace/trace_wprobe.c new file +2
-0+static int __trace_wprobe_create (int argc,const char *argv[]) +{
[ ... ]
kernel/trace/trace_wprobe.c line 615
Quoted 6 lines
kernel/trace/trace_wprobe.c new file +6
-0+if (argv[0 ][1 ] !='\0' ) { +if (argv[0 ][1 ] !=':' ) { +trace_probe_log_set_index (0 ); +trace_probe_log_err (1 ,BAD_MAXACT_TYPE ); +return -EINVAL ; + }
[Severity: Low]
If a user provides an invalid prefix (like w10:event), this will return a BAD_MAXACT_TYPE error.
Since hardware watchpoints do not support maxactive, will logging "Maxactive is only for function exit" be misleading to users debugging their probe definitions?
Quoted 2 lines
kernel/trace/trace_wprobe.c new file +2
-0+ event = &argv[0 ][2 ]; + }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=1
Masami Hiramatsu (Google)
maintainerPATCH v10 2/11 x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Add CONFIG_HAVE_POST_BREAKPOINT_HOOK which indicates the hw_breakpoint on that architecture fires after the target memory has been modified. This is currently x86 only behavior.
--- arch/Kconfig | 10 ++++++++++ arch/x86/Kconfig | 1 + kernel/trace/Kconfig | 1 + 3 files changed, 12 insertions(+)
arch/Kconfig +10 -0
@@ -457,6 +457,16 @@ config HAVE_MIXED_BREAKPOINTS_REGS Select this option if your arch implements breakpoints under the latter fashion. +config HAVE_POST_BREAKPOINT_HOOK + bool + depends on HAVE_HW_BREAKPOINT + help + Depending on the arch implementation of hardware breakpoints, + some of them provide breakpoint hook after the target memory + is modified. + Select this option if your arch implements breakpoints overflow + handler hooks after the target memory is modified. + config HAVE_USER_RETURN_NOTIFIER bool
arch/x86/Kconfig +1 -0
@@ -246,6 +246,7 @@ config X86 select HAVE_FUNCTION_TRACER select HAVE_GCC_PLUGINS select HAVE_HW_BREAKPOINT + select HAVE_POST_BREAKPOINT_HOOK select HAVE_IOREMAP_PROT select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64 select HAVE_IRQ_TIME_ACCOUNTING
kernel/trace/Kconfig +1 -0
@@ -866,6 +866,7 @@ config WPROBE_EVENTS bool "Enable wprobe-based dynamic events" depends on TRACING depends on HAVE_HW_BREAKPOINT + depends on HAVE_POST_BREAKPOINT_HOOK select PROBE_EVENTS select DYNAMIC_EVENTS help
[email protected]
AI reviewRe: [PATCH v10 02/11] x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Alphabetical sorting violation in arch/x86/Kconfig. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Alphabetical sorting violation in arch/x86/Kconfig.
--
--- Patch [2]: [PATCH v10 02/11] x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires --- commit c9585173523a761e022d70aa1fafe72b621c85b9
Author: Masami Hiramatsu (Google) <[email protected]>
x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires
Add CONFIG_HAVE_POST_BREAKPOINT_HOOK which indicates the hw_breakpoint on that architecture fires after the target memory has been modified. This is currently x86 only behavior.
arch/x86/Kconfig config X86 · line 250
Quoted 10 lines
arch/x86/Kconfig +1
-0@@ -246,6 +246,7 @@ config X86 select HAVE_FUNCTION_TRACER select HAVE_GCC_PLUGINS select HAVE_HW_BREAKPOINT + select HAVE_POST_BREAKPOINT_HOOK select HAVE_IOREMAP_PROT
[Severity: Low]
This isn't a bug, but should HAVE_POST_BREAKPOINT_HOOK be placed further down in the list to maintain alphabetical sorting?
It is currently inserted before HAVE_IOREMAP_PROT.
Quoted 2 lines
arch/x86/Kconfig +0
-0select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64 select HAVE_IRQ_TIME_ACCOUNTING
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=2
Steven Rostedt
maintainerRe: [PATCH v10 02/11] x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires
In reply to Masami Hiramatsu (Google)kernel/trace/Kconfig config WPROBE_EVENTS · line 872
Quoted 12 lines · On Thu, 23 Jul 2026 08:02:51 +0900 "Masami Hiramatsu (Google)"…
kernel/trace/Kconfig +1
-0@@ -866,6 +866,7 @@ config WPROBE_EVENTS bool "Enable wprobe-based dynamic events" depends on TRACING depends on HAVE_HW_BREAKPOINT + depends on HAVE_POST_BREAKPOINT_HOOK select PROBE_EVENTS select DYNAMIC_EVENTS help
Is this really needed as a hard dependency?
Is it just because you want to guarantee it happens after the breakpoint? What's wrong if it happens just before it changes? We should probably have a way to note it's before. But isn't it still useful to know *where* it was changed?
-- Steve
Masami Hiramatsu (Google)
maintainerPATCH v10 3/11 selftests: tracing: Add a basic testcase for wprobe
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Add 'add_remove_wprobe.tc' testcase for testing wprobe event that tests adding and removing operations of the wprobe event.
---
Changes in v9:
- Fix command check logic to prevent early exit under 'set -e' (errexit) when grep or test fails.
- Simplify enable/disable status checks by removing cat pipes.
Changes in v8:
- Fixed silently test failure path.
--- tools/testing/selftests/ftrace/config | 1 .../ftrace/test.d/dynevent/add_remove_wprobe.tc | 63 ++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/add_remove_wprobe.tc
tools/testing/selftests/ftrace/config +1 -0
@@ -27,3 +27,4 @@ CONFIG_STACK_TRACER=y CONFIG_TRACER_SNAPSHOT=y CONFIG_UPROBES=y CONFIG_UPROBE_EVENTS=y +CONFIG_WPROBE_EVENTS=y
tools/testing/selftests/ftrace/test.d/dynevent/add_remove_wprobe.tc new file +63 -0
63 lines not on this page · raw message
Masami Hiramatsu (Google)
maintainerPATCH v10 4/11 selftests: tracing: Add syntax testcase for wprobe
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Add "wprobe_syntax_errors.tc" testcase for testing syntax errors of the watch probe events.
--- .../test.d/dynevent/wprobes_syntax_errors.tc | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/wprobes_syntax_errors.tc
tools/testing/selftests/ftrace/test.d/dynevent/wprobes_syntax_errors.tc new file +20 -0
@@ -0,0 +1,20 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 +# description: Watch probe event parser error log check +# requires: dynamic_events "w[:[<group>/][<event>]] [r|w|rw]@<addr>[:<len>]":README + +check_error() { # command-with-error-pos-by-^ + ftrace_errlog_check 'wprobe' "$1" 'dynamic_events' +} + +check_error 'w ^symbol' # BAD_ACCESS_FMT +check_error 'w ^a@symbol' # BAD_ACCESS_TYPE +check_error 'w w@^symbol' # BAD_ACCESS_ADDR +check_error 'w w@jiffies^+offset' # BAD_ACCESS_ADDR +check_error 'w w@jiffies:^100' # BAD_ACCESS_LEN +check_error 'w w@jiffies ^$arg1' # BAD_VAR +check_error 'w w@jiffies ^$retval' # BAD_VAR +check_error 'w w@jiffies ^$stack' # BAD_VAR +check_error 'w w@jiffies ^%ax' # BAD_VAR + +exit 0
Masami Hiramatsu (Google)
maintainerPATCH v10 5/11 x86/hw_breakpoint: Unify breakpoint install/uninstall
In reply to Masami Hiramatsu (Google)From: Jinchao Wang <[email protected]>
Consolidate breakpoint management to reduce code duplication. The diffstat was misleading, so the stripped code size is compared instead. After refactoring, it is reduced from 11976 bytes to 11448 bytes on my x86_64 system built with clang.
This also makes it easier to introduce arch_reinstall_hw_breakpoint().
In addition, including linux/types.h to fix a missing build dependency.
---
Changes in v8:
- Add missing barrier() on the disable path of setup_hwbp() to prevent the compiler from reordering this_cpu_write(cpu_dr7, ...) before set_debugreg(dr7, 7).
- Clear existing slot control and enable bits in cpu_dr7 inside setup_hwbp() using __encode_dr7() before setting new ones to prevent register state corruption on update/reinstall.
--- arch/x86/include/asm/hw_breakpoint.h | 6 + arch/x86/kernel/hw_breakpoint.c | 144 +++++++++++++++++++--------------- 2 files changed, 86 insertions(+), 64 deletions(-)
arch/x86/include/asm/hw_breakpoint.h +6 -0
@@ -5,6 +5,7 @@#include <uapi/asm/hw_breakpoint.h> #define __ARCH_HW_BREAKPOINT_H +#include <linux/types.h> /* * The name should probably be something dealt in
@@ -18,6 +19,11 @@ struct arch_hw_breakpoint {u8 type; }; +enum bp_slot_action { +BP_SLOT_ACTION_INSTALL , +BP_SLOT_ACTION_UNINSTALL , +}; +#include <linux/kdebug.h> #include <linux/percpu.h> #include <linux/list.h>
arch/x86/kernel/hw_breakpoint.c +80 -64
183 lines not on this page · raw message
[email protected]
AI reviewRe: [PATCH v10 05/11] x86/hw_breakpoint: Unify breakpoint install/uninstall
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Spurious debug exceptions due to modifying DR0-DR3 while enabled in DR7.
Pre-existing issues:
- [High] NMI race condition during breakpoint uninstallation leaves the hardware breakpoint permanently enabled. --
--- Patch [5]: [PATCH v10 05/11] x86/hw_breakpoint: Unify breakpoint install/uninstall --- commit 68cc4a145838d44957bc67ccfa580a6107987aa3
Author: Jinchao Wang <[email protected]>
x86/hw_breakpoint: Unify breakpoint install/uninstall
Consolidate breakpoint management to reduce code duplication. The diffstat was misleading, so the stripped code size is compared instead. After refactoring, it is reduced from 11976 bytes to 11448 bytes on my x86_64 system built with clang.
Quoted 4 lines
arch/x86/kernel/hw_breakpoint.c +0
-0
[ ... ]
arch/x86/kernel/hw_breakpoint.c int decode_dr7(unsigned long dr7, int bpnum, unsigned *len, unsigned *type) · line 134
Quoted 6 lines
arch/x86/kernel/hw_breakpoint.c +6
-0+static void setup_hwbp (struct arch_hw_breakpoint *info,int slot,bool enable) +{ +unsigned long dr7; + +set_debugreg (info->address, slot); +__this_cpu_write (cpu_debugreg[slot], info->address);
[Severity: High]
Does this code introduce a spurious debug exception when uninstalling a breakpoint?
When uninstalling (enable == false), the debug address register is modified here, but its corresponding enable bit in the hardware DR7 register is still active. According to the Intel SDM Volume 3 (Section 17.2.4), modifying DR0-DR3 without first clearing the corresponding enable bits in DR7 is forbidden. The old implementation bypassed modifying DR0-DR3 on uninstall, which avoided this issue.
Quoted 20 lines
arch/x86/kernel/hw_breakpoint.c +20
-0+ + dr7 =this_cpu_read (cpu_dr7); + dr7 &= ~(__encode_dr7 (slot,0xc ,0x3 ) | + (DR_LOCAL_ENABLE << (slot *DR_ENABLE_SIZE ))); +if (enable) + dr7 |=encode_dr7 (slot, info->len, info->type); + +/* +* Enabling: +* Ensure we first write cpu_dr7 before we set the DR7 register. +* This ensures an NMI never see cpu_dr7 0 when DR7 is not. +*/ +if (enable) +this_cpu_write (cpu_dr7, dr7); + +barrier (); + +set_debugreg (dr7,7 ); + +amd_set_dr_addr_mask (enable ? info->mask :0 , slot);
arch/x86/kernel/hw_breakpoint.c
Quoted 10 lines
arch/x86/kernel/hw_breakpoint.c +10
-0+ +barrier (); + +/* +* Disabling: +* Ensure the write to cpu_dr7 is after we've set the DR7 register. +* This ensures an NMI never see cpu_dr7 0 when DR7 is not. +*/ +if (!enable) +this_cpu_write (cpu_dr7, dr7);
[Severity: High]
This is a pre-existing issue, but does this sequence create an NMI race condition that could leave the hardware breakpoint permanently enabled?
When uninstalling, this code writes to the hardware DR7 register first (set_debugreg(dr7, 7)), issues a barrier, and then updates the per-CPU tracking variable (this_cpu_write(cpu_dr7, dr7)).
If an NMI (like SysRq-G for KGDB) fires in the window between these writes, the NMI handler (such as kgdb_correct_hw_break()) will call hw_breakpoint_restore(). This reads the stale cpu_dr7 (which still has the slot enabled) and writes it back to hardware DR7, re-enabling the breakpoint.
When the NMI returns, setup_hwbp() continues and writes the disabled state to cpu_dr7 in memory, but the hardware DR7 is left enabled. If the breakpoint triggers later, hw_breakpoint_handler() will see a NULL bp_per_reg and could enter an infinite #DB loop.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=5
Masami Hiramatsu (Google)
maintainerPATCH v10 6/11 x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint
In reply to Masami Hiramatsu (Google)From: Jinchao Wang <[email protected]>
The new arch_reinstall_hw_breakpoint() function can be used in an atomic context, unlike the more expensive free and re-allocation path. This allows callers to efficiently re-establish an existing breakpoint.
---
Changes in v9:
- Update commit message.
- Temporarily disable the active slot in setup_hwbp() before updating the address register to avoid spurious debug exceptions.
--- arch/x86/include/asm/hw_breakpoint.h | 2 ++ arch/x86/kernel/hw_breakpoint.c | 34 ++++++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-)
arch/x86/include/asm/hw_breakpoint.h +2 -0
@@ -21,6 +21,7 @@ struct arch_hw_breakpoint {enum bp_slot_action {BP_SLOT_ACTION_INSTALL , +BP_SLOT_ACTION_REINSTALL ,BP_SLOT_ACTION_UNINSTALL , };
@@ -65,6 +66,7 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,int arch_install_hw_breakpoint (struct perf_event *bp); +int arch_reinstall_hw_breakpoint (struct perf_event *bp);void arch_uninstall_hw_breakpoint (struct perf_event *bp);void hw_breakpoint_pmu_read (struct perf_event *bp);void hw_breakpoint_pmu_unthrottle (struct perf_event *bp);
arch/x86/kernel/hw_breakpoint.c +28 -6
@@ -100,6 +100,10 @@ static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action) old_bp =NULL ; new_bp = bp;break ; +case BP_SLOT_ACTION_REINSTALL : + old_bp = bp; + new_bp = bp; +break ;case BP_SLOT_ACTION_UNINSTALL : old_bp = bp; new_bp =NULL ;
@@ -129,23 +133,36 @@ static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)static void setup_hwbp (struct arch_hw_breakpoint *info,int slot,bool enable) {unsigned long dr7; - -set_debugreg (info->address, slot); -__this_cpu_write (cpu_debugreg[slot], info->address); +bool enabled; dr7 =this_cpu_read (cpu_dr7); + enabled = dr7 & ((DR_LOCAL_ENABLE |DR_GLOBAL_ENABLE ) << (slot *DR_ENABLE_SIZE )); dr7 &= ~(__encode_dr7 (slot,0xc ,0x3 ) | (DR_LOCAL_ENABLE << (slot *DR_ENABLE_SIZE ))); -if (enable) - dr7 |=encode_dr7 (slot, info->len, info->type); + +/* +* If the slot is currently enabled, disable it first before updating +* the address register to prevent spurious debug exceptions. +*/ +if (enable && enabled) { +barrier (); +set_debugreg (dr7,7 ); +barrier (); +this_cpu_write (cpu_dr7, dr7); + } + +set_debugreg (info->address, slot); +__this_cpu_write (cpu_debugreg[slot], info->address);/* * Enabling: * Ensure we first write cpu_dr7 before we set the DR7 register. * This ensures an NMI never see cpu_dr7 0 when DR7 is not. */ -if (enable) +if (enable) { + dr7 |=encode_dr7 (slot, info->len, info->type);this_cpu_write (cpu_dr7, dr7); + }barrier ();
@@ -189,6 +206,11 @@ int arch_install_hw_breakpoint(struct perf_event *bp)return arch_manage_bp (bp,BP_SLOT_ACTION_INSTALL ); } +int arch_reinstall_hw_breakpoint (struct perf_event *bp) +{ +return arch_manage_bp (bp,BP_SLOT_ACTION_REINSTALL ); +} +void arch_uninstall_hw_breakpoint (struct perf_event *bp) {arch_manage_bp (bp,BP_SLOT_ACTION_UNINSTALL );
[email protected]
AI reviewRe: [PATCH v10 06/11] x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Spurious debug exceptions during reinstall of AMD range breakpoints due to incorrect hardware…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Spurious debug exceptions during reinstall of AMD range breakpoints due to incorrect hardware register update ordering. --
--- Patch [6]: [PATCH v10 06/11] x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint --- commit f97fcd502b5557f06d1cd0cadf2259bf2197dc51
Author: Jinchao Wang <[email protected]>
x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint
The new arch_reinstall_hw_breakpoint() function can be used in an atomic context, unlike the more expensive free and re-allocation path. This allows callers to efficiently re-establish an existing breakpoint.
Quoted 4 lines
arch/x86/kernel/hw_breakpoint.c +0
-0
[ ... ]
Quoted 32 lines
arch/x86/kernel/hw_breakpoint.c +16
-5@@ -129,23 +133,36 @@ static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)static void setup_hwbp (struct arch_hw_breakpoint *info,int slot,bool enable) {unsigned long dr7; - -set_debugreg (info->address, slot); -__this_cpu_write (cpu_debugreg[slot], info->address); +bool enabled; dr7 =this_cpu_read (cpu_dr7); + enabled = dr7 & ((DR_LOCAL_ENABLE |DR_GLOBAL_ENABLE ) << (slot *DR_ENABLE_SIZE )); dr7 &= ~(__encode_dr7 (slot,0xc ,0x3 ) | (DR_LOCAL_ENABLE << (slot *DR_ENABLE_SIZE ))); -if (enable) - dr7 |=encode_dr7 (slot, info->len, info->type); + +/* +* If the slot is currently enabled, disable it first before updating +* the address register to prevent spurious debug exceptions. +*/ +if (enable && enabled) { +barrier (); +set_debugreg (dr7,7 ); +barrier (); +this_cpu_write (cpu_dr7, dr7); + } + +set_debugreg (info->address, slot); +__this_cpu_write (cpu_debugreg[slot], info->address);/* * Enabling:
arch/x86/kernel/hw_breakpoint.c static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action) · line 167
Quoted 10 lines
arch/x86/kernel/hw_breakpoint.c +3
-1* Ensure we first write cpu_dr7 before we set the DR7 register. * This ensures an NMI never see cpu_dr7 0 when DR7 is not. */ -if (enable) +if (enable) { + dr7 |=encode_dr7 (slot, info->len, info->type);this_cpu_write (cpu_dr7, dr7); + }barrier ();
[Severity: Medium]
Could this ordering cause a spurious debug exception when reinstalling a breakpoint with a narrowed address mask?
When arch_reinstall_hw_breakpoint() is called to narrow the mask of an active AMD breakpoint (e.g., changing from a wide range mask like 0xFF to an exact match 0x0), setup_hwbp() re-enables the breakpoint in DR7 via set_debugreg(dr7, 7) just after this barrier, but before the AMD address mask is updated via amd_set_dr_addr_mask().
If an NMI occurs in this small window, or if the memory accesses of amd_set_dr_addr_mask() itself fall within the old wider range around the new address, it seems a spurious #DB exception could fire.
Should the mask be updated before re-enabling the breakpoint in DR7?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=6
Masami Hiramatsu (Google)
maintainerPATCH v10 7/11 HWBP: Add modify_wide_hw_breakpoint_local() API
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Add modify_wide_hw_breakpoint_local() arch-wide interface which allows hwbp users to update watch address on-line. This is available if the arch supports CONFIG_HAVE_REINSTALL_HW_BREAKPOINT.
Note that this allows to change the type only for compatible types, because it does not release and reserve the hwbp slot based on type. For instance, you can not change HW_BREAKPOINT_W to HW_BREAKPOINT_X.
---
Changes in v9:
- Add lockdep_assert_irqs_disabled() to enforce and document the interrupt-disabled calling context requirement.
- Implement state rollback logic in modify_wide_hw_breakpoint_local() to prevent inconsistent state on architecture update failure.
Changes in v8:
- Parse new attributes into a temporary struct arch_hw_breakpoint copy to prevent in-place mutation and corruption on parse error paths.
- Synchronize logical perf_event attributes by updating bp->attr.bp_type and bp->attr.bp_len in modify_wide_hw_breakpoint_local().
Changes in v7:
- Update bp->attr.bp_attr so that we can correctly check the address on it.
- Use -EOPNOTSUPP instead of -ENOSYS.
Changes in v4:
- Update kerneldoc comment about modify_wide_hw_breakpoint_local according to Randy's comment.
Changes in v2:
- Check type compatibility by checking slot. (Thanks Jinchao!)
--- arch/Kconfig | 10 +++++++ arch/x86/Kconfig | 1 + include/linux/hw_breakpoint.h | 6 ++++ kernel/events/hw_breakpoint.c | 61 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+)
arch/Kconfig +10 -0
@@ -467,6 +467,16 @@ config HAVE_POST_BREAKPOINT_HOOK Select this option if your arch implements breakpoints overflow handler hooks after the target memory is modified. +config HAVE_REINSTALL_HW_BREAKPOINT + bool + depends on HAVE_HW_BREAKPOINT + help + Depending on the arch implementation of hardware breakpoints, + some of them are able to update the breakpoint configuration + without release and reserve the hardware breakpoint register. + What configuration is able to update depends on hardware and + software implementation. + config HAVE_USER_RETURN_NOTIFIER bool
arch/x86/Kconfig +1 -0
@@ -247,6 +247,7 @@ config X86 select HAVE_GCC_PLUGINS select HAVE_HW_BREAKPOINT select HAVE_POST_BREAKPOINT_HOOK + select HAVE_REINSTALL_HW_BREAKPOINT select HAVE_IOREMAP_PROT select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64 select HAVE_IRQ_TIME_ACCOUNTING
include/linux/hw_breakpoint.h +6 -0
@@ -81,6 +81,9 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,perf_overflow_handler_t triggered, void *context); +extern int modify_wide_hw_breakpoint_local (struct perf_event *bp, +struct perf_event_attr *attr); +extern int register_perf_hw_breakpoint (struct perf_event *bp);extern void unregister_hw_breakpoint (struct perf_event *bp);extern void unregister_wide_hw_breakpoint (struct perf_event *__percpu *cpu_events);
@@ -124,6 +127,9 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,perf_overflow_handler_t triggered, void *context) { return NULL; }static inline int +modify_wide_hw_breakpoint_local (struct perf_event *bp, +struct perf_event_attr *attr) {return -EOPNOTSUPP ; } +static inline int register_perf_hw_breakpoint (struct perf_event *bp) {return -ENOSYS ; }static inline void unregister_hw_breakpoint (struct perf_event *bp) { } static inline void
kernel/events/hw_breakpoint.c +61 -0
67 lines not on this page · raw message
cover hw_breakpoint: Fix and simplify local address updates for wprobe
Hi Masami,
While reviewing wprobe v10, I prepared three patches to fix and simplify the HWBP support required by wprobe.
This series is based on linux-trace/probes/for-next, the same base used by wprobe v10. It is intended to replace patches 5-7 in v10, so it should not be applied on top of topic/wprobe-v2.
Patch 1 fixes the x86 HWBP slot and DR7 races when an operation is interrupted by an NMI.
Patch 2 adds arch_modify_local_hw_breakpoint_addr(), which updates only the watched address of an installed breakpoint on the local CPU.
Patch 3 adds the matching generic modify_local_hw_breakpoint_addr() API. It keeps the original patch structure and Masami's authorship, while removing attribute parsing, type/length updates and rollback.
With the address-only interface, the install/uninstall refactoring in the old patch 5 is no longer needed.
I can take care of follow-up maintenance related to fast local HWBP watched-address updates.
The patches passed checkpatch and the relevant x86_32 and x86_64 object builds.
Jinchao Wang (2):
x86/hw_breakpoints: Make DR7 updates NMI safe x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API
Masami Hiramatsu (Google) (1):
HWBP: Add modify_local_hw_breakpoint_addr() API
arch/Kconfig | 8 ++ arch/x86/Kconfig | 1 + arch/x86/include/asm/debugreg.h | 36 ++++++--- arch/x86/include/asm/hw_breakpoint.h | 2 + arch/x86/kernel/cpu/mce/core.c | 20 +++-- arch/x86/kernel/hw_breakpoint.c | 114 ++++++++++++++++----------- arch/x86/kernel/nmi.c | 10 ++- arch/x86/kernel/traps.c | 10 ++- include/linux/hw_breakpoint.h | 6 ++ kernel/events/hw_breakpoint.c | 30 +++++++ 10 files changed, 166 insertions(+), 71 deletions(-)
base-commit: 1a416ae446afa42d2d8500ce25bd61c564508721
Signature · 1 line
2.53.0
PATCH 1/3 x86/hw_breakpoints: Make DR7 updates NMI safe
Hardware breakpoint installation and removal run with IRQs disabled, but an NMI can still enter the same code through KGDB. The interrupted operation and the NMI can consequently claim the same slot or overwrite each other's DR7 state.
Claim and release per-CPU slots with cmpxchg. Update cpu_dr7 with single-instruction per-CPU operations, and preserve hardware-first disable and hardware-last enable ordering. Add a per-CPU sequence number so interrupted DR7 writers and restore paths detect an NMI update and retry from the latest shadow state.
--- arch/x86/include/asm/debugreg.h | 36 ++++++++---- arch/x86/kernel/cpu/mce/core.c | 20 ++++--- arch/x86/kernel/hw_breakpoint.c | 98 ++++++++++++++++----------------- arch/x86/kernel/nmi.c | 10 +++- arch/x86/kernel/traps.c | 10 +++- 5 files changed, 101 insertions(+), 73 deletions(-)
arch/x86/include/asm/debugreg.h +26 -10
@@ -18,6 +18,7 @@#define DR7_FIXED_1 0x00000400 DECLARE_PER_CPU(unsigned long, cpu_dr7); +DECLARE_PER_CPU(unsigned int, cpu_dr7_seq);#ifndef CONFIG_PARAVIRT_XXL /*
@@ -125,18 +126,20 @@ static __always_inline bool hw_breakpoint_active(void)extern void hw_breakpoint_restore (void ); -static __always_inline unsigned long local_db_save (void ) +static __always_inline void local_db_save(unsigned long *dr7, +unsigned int *dr7_seq) { -unsigned long dr7; + *dr7 =0 ; + *dr7_seq =this_cpu_read (cpu_dr7_seq);if (static_cpu_has (X86_FEATURE_HYPERVISOR ) && !hw_breakpoint_active ()) -return 0 ; +return ; -get_debugreg (dr7,7 ); +get_debugreg (*dr7,7 );/* Architecturally set bit */ - dr7 &= ~DR7_FIXED_1 ; -if (dr7) + *dr7 &= ~DR7_FIXED_1 ; +if (*dr7)set_debugreg (DR7_FIXED_1 ,7 );/*
@@ -145,20 +148,33 @@ static __always_inline unsigned long local_db_save(void)* be good. */ barrier (); - -return dr7; } -static __always_inline void local_db_restore(unsigned long dr7) +static __always_inline void local_db_restore(unsigned long dr7, +unsigned int dr7_seq) { +unsigned int seq; +/* * Ensure the compiler doesn't raise this statement into * the critical section; enabling breakpoints early would * not be good. */ barrier (); -if (dr7) + +do { + seq =this_cpu_read (cpu_dr7_seq); +if (seq == dr7_seq) { +if (!dr7) +return ; + }else { + dr7 =this_cpu_read (cpu_dr7); +if (!dr7) + dr7 =DR7_FIXED_1 ; + } +set_debugreg (dr7,7 ); + }while (unlikely (seq !=this_cpu_read (cpu_dr7_seq))); }#ifdef CONFIG_CPU_SUP_AMD
arch/x86/kernel/cpu/mce/core.c +12 -8
@@ -2139,20 +2139,22 @@ static __always_inline void exc_machine_check_user(struct pt_regs *regs)DEFINE_IDTENTRY_MCE (exc_machine_check) {unsigned long dr7; +unsigned int dr7_seq; - dr7 =local_db_save (); +local_db_save (&dr7, &dr7_seq);exc_machine_check_kernel (regs); -local_db_restore (dr7); +local_db_restore (dr7, dr7_seq); }/* The user mode variant. */ DEFINE_IDTENTRY_MCE_USER (exc_machine_check) {unsigned long dr7; +unsigned int dr7_seq; - dr7 =local_db_save (); +local_db_save (&dr7, &dr7_seq);exc_machine_check_user (regs); -local_db_restore (dr7); +local_db_restore (dr7, dr7_seq); }#ifdef CONFIG_X86_FRED
@@ -2170,13 +2172,14 @@ DEFINE_IDTENTRY_MCE_USER(exc_machine_check)DEFINE_FREDENTRY_MCE (exc_machine_check) {unsigned long dr7; +unsigned int dr7_seq; - dr7 =local_db_save (); +local_db_save (&dr7, &dr7_seq);if (user_mode (regs))exc_machine_check_user (regs);else exc_machine_check_kernel (regs); -local_db_restore (dr7); +local_db_restore (dr7, dr7_seq); }#endif #else
@@ -2184,13 +2187,14 @@ DEFINE_FREDENTRY_MCE(exc_machine_check)DEFINE_IDTENTRY_RAW (exc_machine_check) {unsigned long dr7; +unsigned int dr7_seq; - dr7 =local_db_save (); +local_db_save (&dr7, &dr7_seq);if (user_mode (regs))exc_machine_check_user (regs);else exc_machine_check_kernel (regs); -local_db_restore (dr7); +local_db_restore (dr7, dr7_seq); }#endif
arch/x86/kernel/hw_breakpoint.c +48 -50
153 lines not on this page · raw message
arch/x86/kernel/nmi.c +8 -2
@@ -532,10 +532,13 @@ enum nmi_states { static DEFINE_PER_CPU(enum nmi_states, nmi_state); static DEFINE_PER_CPU(unsigned long, nmi_cr2); static DEFINE_PER_CPU(unsigned long, nmi_dr7); +static DEFINE_PER_CPU(unsigned int, nmi_dr7_seq);DEFINE_IDTENTRY_RAW (exc_nmi ) {irqentry_state_t irq_state; +unsigned long dr7; +unsigned int dr7_seq;struct nmi_stats *nsp =this_cpu_ptr (&nmi_stats);/*
@@ -572,7 +575,9 @@ DEFINE_IDTENTRY_RAW(exc_nmi)*/ sev_es_ist_enter (regs); -this_cpu_write (nmi_dr7,local_db_save ()); +local_db_save (&dr7, &dr7_seq); +this_cpu_write (nmi_dr7, dr7); +this_cpu_write (nmi_dr7_seq, dr7_seq); irq_state =irqentry_nmi_enter (regs);
@@ -594,7 +599,8 @@ DEFINE_IDTENTRY_RAW(exc_nmi)irqentry_nmi_exit (regs, irq_state); -local_db_restore (this_cpu_read (nmi_dr7)); +local_db_restore (this_cpu_read (nmi_dr7), +this_cpu_read (nmi_dr7_seq));sev_es_ist_exit ();
arch/x86/kernel/traps.c +7 -3
@@ -1231,8 +1231,12 @@ static noinstr void exc_debug_kernel(struct pt_regs *regs, unsigned long dr6)* it results in an endless recursion and stack overflow. Thus we stay * with the IDT approach, i.e., save DR7 and disable #DB. */ -unsigned long dr7 =local_db_save (); -irqentry_state_t irq_state =irqentry_nmi_enter (regs); +unsigned long dr7; +unsigned int dr7_seq; +irqentry_state_t irq_state; + +local_db_save (&dr7, &dr7_seq); + irq_state =irqentry_nmi_enter (regs);instrumentation_begin ();/*
@@ -1289,7 +1293,7 @@ static noinstr void exc_debug_kernel(struct pt_regs *regs, unsigned long dr6)instrumentation_end ();irqentry_nmi_exit (regs, irq_state); -local_db_restore (dr7); +local_db_restore (dr7, dr7_seq); } static noinstr void exc_debug_user(struct pt_regs *regs, unsigned long dr6)
Signature · 1 line
2.53.0
PATCH 2/3 x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API
In reply to Jinchao WangWprobe needs to move an active per-CPU watchpoint without releasing and reserving its hardware slot.
Add arch_modify_local_hw_breakpoint_addr() to find the installed local slot and update only its address shadow and hardware debug address register. Publish the shadow first so hw_breakpoint_restore() observes the new address if an NMI interrupts the update.
The caller must provide an installed local event and a valid address. Slot ownership, breakpoint type, length, mask and DR7 remain unchanged.
--- arch/x86/include/asm/hw_breakpoint.h | 2 ++ arch/x86/kernel/hw_breakpoint.c | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+)
arch/x86/include/asm/hw_breakpoint.h +2 -0
@@ -59,6 +59,8 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,int arch_install_hw_breakpoint (struct perf_event *bp); +void arch_modify_local_hw_breakpoint_addr (struct perf_event *bp, +unsigned long addr);void arch_uninstall_hw_breakpoint (struct perf_event *bp);void hw_breakpoint_pmu_read (struct perf_event *bp);void hw_breakpoint_pmu_unthrottle (struct perf_event *bp);
arch/x86/kernel/hw_breakpoint.c +20 -0
@@ -128,6 +128,26 @@ int arch_install_hw_breakpoint(struct perf_event *bp)return 0 ; } +void arch_modify_local_hw_breakpoint_addr (struct perf_event *bp, +unsigned long addr) +{ +int i; + +lockdep_assert_irqs_disabled (); + +for (i =0 ; i <HBP_NUM ; i++) { +if (this_cpu_read (bp_per_reg[i]) == bp) +break ; + } + +if (WARN_ONCE (i ==HBP_NUM ,"Can't find any breakpoint slot" )) +return ; + +this_cpu_write (cpu_debugreg[i], addr); +barrier (); +set_debugreg (addr, i); +} +/* * Uninstall the breakpoint contained in the given counter. *
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH 2/3] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] arch_modify_local_hw_breakpoint_addr() fails to increment cpu_dr7_seq, causing a race condition…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arch_modify_local_hw_breakpoint_addr() fails to increment cpu_dr7_seq, causing a race condition that can desynchronize hardware debug registers from their shadow state if interrupted by an NMI.
- [High] arch_modify_local_hw_breakpoint_addr() fails to update the primary breakpoint shadow address (info->address), causing future updates to silently revert the address. --
--- Patch [2]: [PATCH 2/3] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API --- commit 91337a1d81005389f6b66cba9728d9f95a16cec3
Author: Jinchao Wang <[email protected]>
x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API
Wprobe needs to move an active per-CPU watchpoint without releasing and reserving its hardware slot. This adds a new API to find the installed local slot and update only its address shadow and hardware debug address register.
Quoted 4 lines
arch/x86/kernel/hw_breakpoint.c +0
-0
[ ... ]
Quoted 7 lines
arch/x86/kernel/hw_breakpoint.c +0
-0@@ -128,6 +128,26 @@ int arch_install_hw_breakpoint(struct perf_event *bp)void arch_modify_local_hw_breakpoint_addr (struct perf_event *bp,unsigned long addr) {int i;
arch/x86/kernel/hw_breakpoint.c int arch_install_hw_breakpoint(struct perf_event *bp)
Quoted 10 lines
arch/x86/kernel/hw_breakpoint.c +0
-0lockdep_assert_irqs_disabled ();for (i =0 ; i <HBP_NUM ; i++) {if (this_cpu_read (bp_per_reg[i]) == bp)break ; }if (WARN_ONCE (i ==HBP_NUM ,"Can't find any breakpoint slot" ))return ;
[Severity: High]
Does this missing cpu_dr7_seq increment leave a race condition if interrupted by an NMI?
If hw_breakpoint_restore() is running, reads the old shadow value, and is interrupted by an NMI that calls this function to update the address, the lack of a sequence bump means hw_breakpoint_restore() will not detect the interruption. It would exit its retry loop and overwrite the hardware register with the stale value.
[Severity: High]
Could this also cause the watchpoint to silently revert to its previous value when rescheduled?
Since the perf_event's internal info->address (retrieved via counter_arch_bp(bp)) is not updated here, any future call to arch_install_hw_breakpoint() during PMU multiplexing or CPU hotplug would reinstall the breakpoint using that stale address.
Quoted 4 lines
arch/x86/kernel/hw_breakpoint.c +0
-0this_cpu_write (cpu_debugreg[i], addr);barrier ();set_debugreg (addr, i); }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH 3/3 HWBP: Add modify_local_hw_breakpoint_addr() API
In reply to Jinchao WangFrom: "Masami Hiramatsu (Google)" <[email protected]>
Add modify_local_hw_breakpoint_addr() to update only the watched address of an installed hardware breakpoint on the local CPU without releasing and reserving its hardware slot. This is available when the architecture selects CONFIG_HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR.
The caller must provide an installed local event and a valid address, and update other CPUs separately.
--- arch/Kconfig | 8 ++++++++ arch/x86/Kconfig | 1 + include/linux/hw_breakpoint.h | 6 ++++++ kernel/events/hw_breakpoint.c | 30 ++++++++++++++++++++++++++++++ 4 files changed, 45 insertions(+)
arch/Kconfig +8 -0
@@ -457,6 +457,14 @@ config HAVE_MIXED_BREAKPOINTS_REGS Select this option if your arch implements breakpoints under the latter fashion. +config HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR + bool + depends on HAVE_HW_BREAKPOINT + help + Select this if the architecture can modify the address of an + installed hardware breakpoint on the local CPU without releasing + and reserving its hardware slot. + config HAVE_USER_RETURN_NOTIFIER bool
arch/x86/Kconfig +1 -0
@@ -246,6 +246,7 @@ config X86 select HAVE_FUNCTION_TRACER select HAVE_GCC_PLUGINS select HAVE_HW_BREAKPOINT + select HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR select HAVE_IOREMAP_PROT select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64 select HAVE_IRQ_TIME_ACCOUNTING
include/linux/hw_breakpoint.h +6 -0
@@ -81,6 +81,9 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,perf_overflow_handler_t triggered, void *context); +int modify_local_hw_breakpoint_addr (struct perf_event *bp, +unsigned long addr); +extern int register_perf_hw_breakpoint (struct perf_event *bp);extern void unregister_hw_breakpoint (struct perf_event *bp);extern void unregister_wide_hw_breakpoint (struct perf_event *__percpu *cpu_events);
@@ -124,6 +127,9 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,perf_overflow_handler_t triggered, void *context) { return NULL; }static inline int +modify_local_hw_breakpoint_addr (struct perf_event *bp, +unsigned long addr) {return -EOPNOTSUPP ; } +static inline int register_perf_hw_breakpoint (struct perf_event *bp) {return -ENOSYS ; }static inline void unregister_hw_breakpoint (struct perf_event *bp) { } static inline void
kernel/events/hw_breakpoint.c +30 -0
@@ -888,6 +888,36 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events) }EXPORT_SYMBOL_GPL (unregister_wide_hw_breakpoint); +/** +* modify_local_hw_breakpoint_addr - update a local breakpoint address +* @bp: the hwbp perf event for this CPU +* @addr: the new address for @bp +* +* Update only the address of an installed breakpoint on the local CPU without +* releasing and reserving its hardware slot. The caller must update other CPUs. +* Return 0, or -EOPNOTSUPP if the architecture does not support this operation. +*/ +#ifdef CONFIG_HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR +int modify_local_hw_breakpoint_addr (struct perf_event *bp, +unsigned long addr) +{ +lockdep_assert_irqs_disabled (); + +counter_arch_bp (bp)->address = addr; + bp->attr.bp_addr = addr; +arch_modify_local_hw_breakpoint_addr (bp, addr); + +return 0 ; +} +#else +int modify_local_hw_breakpoint_addr (struct perf_event *bp, +unsigned long addr) +{ +return -EOPNOTSUPP ; +} +#endif +EXPORT_SYMBOL_GPL (modify_local_hw_breakpoint_addr); +/** * hw_breakpoint_is_used - check if breakpoints are currently used *
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH 3/3] HWBP: Add modify_local_hw_breakpoint_addr() API
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] arch_modify_local_hw_breakpoint_addr() lacks NMI-safe sequence counter protection and violates the…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arch_modify_local_hw_breakpoint_addr() lacks NMI-safe sequence counter protection and violates the x86 SDM by modifying active debug registers.
- [High] modify_local_hw_breakpoint_addr() bypasses mandatory architecture-specific address validation, allowing safe-guards to be circumvented.
- [Medium] modify_local_hw_breakpoint_addr() updates generic software state locklessly before calling the arch-specific function, and returns success even if the arch function fails. --
--- Patch [3]: [PATCH 3/3] HWBP: Add modify_local_hw_breakpoint_addr() API --- commit 46c7b1b307ec0fc122a4f26e8e8f3b876fbd24b7
Author: Masami Hiramatsu (Google) <[email protected]>
HWBP: Add modify_local_hw_breakpoint_addr() API
Add modify_local_hw_breakpoint_addr() to update only the watched address of an installed hardware breakpoint on the local CPU without releasing and reserving its hardware slot. This is available when the architecture selects CONFIG_HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR.
The caller must provide an installed local event and a valid address, and update other CPUs separately.
[ ... ]
Quoted 15 lines
kernel/events/hw_breakpoint.c +7
-0@@ -888,6 +888,36 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events) }EXPORT_SYMBOL_GPL (unregister_wide_hw_breakpoint); +/** +* modify_local_hw_breakpoint_addr - update a local breakpoint address +* @bp: the hwbp perf event for this CPU +* @addr: the new address for @bp +* +* Update only the address of an installed breakpoint on the local CPU without +* releasing and reserving its hardware slot. The caller must update other CPUs.
kernel/events/hw_breakpoint.c void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events) · line 907
Quoted 10 lines
kernel/events/hw_breakpoint.c +10
-0+* Return 0, or -EOPNOTSUPP if the architecture does not support this operation. +*/ +#ifdef CONFIG_HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR +int modify_local_hw_breakpoint_addr (struct perf_event *bp, +unsigned long addr) +{ +lockdep_assert_irqs_disabled (); + +counter_arch_bp (bp)->address = addr; + bp->attr.bp_addr = addr;
[Severity: High]
Is mandatory architecture-specific address validation bypassed here?
The normal breakpoint installation path uses hw_breakpoint_arch_parse() to ensure the address is safe (e.g., properly aligned, canonical, and not overlapping with critical CPU entry areas like cpu_dr7_seq or the GDT).
By blindly assigning the new address and passing it to the architecture layer without invoking hw_breakpoint_arch_parse() or any architectural validation, modify_local_hw_breakpoint_addr() bypasses all these safe-guards.
Since generic code cannot know x86-specific exclusion zones, any caller supplying an invalid address will directly program the hardware, potentially triggering recursive exceptions that hang the system.
kernel/events/hw_breakpoint.c void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events) · line 908
Quoted 1 line
kernel/events/hw_breakpoint.c +1
-0+arch_modify_local_hw_breakpoint_addr (bp, addr);
[Severity: High]
Are we missing NMI-safe sequence counter protection in arch_modify_local_hw_breakpoint_addr()?
Looking at arch_modify_local_hw_breakpoint_addr() in arch/x86/kernel/hw_breakpoint.c, it modifies the CPU debug register directly without incrementing cpu_dr7_seq to protect against NMI races, and without temporarily disabling the breakpoint in DR7:
this_cpu_write(cpu_debugreg[i], addr);
barrier();
set_debugreg(addr, i);If an NMI occurs inside hw_breakpoint_restore() between reading a debug register and writing it, and the NMI handler uses modify_local_hw_breakpoint_addr() to update the same debug register, the sequence counter (cpu_dr7_seq) is not incremented.
When hw_breakpoint_restore() resumes, it will overwrite the debug register with the old stale address without detecting the interference, while the software state retains the new address.
Additionally, does writing directly to DR0-DR3 while the breakpoint is enabled in DR7 violate the Intel SDM (Vol 3, 18.2.2)? This could result in hardware and software breakpoint state becoming permanently desynced, and undefined hardware behavior may occur.
kernel/events/hw_breakpoint.c void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events) · line 910
Quoted 2 lines
kernel/events/hw_breakpoint.c +2
-0+ +return 0 ;
[Severity: Medium]
Could this unconditionally return success even if the underlying architecture function fails?
If a caller passes an inactive or remote-CPU hardware breakpoint to modify_local_hw_breakpoint_addr(), the generic code unconditionally updates bp->attr.bp_addr and counter_arch_bp(bp)->address locklessly before calling the arch-specific function.
The arch_modify_local_hw_breakpoint_addr() function will fail to find the slot, trigger a WARN_ONCE, and return early without modifying hardware.
However, since it returns void, the generic function unconditionally returns 0 (success). This leaves the software and hardware states permanently desynced.
Quoted 1 line
kernel/events/hw_breakpoint.c +1
-0+}
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Masami Hiramatsu (Google)
maintainerRe: [PATCH 0/3] hw_breakpoint: Fix and simplify local address updates for wprobe
In reply to Jinchao WangQuoted 4 lines · On Sun, 26 Jul 2026 20:22:29 +0800 Jinchao Wang <[email protected]>…
Hi Masami,
While reviewing wprobe v10, I prepared three patches to fix and simplify the HWBP support required by wprobe.
Hi Jinchao,
Thanks for the series!
Quoted 4 lines
This series is based on linux-trace/probes/for-next, the same base used by wprobe v10. It is intended to replace patches 5-7 in v10, so it should not be applied on top of topic/wprobe-v2.
OK, let me replace it with this series.
I'll also update wprobe side and make it v11.
Quoted 7 lines
Patch 1 fixes the x86 HWBP slot and DR7 races when an operation is interrupted by an NMI.
Patch 2 adds
arch_modify_local_hw_breakpoint_addr(), which updates only the watched address of an installed breakpoint on the local CPU.
Quoted 6 lines
Patch 3 adds the matching generic
modify_local_hw_breakpoint_addr()API. It keeps the original patch structure and Masami's authorship, while removing attribute parsing, type/length updates and rollback.With the address-only interface, the install/uninstall refactoring in the old patch 5 is no longer needed.
OK.
Quoted 6 lines
I can take care of follow-up maintenance related to fast local HWBP watched-address updates.
The patches passed checkpatch and the relevant
x86_32andx86_64object builds.
Thanks!
Quoted 24 lines
Jinchao Wang (2):
x86/hw_breakpoints: Make DR7 updates NMI safe x86/hw_breakpoints: Addarch_modify_local_hw_breakpoint_addr()APIMasami Hiramatsu (Google) (1):
HWBP: Addmodify_local_hw_breakpoint_addr()APIarch/Kconfig | 8 ++ arch/x86/Kconfig | 1 + arch/x86/include/asm/debugreg.h | 36 ++++++--- arch/x86/include/asm/hw_breakpoint.h | 2 + arch/x86/kernel/cpu/mce/core.c | 20 +++-- arch/x86/kernel/hw_breakpoint.c | 114 ++++++++++++++++----------- arch/x86/kernel/nmi.c | 10 ++- arch/x86/kernel/traps.c | 10 ++- include/linux/hw_breakpoint.h | 6 ++ kernel/events/hw_breakpoint.c | 30 +++++++ 10 files changed, 166 insertions(+), 71 deletions(-)base-commit:
1a416ae446afa42d2d8500ce25bd61c564508721Signature · 1 line
2.53.0
Signature · 1 line
Masami Hiramatsu (Google) <[email protected]>
Masami Hiramatsu (Google)
maintainerPATCH v10 8/11 tracing: wprobe: Add wprobe event trigger
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Add wprobe event trigger to set and clear the watch event dynamically. This allows us to set an watchpoint on a given local variables and a slab object instead of static objects.
The trigger syntax is below:
- set_wprobe:WPROBE:FIELD[+OFFSET] [if FILTER]
- clear_wprobe:WPROBE[:FIELD[+OFFSET]] [if FILTER]
set_wprobe sets the address pointed by FIELD[+offset] to the WPROBE event. The FIELD is the field name of trigger event. clear_wprobe clears the watch address of WPROBE event. If the FIELD option is specified, it clears only if the current watch address is same as the given FIELD[+OFFSET] value.
The set_wprobe trigger does not change the type and length, these must be set when creating a new wprobe.
Also, the WPROBE event must be disabled when setting the new trigger and it will be busy afterwards. Recommended usage is to add a new wprobe at NULL address and keep disabled.
---
Changes in v9:
- Make event_trigger_free() non-static to solve build dependency.
- Sync irq_work and work inside __unregister_trace_wprobe() before
clearing tw->bp_event to prevent concurrent NULL pointer dereference.
- Introduce private_free destructor in struct event_trigger_data to
safely release wprobe_trigger_data after tracepoint readers exit.
- Fix filter memory leak in wprobe_trigger_cmd_parse() on the error
handling path of trace_event_try_get_ref() failure.
- Avoid overwriting tw->addr on clear_wprobe trigger registration to
prevent active watchpoint corruption and hardware breakpoint leak.
Changes in v8:
- Redesign wprobe_trigger() to be safe in NMI/hardirq contexts by
deferring register updates to a workqueue via irq_work.
- Skip trigger execution and increment an atomic missed count
(tw->missed) if the tracepoint runs in NMI context to prevent
recursive spinlock deadlocks. (this is currently hidden counter)
- Prohibit attaching wprobe triggers to kprobe_events by checking
TRACE_EVENT_FL_KPROBE in wprobe_trigger_cmd_parse().
- Use call_rcu() in trigger deactivation path and add
synchronize_rcu() in parse failure path to ensure safe RCU
lifetime cleanup.
- Acquire the target tracepoint's module reference via
trace_event_try_get_ref() to prevent module refcount underflows.
- Fix event_trigger_data memory leak by properly freeing the
initial refcount in wprobe_trigger_cmd_parse() on success.
- Call on_each_cpu() with wait=true in wprobe_work_func() to
prevent use-after-free during trigger unregistration.
- Synchronize concurrent wprobe triggers on the same event by
using a shared raw spinlock (tw->lock).
- Drop the support of kprobe events (that should be done later).
Changes in v7:
- Use kzalloc_obj().
- Update sample code in document.
Changes in v6:
- Update according to the latest change of trigger ops.
Changes in v5:
- Following the suggestions, the documentation was revised to suit rst.
Changes in v3:
- Add FIELD option support for clear_wprobe and update document.
- Fix to unregister/free event_trigger_data on file correctly.
- Fix syntax comments.
Changes in v2:
- Getting local cpu perf_event from trace_wprobe directly.
- Remove trace_wprobe_local_perf() because it is conditionally unused.
- Make CONFIG_WPROBE_TRIGGERS a hidden config.
---
Documentation/trace/wprobetrace.rst | 93 +++++++
include/linux/trace_events.h | 1
kernel/trace/Kconfig | 10 +
kernel/trace/trace.h | 2
kernel/trace/trace_events_trigger.c | 12 +
kernel/trace/trace_wprobe.c | 438 +++++++++++++++++++++++++++++++++++
6 files changed, 553 insertions(+), 3 deletions(-)Documentation/trace/wprobetrace.rst +93 -0
@@ -68,3 +68,96 @@ Here is an example to add a wprobe event on a variable `jiffies`. <idle>-0 [000] d.Z1. 717.026373: my_jiffies: (tick_do_update_jiffies64+0xbe/0x130) You can see the code which writes to `jiffies` is `tick_do_update_jiffies64()`. + +Combination with trigger action +------------------------------- +The event trigger action can extend the utilization of this wprobe. + +- set_wprobe:WPEVENT:FIELD[+|-ADJUST] +- clear_wprobe:WPEVENT[:FIELD[+|-]ADJUST] + +Set these triggers to the target event, then the WPROBE event will be +setup to trace the memory access at FIELD[+|-ADJUST] address. +When clear_wprobe is hit, if FIELD is NOT specified, the WPEVENT is +forcibly cleared. If FIELD[[+|-]ADJUST] is set, it clears WPEVENT only +if its watching address is the same as the FIELD[[+|-]ADJUST] value. + +Notes: +The set_wprobe trigger does not change the type and length, these +must be set when creating a new wprobe. + +The WPROBE event must be disabled when setting the new trigger +and it will be busy afterwards. Recommended usage is to add a new +wprobe at NULL address and keep disabled. + +Wprobe triggers are not supported on kprobe_events, because kprobes +themselves can use software breakpoints which conflicts with wprobe +operation. + + +For example, trace the first 8 bytes of the dentry data structure passed +to do_truncate() until it is deleted by dentry_kill(). +(Note: all tracefs setup uses '>>' so that it does not kick do_truncate()) +:: + + # echo 'w:watch rw@0:8 address=$addr value=+0($addr)' >> dynamic_events + # echo 'f:truncate do_truncate dentry=$arg2' >> dynamic_events + # echo 'set_wprobe:watch:dentry' >> events/fprobes/truncate/trigger + # echo 'f:dentry_kill dentry_kill dentry=$arg1' >> dynamic_events + # echo 'clear_wprobe:watch:dentry' >> events/fprobes/dentry_kill/trigger + # echo 1 >> events/fprobes/truncate/enable + # echo 1 >> events/fprobes/dentry_kill/enable + + # echo aaa > /tmp/hoge + # echo bbb > /tmp/hoge + # echo ccc > /tmp/hoge + # rm /tmp/hoge + +Then, the trace data will show:: + + # tracer: nop + # + # entries-in-buffer/entries-written: 32/32 #P:8 + # + # _-----=> irqs-off/BH-disabled + # / _----=> need-resched + # | / _---=> hardirq/softirq + # || / _--=> preempt-depth + # ||| / _-=> migrate-disable + # |||| / delay + # TASK-PID CPU# ||||| TIMESTAMP FUNCTION + # | | | ||||| | | + sh-107 [004] ...1. 9.990418: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004ad6618 + sh-107 [004] ...1. 9.990914: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b3de78 + sh-107 [004] ...1. 9.993175: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049ddd40 + sh-107 [004] ..... 9.995198: truncate: (do_truncate+0x4/0x120) dentry=0xffff8880048083a8 + sh-107 [004] ...1. 9.995389: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db998 + sh-107 [004] ..Zff 9.997503: watch: (lookup_fast+0xaa/0x150) address=0xffff8880048083a8 value=0x8200080 + sh-107 [004] ..Zff 9.997509: watch: (path_openat+0x211/0xda0) address=0xffff8880048083a8 value=0x8200080 + sh-107 [004] ..Zff 9.997514: watch: (path_openat+0xa56/0xda0) address=0xffff8880048083a8 value=0x8200080 + sh-107 [004] ..Zff 9.997518: watch: (path_openat+0xae2/0xda0) address=0xffff8880048083a8 value=0x8200080 + sh-107 [004] ..... 9.997521: truncate: (do_truncate+0x4/0x120) dentry=0xffff8880048083a8 + sh-107 [004] ...1. 9.997582: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004808270 + sh-107 [004] ...1. 9.999365: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db728 + sh-107 [004] ...1. 9.999388: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b1c000 + rm-113 [005] ..Zff 10.000965: watch: (lookup_fast+0xaa/0x150) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.000971: watch: (path_lookupat+0x97/0x1e0) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.000984: watch: (lookup_fast+0xaa/0x150) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.000988: watch: (path_lookupat+0x97/0x1e0) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.001010: watch: (lookup_one_qstr_excl+0x28/0x140) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.001014: watch: (lookup_one_qstr_excl+0xd1/0x140) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.001018: watch: (may_delete_dentry+0x1c/0x200) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.001021: watch: (may_delete_dentry+0x195/0x200) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] ..Zff 10.001031: watch: (vfs_unlink+0x5e/0x260) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] d.Z.. 10.001067: watch: (d_make_discardable+0x1b/0x40) address=0xffff8880048083a8 value=0x8200080 + rm-113 [005] d.Z.. 10.001071: watch: (d_make_discardable+0x29/0x40) address=0xffff8880048083a8 value=0x200080 + rm-113 [005] ...1. 10.001072: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880048083a8 + rm-113 [005] ...1. 10.001218: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880048083a8 + sh-107 [004] ...1. 10.001416: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db110 + sh-107 [004] ...1. 10.001444: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff8880049db248 + sh-107 [004] ...1. 10.001500: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004ad6618 + sh-107 [004] ...1. 10.002067: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b41e78 + sh-107 [004] ...1. 10.904920: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004b41e78 + sh-107 [004] ...1. 10.905129: dentry_kill: (dentry_kill+0x0/0x2c0) dentry=0xffff888004ad6618 + +You can see the watch event is correctly configured on the dentry.
include/linux/trace_events.h +1 -0
@@ -729,6 +729,7 @@ enum event_trigger_type {ETT_EVENT_HIST = (1 <<4 ),ETT_HIST_ENABLE = (1 <<5 ),ETT_EVENT_EPROBE = (1 <<6 ), + ETT_EVENT_WPROBE = (1 << 7), };extern int filter_match_preds (struct event_filter *filter,void *rec);
kernel/trace/Kconfig +10 -0
@@ -876,6 +876,16 @@ config WPROBE_EVENTS Those events can be inserted wherever hardware breakpoints can be set, and record accessed memory address and values. +config WPROBE_TRIGGERS + depends on WPROBE_EVENTS + depends on HAVE_REINSTALL_HW_BREAKPOINT + bool + default y + help + This adds an event trigger which will set the wprobe on a specific + field of an event. This allows user to trace the memory access of + an address pointed by the event field. + config BPF_EVENTS depends on BPF_SYSCALL depends on (KPROBE_EVENTS || UPROBE_EVENTS) && PERF_EVENTS
kernel/trace/trace.h +2 -0
@@ -1939,6 +1939,7 @@ struct event_trigger_data {struct event_filter __rcu *filter;char *filter_str;void *private_data; +void (*private_free)(void *private_data);bool paused;bool paused_tmp;struct list_head list;
@@ -1982,6 +1983,7 @@ trigger_data_alloc(struct event_command *cmd_ops, char *cmd, char *param, void *private_data);extern void trigger_data_free (struct event_trigger_data *data);extern int event_trigger_init (struct event_trigger_data *data); +extern void event_trigger_free (struct event_trigger_data *data);extern int trace_event_trigger_enable_disable (struct trace_event_file *file,int trigger_enable);extern void update_cond_flag (struct trace_event_file *file);
kernel/trace/trace_events_trigger.c +9 -3
@@ -51,8 +51,11 @@ static void trigger_data_free_queued_locked(void)tracepoint_synchronize_unregister (); -llist_for_each_entry_safe (data, tmp, llnodes, llist) +llist_for_each_entry_safe (data, tmp, llnodes, llist) { +if (data->private_free) + data->private_free (data->private_data);kfree (data); + } }/* Bulk garbage collection of event_trigger_data elements */
@@ -74,8 +77,11 @@ static int trigger_kthread_fn(void *ignore)/* make sure current triggers exit before free */ tracepoint_synchronize_unregister (); -llist_for_each_entry_safe (data, tmp, llnodes, llist) +llist_for_each_entry_safe (data, tmp, llnodes, llist) { +if (data->private_free) + data->private_free (data->private_data);kfree (data); + } }return 0 ;
@@ -582,7 +588,7 @@ int event_trigger_init(struct event_trigger_data *data)* Usually used directly as the @free method in event trigger * implementations. */ -static void +void event_trigger_free (struct event_trigger_data *data) {if (WARN_ON_ONCE (data->ref <=0 ))
kernel/trace/trace_wprobe.c +438 -0
485 lines not on this page · raw message
[email protected]
AI reviewRe: [PATCH v10 08/11] tracing: wprobe: Add wprobe event trigger
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Premature teardown of wprobe resources during trigger unregistration leads to a Use-After-Free and…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Premature teardown of wprobe resources during trigger unregistration leads to a Use-After-Free and NULL pointer dereference in the deferred workqueue because RCU synchronization is missing.
- [High] Incorrect RCU synchronization primitive in wprobe_trigger_cmd_parse() error path leads to a Use-After-Free on faultable (syscall) tracepoints.
--
--- Patch [8]: [PATCH v10 08/11] tracing: wprobe: Add wprobe event trigger --- commit 7a3a7529186a14015a2ccb9a93108962a135a419
Author: Masami Hiramatsu (Google) <[email protected]>
tracing: wprobe: Add wprobe event trigger
Add wprobe event trigger to set and clear the watch event dynamically. This allows us to set an watchpoint on a given local variables and a slab object instead of static objects.
Quoted 4 lines
kernel/trace/trace_wprobe.c +0
-0
[ ... ]
Quoted 30 lines
kernel/trace/trace_wprobe.c +26
-0@@ -794,3 +794,391 @@ static __init int init_wprobe_trace(void) }fs_initcall (init_wprobe_trace); +#ifdef CONFIG_WPROBE_TRIGGERS + +static int wprobe_trigger_global_enabled; + +#define SET_WPROBE_STR "set_wprobe" +#define CLEAR_WPROBE_STR "clear_wprobe" +#define WPROBE_DEFAULT_CLEAR_ADDRESS ((unsigned long)&wprobe_trigger_global_enabled) + +struct wprobe_trigger_data { +struct rcu_head rcu; +struct trace_event_file *file; +struct trace_wprobe *tw; +unsigned int offset; +long adjust; +const char *field; +bool clear; +}; + +static void wprobe_trigger (struct event_trigger_data *data, +struct trace_buffer *buffer,void *rec, +struct ring_buffer_event *event) +{ +struct wprobe_trigger_data *wprobe_data = data->private_data; +struct trace_wprobe *tw = wprobe_data->tw; +unsigned long addr, flags; +bool changed = false;
[ ... ]
Quoted 5 lines
kernel/trace/trace_wprobe.c +5
-0+if (changed) +irq_work_queue (&tw->irq_work); + +raw_spin_unlock_irqrestore (&tw->lock, flags); +}
[ ... ]
kernel/trace/trace_wprobe.c static __init int init_wprobe_trace(void) · line 922
Quoted 11 lines
kernel/trace/trace_wprobe.c +11
-0+static void wprobe_trigger_free (struct event_trigger_data *data) +{ +struct wprobe_trigger_data *wprobe_data = data->private_data; + +if (WARN_ON_ONCE (data->ref <=0 )) +return ; + + data->ref--; +if (!data->ref) { +/* Remove the SOFT_MODE flag */ +trace_event_enable_disable (wprobe_data->file,0 ,1 );
[Severity: High]
Does this disable the wprobe event synchronously before waiting for RCU readers?
If an in-flight RCU reader is executing wprobe_trigger() when this is called, trace_event_enable_disable() will trigger __unregister_trace_wprobe() which nullifies tw->bp_event and allows tw to be concurrently freed.
The in-flight reader could then queue tw->irq_work in wprobe_trigger():
if (changed) irq_work_queue(&tw->irq_work);
This work then executes trace_wprobe_update_local() which dereferences the nullified tw->bp_event:
struct perf_event *bp = *this_cpu_ptr(tw->bp_event);
Can this lead to a NULL pointer dereference or use-after-free since the cleanup doesn't wait for the RCU readers to finish?
Quoted 10 lines
kernel/trace/trace_wprobe.c +10
-0+trace_event_put_ref (wprobe_data->file->event_call); +trigger_data_free (data); + } +} + +static int wprobe_trigger_cmd_parse (struct event_command *cmd_ops, +struct trace_event_file *file, +char *glob,char *cmd, +char *param_and_filter) +{
[ ... ]
kernel/trace/trace_wprobe.c static __init int init_wprobe_trace(void) · line 1055
Quoted 6 lines
kernel/trace/trace_wprobe.c +6
-0+ ret =event_trigger_register (cmd_ops, file, glob, trigger_data); +if (ret) { +event_trigger_reset_filter (cmd_ops, trigger_data); +trace_event_enable_disable (wprobe_file,0 ,1 ); +trace_event_put_ref (wprobe_file->event_call); +synchronize_rcu ();
[Severity: High]
Is synchronize_rcu() sufficient to wait for faultable tracepoint readers?
Faultable tracepoints (like syscall tracepoints) use RCU Tasks Trace for protection, which synchronize_rcu() does not wait for. If this error path is hit while attaching to a syscall tracepoint, the in-flight readers could still be executing when the memory is freed via the __free() cleanup attributes at the end of the function scope.
Should this use tracepoint_synchronize_unregister() instead to ensure it waits for all tracepoint reader types?
Quoted 9 lines
kernel/trace/trace_wprobe.c +9
-0+return ret; + } +/* Make it NULL to avoid freeing trigger_data and wprobe_data by __free() */ + wprobe_data =NULL ; +event_trigger_free (trigger_data); + trigger_data =NULL ; + +return 0 ; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=8
Masami Hiramatsu (Google)
maintainerPATCH v10 9/11 selftests: ftrace: Add wprobe trigger testcase
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Add a testcase for checking wprobe trigger. This sets set_wprobe and clear_wprobe triggers on fprobe events to watch dentry access. So this depends on both wprobe and fprobe.
---
Changes in v9:
- Drop dentry cache after removing tempfile for on-disk filesystem.
Changes in v8:
- Use TMPDIR for making a test file.
- Ensure the fprobe and target functions exists, if not, exit
with UNRESOLVED (== skip). Changes in v7: - Add a newline at the end of file.(style fix) - Use dentry_kill instead of __dentry_kill. Changes in v5: - Enable CONFIG_WPROBE_TRIGGERS in the config for ftrace test. Changes in v3: - Newly added. --- tools/testing/selftests/ftrace/config | 1 .../ftrace/test.d/trigger/trigger-wprobe.tc | 70 ++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc
tools/testing/selftests/ftrace/config +1 -0
@@ -28,3 +28,4 @@ CONFIG_TRACER_SNAPSHOT=y CONFIG_UPROBES=y CONFIG_UPROBE_EVENTS=y CONFIG_WPROBE_EVENTS=y +CONFIG_WPROBE_TRIGGERS=y
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc new file +70 -0
70 lines not on this page · raw message
[email protected]
AI reviewRe: [PATCH v10 09/11] selftests: ftrace: Add wprobe trigger testcase
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] Test teardown verification is bypassed due to shell quoting and explicit `exit 0`, hiding…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Test teardown verification is bypassed due to shell quoting and explicit exit 0, hiding teardown failures.
- [Medium] Hard-enabling dentry_kill before dropping caches floods the trace ring buffer, causing test flakiness.
- [Medium] Global tracepoints lacking PID filters allow background processes to hijack the wprobe watchpoint, causing test flakiness. --
--- Patch [9]: [PATCH v10 09/11] selftests: ftrace: Add wprobe trigger testcase --- commit fe30508ad92d4c48b6107f15074a5539f6b55528
Author: Masami Hiramatsu (Google) <[email protected]>
selftests: ftrace: Add wprobe trigger testcase
Add a testcase for checking wprobe trigger. This sets set_wprobe and clear_wprobe triggers on fprobe events to watch dentry access.
Quoted 4 lines
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc new file +0
-0
[ ... ]
Quoted 2 lines
+:;: "Add wprobe triggers" ;:
+echo 'set_wprobe:watch:dentry' >> events/fprobes/truncate/trigger
[Severity: Medium]
Since this trigger is attached globally without a PID filter, could a background process hitting do_truncate latch the wprobe onto the wrong dentry?
If another process truncates a file before the test modifies its temporary file, it seems the test might fail to detect its own file accesses, causing flakiness.
Quoted 12 lines
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc new file +12
-0+echo 'clear_wprobe:watch:dentry' >> events/fprobes/dentry_kill/trigger +cat events/fprobes/truncate/trigger | grep ^set_wprobe +cat events/fprobes/dentry_kill/trigger | grep ^clear_wprobe + +:;: "Ensure wprobe is still disabled" ;: +cat events/wprobes/watch/enable | grep 0 + +:;: "Enable events for triggers" ;: +echo 1 >> events/fprobes/truncate/enable +echo 1 >> events/fprobes/dentry_kill/enable + +:;: "Start test workload" ;:
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc line 55
Quoted 10 lines
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc new file +10
-0+echo 1 >> tracing_on + +echo aaa > $TMPDIR/hoge +echo bbb > $TMPDIR/hoge +echo ccc > $TMPDIR/hoge +rm $TMPDIR/hoge + +:;: "Drop dentry caches (for dentry_kill)" ;: +# Use append (>>) to avoid calling do_truncate again. +sync && echo 2 >> /proc/sys/vm/drop_caches
[Severity: Medium]
Will hard-enabling the dentry_kill fprobe earlier cause this drop_caches command to flood the trace ring buffer?
A system-wide dentry eviction generates a massive number of dentry_kill events. This could easily wrap the default ftrace ring buffer, potentially overwriting the watch events needed for the final assertion.
Quoted 5 lines
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc new file +5
-0+ +:;: "Check trace results" ;: +cat trace | grep watch + +
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc line 70
Quoted 10 lines
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc new file +10
-0+:;: "Ensure wprobe becomes disabled again" ;: +cat events/wprobes/watch/enable | grep 0 + +:;: "Remove wprobe triggers" ;: +echo '!set_wprobe:watch:dentry' >> events/fprobes/truncate/trigger +echo '!clear_wprobe:watch' >> events/fprobes/dentry_kill/trigger +! grep ^set_wprobe events/fprobes/truncate/trigger +! grep ^clear_wprobe events/fprobes/dentry_kill/trigger + +exit 0
[Severity: Medium]
Does this sequence properly fail the test if the triggers cannot be removed?
Because the script ends with exit 0, and the grep pipeline ignores the shell errexit option, any failures in verifying the trigger removal might be swallowed, causing the test to pass even if teardown fails.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=9
Masami Hiramatsu (Google)
maintainerPATCH v10 10/11 tracing/wprobe: Support BTF typecast in fetchargs
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Allow BTF typecast syntax (STRUCT)FETCHARG->MEMBER in wprobe event fetchargs. Previously, handle_typecast() rejected any probe context that was not a function entry/return or tracepoint event probe.
Wprobe events use (the accessed address) and (the value at that address). By enabling BTF typecast, users can now cast these to a concrete struct type and access its fields directly. For example:
echo 'w:watch rw@0:8 dflag=(struct dentry)$addr->d_flags' >> dynamic_events
With a set_wprobe trigger pointing the watchpoint at a dentry address, the resulting trace shows d_flags being accessed at that location.
Note that and are restricted to kernel-space memory, which is consistent with the existing TPARG_FL_KERNEL flag used when parsing wprobe fetchargs.
---
Changes in v9:
- Newly added.
--- kernel/trace/trace_probe.c | 3 + kernel/trace/trace_probe.h | 5 + .../test.d/trigger/trigger-wprobe-btf-typecast.tc | 80 ++++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-btf-typecast.tc
kernel/trace/trace_probe.c +2 -1
@@ -967,7 +967,8 @@ static int handle_typecast(char *arg, struct traceprobe_parse_context *ctx)if (!(tparg_is_event_probe (ctx->flags) ||tparg_is_function_entry (ctx->flags) || -tparg_is_function_return (ctx->flags))) { +tparg_is_function_return (ctx->flags) || +tparg_is_wprobe (ctx->flags))) {trace_probe_log_err (ctx->offset,NOSUP_BTFARG );return -EOPNOTSUPP ; }
kernel/trace/trace_probe.h +5 -0
@@ -439,6 +439,11 @@ static inline bool tparg_is_event_probe(unsigned int flags)return !!(flags &TPARG_FL_TEVENT ); } +static inline bool tparg_is_wprobe (unsigned int flags) +{ +return !!(flags &TPARG_FL_WPROBE ); +} +/* Each typecast consumes nested level. So the max number of typecast is 8. */ #define TRACEPROBE_MAX_NESTED_LEVEL 8
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-btf-typecast.tc new file +80 -0
80 lines not on this page · raw message
Masami Hiramatsu (Google)
maintainerPATCH v10 11/11 tracing/wprobe: Support BTF typecast in wprobe trigger command
In reply to Masami Hiramatsu (Google)From: Masami Hiramatsu (Google) <[email protected]>
Extend the set_wprobe trigger syntax to support automatic BTF-based offset calculation using the form:
set_wprobe:WPEVENT:(TYPE[,ASGN])FIELD->MEMBER[.SUBMEMBER[...]]
Previously, the ADJUST value in FIELD[+/-ADJUST] had to be a numeric literal, requiring the user to know the exact byte offset of the target struct member.
With this change, if the FIELD portion starts with (STRUCTTYPE), the offset of MEMBER within STRUCTTYPE is automatically resolved via BTF. This allows symbolic, kernel-version-independent watchpoint placement at specific struct fields.
For example, to watch when the d_inode pointer inside a dentry is modified (not just when the dentry itself is accessed):
echo 'w:watch rw@0:8 address=$addr value=$value' >> dynamic_events
echo 'f:truncate do_truncate dentry=$arg2' >> dynamic_events
echo 'set_wprobe:watch:(dentry)dentry->d_inode' \
>> events/fprobes/truncate/triggerHere, "dentry" is a field in the fprobe event record. The BTF lookup resolves offsetof(struct dentry, d_inode) at set_wprobe parse time, so no manual numeric offset is needed.
---
Changes in v10:
- Newly added.
--- kernel/trace/trace_wprobe.c | 166 +++++++++++++++++--- .../test.d/trigger/trigger-wprobe-btf-offset.tc | 73 +++++++++ 2 files changed, 214 insertions(+), 25 deletions(-) create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-btf-offset.tc
kernel/trace/trace_wprobe.c +141 -25
@@ -27,6 +27,7 @@#include <asm/ptrace.h> #include "trace.h" +#include "trace_btf.h" #include "trace_dynevent.h" #include "trace_probe.h" #include "trace_probe_kernel.h"
@@ -925,6 +926,143 @@ static void wprobe_trigger_free(struct event_trigger_data *data) } } +#ifdef CONFIG_PROBE_EVENTS_BTF_ARGS + +static int get_offset_of_field (struct btf *btf,const struct btf_type *type,char *field_name) +{ +const struct btf_member *field; +int bitoffs =0 ; +u32 anon_offs; +char *next; + +do { + next =strchr (field_name,'.' ); +if (next) + *next++ ='\0' ; + + field =btf_find_struct_member (btf, type, field_name, &anon_offs); +if (IS_ERR_OR_NULL (field)) +return -ENOENT ; + type =btf_type_by_id (btf, field->type); + +/* Reject bitfield member access */ +if (btf_type_kflag (type)) +return -EINVAL ; + +/* add offset for anonymous struct type */ + bitoffs += anon_offs + field->offset; + field_name = next; +if (next) { + type =btf_type_skip_modifiers (btf, field->type,NULL ); +if (!type) +return -ENOENT ; + } + }while (next); +return bitoffs /BITS_PER_BYTE ; +} + +/* btf_put(NULL) is acceptable. */ +DEFINE_FREE(btf_put, struct btf *, btf_put(_T)) + +/* parse typecast: (TYPE[,ASGN])EVENT_FIELD->FIELD[.SUBFIELD...] and set adjust. */ +static int wprobe_trigger_typecast_parse (char **field_str_ptr, +struct wprobe_trigger_data *wprobe_data) +{ + struct btf *btf __free(btf_put) = NULL; +const struct btf_type *type; +char *assign_field; +char *event_field; +char *type_field; +char *type_name; +int id; +int adjust; + + type_name = *field_str_ptr +1 ; + event_field =strchr (type_name,')' ); +if (!event_field) +return -EINVAL ; + *event_field++ ='\0' ; + +/* Check the optional assign field. */ + assign_field =strchr (type_name,',' ); +if (assign_field) + *assign_field++ ='\0' ; + +/* Get the type field name. */ + type_field =strstr (event_field,"->" ); +if (!type_field) +return -EINVAL ; + *type_field ='\0' ; + type_field +=2 ; + +/* find type from BTF */ + id =bpf_find_btf_id (type_name,BTF_KIND_STRUCT , &btf); +if (id <0 ) +return id; + + type =btf_type_by_id (btf, id); +if (!type) +return -EINVAL ; + + adjust =get_offset_of_field (btf, type, type_field); +if (adjust <0 ) +return adjust; + wprobe_data->adjust = adjust; + +if (assign_field) { +/* assign_field should be a struct field */ + adjust =get_offset_of_field (btf, type, assign_field); +if (adjust <0 ) +return adjust; + wprobe_data->adjust -= adjust; + } + + *field_str_ptr = event_field; +return 0 ; +} +#else +static int wprobe_trigger_typecast_parse (char **field_str_ptr,struct wprobe_trigger_data *wprobe_data) +{ +return -EOPNOTSUPP ; +} +#endif /* CONFIG_PROBE_EVENTS_BTF_ARGS */ + +static int wprobe_trigger_field_parse (char *field_str,struct trace_event_file *file, +struct wprobe_trigger_data *wprobe_data) +{ +struct ftrace_event_field *field; +char *offs; + +if (field_str[0 ] =='(' ) { +int ret =wprobe_trigger_typecast_parse (&field_str, wprobe_data); + +if (ret <0 ) +return ret; + }else { + offs =strpbrk (field_str,"+-" ); +if (offs) { +long val; + +if (kstrtol (offs,0 , &val) <0 ) +return -EINVAL ; + wprobe_data->adjust = val; + *offs ='\0' ; + } + } + + field =trace_find_event_field (file->event_call, field_str); +if (!field) +return -ENOENT ; +if (field->size !=sizeof (void *)) +return -ENOEXEC ; + wprobe_data->offset = field->offset; + wprobe_data->field =kstrdup (field_str,GFP_KERNEL ); +if (!wprobe_data->field) +return -ENOMEM ; + +return 0 ; +} + static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops, struct trace_event_file *file, char *glob, char *cmd,
@@ -936,10 +1074,8 @@ static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops,*/ struct wprobe_trigger_data *wprobe_data __free(free_wprobe_trigger_data) = NULL; struct event_trigger_data *trigger_data __free(kfree) = NULL; -struct ftrace_event_field *field =NULL ;struct trace_event_file *wprobe_file;struct trace_array *tr = file->tr; -struct trace_event_call *event;char *event_str, *field_str;bool remove, clear = false;struct trace_wprobe *tw;
@@ -986,29 +1122,9 @@ static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops,return -EINVAL ;if (field_str) { -char *offs; - - offs =strpbrk (field_str,"+-" ); -if (offs) { -long val; - -if (kstrtol (offs,0 , &val) <0 ) -return -EINVAL ; - wprobe_data->adjust = val; - *offs ='\0' ; - } - - event = file->event_call; - field =trace_find_event_field (event, field_str); -if (!field) -return -ENOENT ; - -if (field->size !=sizeof (void *)) -return -ENOEXEC ; - wprobe_data->offset = field->offset; - wprobe_data->field =kstrdup (field_str,GFP_KERNEL ); -if (!wprobe_data->field) -return -ENOMEM ; + ret =wprobe_trigger_field_parse (field_str, file, wprobe_data); +if (ret <0 ) +return ret; } trigger_data =trigger_data_alloc (cmd_ops, cmd, param, wprobe_data);
tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-btf-offset.tc new file +73 -0
73 lines not on this page · raw message
[email protected]
AI reviewRe: [PATCH v10 11/11] tracing/wprobe: Support BTF typecast in wprobe trigger command
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incorrect BTF kflag check in get_offset_of_field() falsely rejects nested structs and computes…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect BTF kflag check in get_offset_of_field() falsely rejects nested structs and computes massive out-of-bounds offsets for bitfield members. --
--- Patch [11]: [PATCH v10 11/11] tracing/wprobe: Support BTF typecast in wprobe trigger command --- commit 61f26bb67fea52d49bbb314e0a53092298348830
Author: Masami Hiramatsu (Google) <[email protected]>
tracing/wprobe: Support BTF typecast in wprobe trigger command
This commit extends the set_wprobe trigger syntax to support automatic BTF-based offset calculation. It allows symbolic watchpoint placement at specific struct fields by resolving the offset of a member within a struct type using BTF, avoiding the need for manual numeric offsets.
Quoted 4 lines
kernel/trace/trace_wprobe.c +0
-0
[ ... ]
Quoted 19 lines
kernel/trace/trace_wprobe.c +15
-0@@ -925,6 +926,143 @@ static void wprobe_trigger_free(struct event_trigger_data *data) } } +#ifdef CONFIG_PROBE_EVENTS_BTF_ARGS + +static int get_offset_of_field (struct btf *btf,const struct btf_type *type,char *field_name) +{ +const struct btf_member *field; +int bitoffs =0 ; +u32 anon_offs; +char *next; + + do { + next = strchr(field_name, '.'); + if (next) + *next++ = '\0'; + + field = btf_find_struct_member(btf, type, field_name, &anon_offs);
kernel/trace/trace_wprobe.c static void wprobe_trigger_free(struct event_trigger_data *data) · line 953
Quoted 10 lines
kernel/trace/trace_wprobe.c +10
-0+if (IS_ERR_OR_NULL (field)) +return -ENOENT ; + type =btf_type_by_id (btf, field->type); + +/* Reject bitfield member access */ +if (btf_type_kflag (type)) +return -EINVAL ; + +/* add offset for anonymous struct type */ + bitoffs += anon_offs + field->offset;
[Severity: High]
Does this code in get_offset_of_field() incorrectly check the child member's type instead of the parent struct?
Because the type variable is updated to the child member's type before evaluating btf_type_kflag(type), it seems this check will fail to reject bitfields (since a child type like BTF_KIND_INT doesn't have a kflag).
When a bitfield is not rejected, field->offset (which contains the bitfield size shifted by 24 bits for structs with kflag=1) is directly added to bitoffs, producing an out-of-bounds byte offset for the watchpoint.
Additionally, could this falsely reject valid accesses if the target member is a nested struct that does have its kflag set?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=11