Re: [PATCH v12 09/11] selftests: ftrace: Add wprobe trigger testcase

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Race condition in `trigger-wprobe.tc` causes spurious test failures because `set_wprobe` trigger is not removed before verifying `clear_wprobe`.
- [Medium] `trigger-wprobe-syntax-errors.tc` relies on `do_sys_open` without verifying its existence, causing the test to fail on kernels where the function is unavailable.
--

--- Patch [9]: [PATCH v12 09/11] selftests: ftrace: Add wprobe trigger testcase ---
commit d811031bfa18879b95fa2051a73496d3fcc8f562
Author: Masami Hiramatsu (Google) <[email protected]>

selftests: ftrace: Add wprobe trigger testcase

This patch adds a testcase for checking the wprobe trigger. It sets
set_wprobe and clear_wprobe triggers on fprobe events and static trace events
to monitor memory accesses. It also includes a testcase to verify 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-syntax-errors.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-syntax-errors.tc
> new file mode 100644
> index 0000000000000..7e02313cf1c73
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe-syntax-errors.tc
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: event trigger - test wprobe trigger syntax errors
> +# requires: dynamic_events error_log "w[:[<group>/][<event>]] [r|w|rw]@<addr>[:<len>]":README "f[:[<group>/][<event>]] <func-name>[%return] [<args>]":README
> +
> +check_error() { # command-with-error-pos-by-^
> +    ftrace_errlog_check "wprobe_trigger" "$1" "events/fprobes/testevent/trigger"
> +}
> +
> +# Add a dummy fprobe event to attach triggers to
> +echo 'f:fprobes/testevent do_sys_open dfd=$arg1' > dynamic_events

[Severity: Medium]
Will this script fail unconditionally on kernels where do_sys_open is
unavailable as a valid fprobe target?

On many modern kernels, do_sys_open has been removed, renamed (e.g., to
do_sys_openat2), or heavily inlined, which causes this echo command to
dynamic_events to fail with an error.

Could we check against /proc/kallsyms before using it, similar to how
sample_timer_cb is checked in trigger-wprobe.tc?

[ ... ]
> 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..4071fe0c878b9
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc
[ ... ]
> +:;: "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
> +
> +# Clear trace and wait to ensure no new watchpoint events are generated
> +clear_trace
> +sleep 1
> +
> +:;: "Ensure clear_wprobe trigger deactivated the watchpoint" ;:
> +! grep -q watch trace

[Severity: Medium]
Is there a race condition here that could cause spurious test failures?

Since the set_wprobe trigger on testevent (sample_timer_cb) is still active
and fires concurrently with foo_bar_with_fn, if sample_timer_cb fires after
foo_bar_with_fn during this 1-second window, the wprobe could be re-armed.

Memory accesses would then generate new watch events, causing this assertion
to fail.

Should the set_wprobe trigger be removed before verifying the clear_wprobe
behavior?

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