Re: [PATCH v5 14/18] tracing/remotes: selftests: Add a test for the dump_on_panic tracefs file

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The test contains an unconditional early return, leaving dead code intended to manually trigger a kernel panic, which fails to validate the feature and creates a dangerous booby trap.
- [Medium] Unconditional write to `dump_on_panic` in the shared `setup_remote()` function will break all ftrace remote selftests on older kernels.
- [Medium] The new test fails instead of gracefully skipping on kernels that do not support the dump_on_panic feature.
--

--- Patch [14]: [PATCH v5 14/18] tracing/remotes: selftests: Add a test for the dump_on_panic tracefs file ---
commit 77956bd02c91415a58b7ae353d502fbc7538fb12
Author: Vincent Donnefort <[email protected]>

tracing/remotes: selftests: Add a test for the dump_on_panic tracefs file

This patch adds a selftest for the dump_on_panic tracefs file. It creates a
new test script and updates the remote setup functions to configure the
parameter.

> diff --git a/tools/testing/selftests/ftrace/test.d/remotes/dump_on_panic.tc b/tools/testing/selftests/ftrace/test.d/remotes/dump_on_panic.tc
> new file mode 100644
> index 0000000000000..cb9b4c6dc86c5
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/remotes/dump_on_panic.tc
> @@ -0,0 +1,51 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: Test trace remote dump_on_panic
> +# requires: remotes/test
> +
> +. $TEST_DIR/remotes/functions
> +
> +test_dump_on_panic()
> +{
> +    #
> +    # Toggle when the buffer is loaded
> +    #
> +    assert_loaded

[Severity: Medium]
Since this test specifically targets the dump_on_panic file, should there be
an existence check here (such as [ -f dump_on_panic ] || exit_unsupported) to
prevent false negatives?

Without this check, the test fails instead of gracefully skipping on kernels
that do not support the dump_on_panic feature.

> +    echo 1 > dump_on_panic
> +    echo 0 > dump_on_panic

[ ... ]

> +    echo 1 > dump_on_panic
> +    echo 1 > tracing_on
> +    echo 0 > tracing_on
> +
> +    # REMOVE ME FOR A PROPER OOPS TEST
> +    return
> +
> +    echo 1 > tracing_on
> +
> +    for i in $(seq 1 32); do
> +        echo $i > write_event
> +    done
> +
> +    echo c > /proc/sysrq-trigger
> +}

[Severity: Medium]
Is this early return intentional?

This skips the actual panic-triggering code, leaving the dump_on_panic
feature untested. Furthermore, leaving the sysrq-trigger command behind
an early return could be dangerous if someone removes the return without
realizing it will crash the host machine during a CI run.

> +
> +if [ -z "$SOURCE_REMOTE_TEST" ]; then
> +    set -e
> +    setup_remote_test
> +    test_dump_on_panic
> +fi
> diff --git a/tools/testing/selftests/ftrace/test.d/remotes/functions b/tools/testing/selftests/ftrace/test.d/remotes/functions
> index 4a14aa72fdf09..bdd28b5b85966 100644
> --- a/tools/testing/selftests/ftrace/test.d/remotes/functions
> +++ b/tools/testing/selftests/ftrace/test.d/remotes/functions
> @@ -9,6 +9,7 @@ setup_remote()
>  	cd remotes/$name/
>  	echo 0 > tracing_on
>  	echo 0 > dmesg
> +	echo 0 > dump_on_panic

[Severity: Medium]
Could this unconditional write cause failures on older kernels?

Because setup_remote() is a shared setup function, and test scripts execute
with set -e (errexit), if this is run against an older kernel lacking the
dump_on_panic file, the shell will immediately abort. This would break all
ftrace remote selftests on those kernels.

>  	clear_trace
>  	echo 7 > buffer_size_kb
>  	echo 0 > events/enable

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14