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 <[email protected]> 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/[email protected]?part=8
