Re: [PATCH v13 10/12] selftests: ftrace: Add wprobe trigger testcase

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `trigger-wprobe.tc` test has a race condition causing intermittent failures because the `clear_wprobe` trigger executes asynchronously and the test does not wait for it to fire before verifying the trace buffer.
- [Medium] The `trigger-wprobe.tc` test uses `timer=t` as a fetcharg without enforcing a BTF requirement, which will cause false-positive test failures on kernels without BTF support.
--

--- Patch [10]: [PATCH v13 10/12] selftests: ftrace: Add wprobe trigger testcase ---

commit f8200da0c99be51814ee14df413b8fd81995a06f
Author: Masami Hiramatsu (Google) [off-list ref]

selftests: ftrace: Add wprobe trigger testcase

This commit adds a testcase to verify wprobe triggers. It uses set_wprobe
and clear_wprobe triggers on fprobe and static trace events to monitor
memory accesses within the trace-events-sample kernel module, and tests
wprobe trigger syntax error logging.

Link: https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao600@gmail.com/ (local)

quoted

diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc
new file mode 100644
index 0000000000000..0565e2c421778
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc

[ ... ]

quoted

+:;: "Add a wprobe event used by trigger" ;:
+echo 'w:watch rw@0:8 address=$addr value=$value' > dynamic_events
+
+:;: "Add events for triggering wprobe" ;:
+echo 'f:fprobes/testevent sample_timer_cb timer=t' >> dynamic_events

[Severity: Medium]
Is it intentional to use a local variable t as a fetcharg here?

This relies on BTF debug info. On kernels without 
CONFIG_PROBE_EVENTS_BTF_ARGS=y or architectures lacking BTF support, 
the kernel's tracing subsystem cannot resolve the argument and the 
fprobe registration will fail.

Should this test either add a requirement for BTF arguments in the requires
directive, or use a register argument like $arg1 instead?

quoted

+
+:;: "Enable all events before setting triggers" ;:
+echo 1 > tracing_on
+echo 1 >> events/fprobes/testevent/enable
+echo 1 >> events/sample-trace/foo_bar_with_fn/enable

[ ... ]

quoted

+:;: "Set clear_wprobe trigger on foo_bar_with_fn" ;:
+echo 'clear_wprobe:watch' >> events/sample-trace/foo_bar_with_fn/trigger
+cat events/sample-trace/foo_bar_with_fn/trigger | grep ^clear_wprobe
+
+# Disable set_wprobe to prevent sample_timer_cb from re-arming the watchpoint
+echo '!set_wprobe:watch:timer' >> events/fprobes/testevent/trigger
+
+# Clear trace and wait to ensure no new watchpoint events are generated
+clear_trace
+sleep 1

[Severity: Medium]
Can this sequence cause a race condition resulting in intermittent test 
failures?

If the test clears the trace buffer here without first waiting for
foo_bar_with_fn to execute and actually clear the watchpoint, there is a race
window.

Since both foo_bar_with_fn and sample_timer_cb run periodically every 
second, could sample_timer_cb execute after clear_trace but before
foo_bar_with_fn has fired?

If so, it would access the still-armed watchpoint and log a watch event in the
newly cleared trace buffer, causing the final verification step 
(! grep -q watch trace) to fail.

quoted

+
+:;: "Ensure clear_wprobe trigger deactivated the watchpoint" ;:
+! grep -q watch trace
+

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