Re: [PATCH v5 8/9] selftests/verification: Add tlob selftests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The test `uprobe_detail_waiting.tc` spawns an unbounded SCHED_FIFO priority-99 hog, causing a system-wide hard deadlock on single-CPU machines.
- [Medium] Empty `noinline` uprobe target functions will be optimized away by the compiler, breaking the stop probe and causing tests to fail.
- [Medium] The test suite fails when run from an installed kselftest environment due to relying on an unexported `VERIFICATIONTEST_BINDIR` environment variable.
- [Medium] The test scripts hardcode absolute paths to `/sys/kernel/tracing/`, breaking portability on systems where `tracefs` is mounted at `/sys/kernel/debug/tracing`.
- [Low] The `tlob_sym` ELF parser lacks bounds checking for the program headers offset (`e_phoff`), allowing out-of-bounds memory reads.
- [Low] The `run_tlob_tests.sh` standalone script relies on relative source paths and `make`, making it non-functional in installed kselftest environments.
--

--- Patch [8]: [PATCH v5 8/9] selftests/verification: Add tlob selftests ---
commit 71ad2abd1a52a7f2eb279e47c07bd2822ab59ec5
Author: Wen Yang <wen.yang@linux.dev>

selftests/verification: Add tlob selftests

Add seven ftrace-style test scripts for the tlob RV monitor under
tools/testing/selftests/verification/test.d/tlob/.  The tests cover
uprobe binding management, budget violation detection, and per-state
time accounting.

> diff --git a/tools/testing/selftests/verification/Makefile b/tools/testing/selftests/verification/Makefile
> index aa8790c22a71b..41445d15b86a0 100644
> --- a/tools/testing/selftests/verification/Makefile
> +++ b/tools/testing/selftests/verification/Makefile

[ ... ]

>  TEST_PROGS := verificationtest-ktap
>  TEST_FILES := test.d settings
>  EXTRA_CLEAN := $(OUTPUT)/logs/*
>  
> +TEST_GEN_FILES := tlob_sym tlob_target
> +
>  include ../lib.mk
> +
> +export VERIFICATIONTEST_BINDIR := $(OUTPUT)

[Severity: Medium]
Will this environment variable be available when the tests are run from an
installed kselftest environment? Since the underlying runner doesn't invoke
make during installed test execution, this might leave the variable
undefined.

> diff --git a/tools/testing/selftests/verification/test.d/tlob/run_tlob_tests.sh b/tools/testing/selftests/verification/test.d/tlob/run_tlob_tests.sh
> new file mode 100755
> index 0000000000000..13adf1eec9a26
> --- /dev/null
> +++ b/tools/testing/selftests/verification/test.d/tlob/run_tlob_tests.sh

[ ... ]

> +FTRACETEST="$SCRIPT_DIR/../../../ftrace/ftracetest"
> +
> +# Build test helpers
> +echo "Building tlob test helpers..."
> +make -C "$SCRIPT_DIR/../.." all

[Severity: Low]
Does this script work in an installed kselftest environment? The source
Makefile and .c files won't be present there, which could cause this make
command to fail.

> diff --git a/tools/testing/selftests/verification/test.d/tlob/uprobe_bind.tc b/tools/testing/selftests/verification/test.d/tlob/uprobe_bind.tc
> new file mode 100644
> index 0000000000000..baa6c5fa0ff2a
> --- /dev/null
> +++ b/tools/testing/selftests/verification/test.d/tlob/uprobe_bind.tc
> @@ -0,0 +1,35 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# description: Test tlob monitor uprobe binding (visible in monitor file, removable, duplicate rejected)
> +# requires: tlob:monitor
> +
> +UPROBE_TARGET="${VERIFICATIONTEST_BINDIR}/tlob_target"

[Severity: Medium]
Could this lead to path resolution errors? If VERIFICATIONTEST_BINDIR is
empty during installed test execution, it tries to execute /tlob_target
from the root directory.

> diff --git a/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_running.tc b/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_running.tc
> new file mode 100644
> index 0000000000000..f9a412930cda4
> --- /dev/null
> +++ b/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_running.tc

[ ... ]

> +echo 1 > /sys/kernel/tracing/events/rv/detail_env_tlob/enable
> +echo 1 > /sys/kernel/tracing/tracing_on
> +echo 1 > monitors/tlob/enable
> +echo > /sys/kernel/tracing/trace

[Severity: Medium]
Could hardcoding absolute paths to /sys/kernel/tracing/ cause issues on
systems where tracefs is mounted at /sys/kernel/debug/tracing? ftracetest
handles dynamically locating the mount point, so relative paths might be
more portable.

> diff --git a/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_waiting.tc b/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_waiting.tc
> new file mode 100644
> index 0000000000000..68aeb1fa2172f
> --- /dev/null
> +++ b/tools/testing/selftests/verification/test.d/tlob/uprobe_detail_waiting.tc

[ ... ]

> +# Pick the last CPU to avoid cpu0 which is used by vng infrastructure.
> +cpu=$(($(nproc) - 1))

[ ... ]

> +# Start the RT hog BEFORE the target so the target is immediately preempted
> +# when it calls tlob_preempt_work() (start probe fires), minimising running_ns.
> +chrt -f 99 taskset -c "$cpu" sh -c 'while true; do :; done' 2>/dev/null &
> +hog_pid=$!

[Severity: High]
Can this script cause a system-wide hard deadlock on single-CPU machines?

On a single core, cpu evaluates to 0, and spawning an unbounded SCHED_FIFO
priority-99 loop will permanently monopolize the CPU. With ftracetest
disabling RT throttling, the test script is starved and cannot proceed to
teardown the process.

> diff --git a/tools/testing/selftests/verification/tlob_sym.c b/tools/testing/selftests/verification/tlob_sym.c
> new file mode 100644
> index 0000000000000..a92fc49d13043
> --- /dev/null
> +++ b/tools/testing/selftests/verification/tlob_sym.c

[ ... ]

> +		Elf64_Phdr *phdrs = (Elf64_Phdr *)((char *)map + ehdr->e_phoff);
> +		int pi;
> +
> +		for (pi = 0; pi < ehdr->e_phnum; pi++) {
> +			Elf64_Phdr *ph = &phdrs[pi];
> +
> +			if (ph->p_type != PT_LOAD)

[Severity: Low]
Is it possible for this parser to perform an out-of-bounds memory read?

The parser verifies the section header offsets against the file size,
but it looks like the program header offset e_phoff and e_phnum are not
similarly validated before casting and reading ph->p_type.

> diff --git a/tools/testing/selftests/verification/tlob_target.c b/tools/testing/selftests/verification/tlob_target.c
> new file mode 100644
> index 0000000000000..adf4c2397fb37
> --- /dev/null
> +++ b/tools/testing/selftests/verification/tlob_target.c

[ ... ]

> +/* stop probe; noinline keeps the entry point visible to uprobes */
> +noinline void tlob_busy_work_done(void)
> +{
> +	/* empty: uprobe fires on entry */
> +}
> +
> +/* start probe; busy-spin so running_ns dominates */
> +noinline void tlob_busy_work(unsigned long duration_ms)
> +{

[ ... ]

> +	tlob_busy_work_done();
> +}

[Severity: Medium]
Will the compiler optimize away the call to tlob_busy_work_done()?

Since the function is empty and returns void, interprocedural analysis might
determine it has no side effects and remove the call from tlob_busy_work(),
which would prevent the uprobe stop event from firing. Could an empty
volatile asm statement prevent it from being optimized out?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787161646.git.wen.yang@linux.dev?part=8