Re: [RFC PATCH 4/4] selftests/ftrace: Add a test for eBPF compiled fetchargs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Hardcoded little-endian expectation for string memory read as u32 causes selftest failure on big-endian architectures.
- [Medium] Improper cleanup of `trace-events-sample` module causes resource leaks on early test failure and violates test isolation on success.
- [Medium] The test fails to verify the fallback to the interpreter on unsupported cases, despite claims in the commit message.
- [Low] The test configures multiple fetch arguments but only verifies the dereferenced string, ignoring the register argument.
--

commit 4bb415f7039780520dc7b73a0fda0fb23e2616db
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

selftests/ftrace: Add a test for eBPF compiled fetchargs
    
Add a selftest for trace probe BPF compilation to verify that BPF is
correctly compiled and executes properly for valid configurations,
and falls back to interpreter on unsupported cases.

> Add a selftest for trace probe BPF compilation to verify that BPF is
> correctly compiled and executes properly for valid configurations,
> and falls back to interpreter on unsupported cases.

[Severity: Medium]
Does the test actually verify the fallback to the interpreter on unsupported
cases? The script configures REG and DEREF fetchargs, but there doesn't
appear to be a test case that intentionally uses an unsupported configuration
(such as dynamic strings) to verify the interpreter fallback mechanism.

> diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/test_bpf_fetchargs.tc b/tools/testing/selftests/ftrace/test.d/dynevent/test_bpf_fetchargs.tc
> new file mode 100644
> index 0000000000000..6c6f4dd4517ac
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/dynevent/test_bpf_fetchargs.tc
> @@ -0,0 +1,51 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: Dynamic event - test eBPF compiled fetchargs for tprobe and fprobe
> +# requires: dynamic_events "t[:[<group>/][<event>]] <tracepoint> [<args>]":README
> +
> +# Check if the sample module is loaded
> +if ! lsmod | grep -q trace_events_sample; then
> +  modprobe trace-events-sample || exit_unsupported
> +fi

[Severity: Medium]
If the script exits early on failure, will this module be left loaded
since there is no trap EXIT registered to clean it up?

[ ... ]

> +
> +# Now verify the trace
> +grep -q "test_tprobe.*myfoo=1819043176" trace || exit_fail

[Severity: Medium]
Will this hardcoded value cause false-positive test failures on big-endian
architectures? The test dereferences the string "hello" as a 32-bit unsigned
integer and unconditionally expects the little-endian integer representation
1819043176 (0x6c6c6568). On big-endian systems, this value would be
1751477356 (0x68656c6c).

[Severity: Low]
Does this grep ignore the register argument configured earlier? The tprobe
was configured to capture two arguments (mybar=$arg2 myfoo=+0($arg1):u32),
but this verification only checks for the presence of the dereferenced
myfoo value.

> +
> +if [ -d events/fprobes/test_fprobe ]; then
> +  grep -q "test_fprobe.*myfmt=1819043176" trace || exit_fail

[Severity: Low]
Similarly, does this grep check ignore the myfoo register argument configured
earlier for the fprobe?

> +fi
> +
> +echo > dynamic_events
> +rmmod trace-events-sample

[Severity: Medium]
If the trace-events-sample module was already loaded before the test started,
will this unconditional rmmod incorrectly unload it and alter the preexisting
system state?

> +
> +exit 0

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