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) <[email protected]>

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/

> 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
[ ... ]
> +:;: "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?

> +
> +:;: "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
[ ... ]
> +:;: "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.

> +
> +:;: "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