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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Permanent leak of trigger_data and wprobe_data when unregistering a wprobe trigger.
- [Medium] The selftest fails to declare its dependency on fprobe in the `# requires` directive, causing test failures on unsupported kernels.
- [Low] The test uses a hardcoded path `/tmp/hoge` for triggering file operations, leading to test brittleness.
- [Low] The test hooks a static function `dentry_kill` without checking its presence in `available_filter_functions`.
--

--- Patch [10]: [PATCH v7 10/10] selftests: ftrace: Add wprobe trigger testcase ---
commit 3a609968e71e94ce3399d95a2f00d2d3b0974ab0
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

selftests: ftrace: Add wprobe trigger testcase

This commit adds a testcase to check the wprobe trigger functionality.
It sets set_wprobe and clear_wprobe triggers on fprobe events to watch
dentry access, validating features that depend on both wprobe and fprobe.

[Severity: High]
While not part of this commit's diff, there appears to be a regression
associated with the wprobe trigger registration tested here.

In kernel/trace/trace_wprobe.c:wprobe_trigger_cmd_parse():

	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);
		return ret;
	}
	/* Make it NULL to avoid freeing trigger_data and wprobe_data by __free() */
	trigger_data = NULL;
	wprobe_data = NULL;

	return 0;

Does this code permanently leak the trigger_data and wprobe_data structures
when unregistering a wprobe trigger?

The successful path returns without calling event_trigger_free(trigger_data),
stranding the reference count at 2.

> 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..cc7a0532d7ff8
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-wprobe.tc
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: event trigger - test wprobe trigger
> +# requires: dynamic_events "w[:[<group>/][<event>]] [r|w|rw]@<addr>[:<len>]":README events/sched/sched_process_fork/trigger

[Severity: Medium]
Will this script fail with an error exit code on environments without
fprobe support, rather than gracefully skipping the test?

The test relies on fprobe (f:), but does not declare this dependency in the
# requires directive.

Without this requirement, the ftracetest runner will try to write f:truncate
to dynamic_events on kernels configured with CONFIG_WPROBE_EVENTS=y but
without CONFIG_FPROBE_EVENTS, causing the test to fail.

> +
> +echo 0 > tracing_on
> +
> +:;: "Add a wprobe event used by trigger" ;:
> +echo 'w:watch rw@0:8 address=$addr value=+0($addr)' > dynamic_events
> +
> +:;: "Add events for triggering wprobe" ;:
> +echo 'f:truncate do_truncate dentry=$arg2' >> dynamic_events
> +echo 'f:dentry_kill dentry_kill dentry=$arg1' >> dynamic_events

[Severity: Low]
Is it safe to assume the static function dentry_kill is always available
for tracing?

The script attaches an fprobe to dentry_kill without checking if it exists in
available_filter_functions.

If the compiler aggressively inlines or eliminates dentry_kill, the fprobe
creation will fail and abort the test.

[ ... ]

> +:;: "Start test workload" ;:
> +echo 1 >> tracing_on
> +
> +echo aaa > /tmp/hoge
> +echo bbb > /tmp/hoge
> +echo ccc > /tmp/hoge
> +rm /tmp/hoge

[Severity: Low]
Could this hardcoded path cause unexpected failures if /tmp/hoge already
exists, is owned by another user, or if tests run in parallel?

The test writes to a fixed global path instead of relying on the isolated
$TMPDIR standard within ftracetest.

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