Re: [PATCH v2 14/15] tools/rv: Add selftest for rv bpf monitors
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-01 16:03:53
Also in:
bpf
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] selftests/rv: catch-all regex bypasses exact monitor name validation if BPF uncompiled - [Medium] selftests/rv: spurious failure due to missing check for BPF monitor presence -- commit 47b33f7df8aa5888f987d177930041a5edcca5ec Author: Gabriele Monaco [off-list ref] tools/rv: Add selftest for rv bpf monitors This commit adds selftest cases for the rv userspace tool BPF monitors. It tests BPF monitor listing, functionality, traces, and reactors.
quoted hunk ↗ jump to hunk
diff --git a/tools/verification/rv/tests/rv_list.t b/tools/verification/rv/tests/rv_list.t index 201af33a52ccc..3c94f2af21097 100644 --- a/tools/verification/rv/tests/rv_list.t +++ b/tools/verification/rv/tests/rv_list.t@@ -17,13 +19,15 @@ check "verify list subcommand help" \ all_nested=$(grep : $RVDIR/available_monitors | cut -d: -f2 | paste -s | sed 's/\t/\\|/g') all_non_nested=$(grep -v : $RVDIR/available_monitors | cut -d: -f2 | paste -s | sed 's/\t/\\|/g') sched_monitors=$(grep sched: $RVDIR/available_monitors | cut -d: -f2 | paste -s | sed 's/\t/\\|/g') +all_bpf=$(find "$RVTOOL/bpf_monitors" -name "*.o" -exec basename \{} .o \; | paste -s | sed 's/\t/\\|/g') description_state="[[:space:]]\+[[:print:]]\+\[\(OFF\|ON\)\]" line_nested=" - \($all_nested\)${description_state}" line_non_nested="\($all_non_nested\)${description_state}" +line_bpf="\($all_bpf\)${description_state}" # List monitors and containers check "list all monitors" \ - "$RV list" 0 "" "" "^\($line_nested\|$line_non_nested\)$" + "$RV list" 0 "" "" "^\($line_nested\|$line_non_nested\|$line_bpf\)$"
[Severity: Medium] If the BPF monitors are not compiled, all_bpf will be empty, making line_bpf evaluate to "\(\)[[:space:]]\+[[:print:]]\+\[\(OFF\|ON\)\]". When this is combined in the catch-all regex "^(...) $", does this accidentally allow any line starting with a space to pass validation instead of strictly enforcing the monitor names?
quoted hunk ↗ jump to hunk
diff --git a/tools/verification/rv/tests/rv_mon.t b/tools/verification/rv/tests/rv_mon.t index cbc346c74c71a..8e14471661c7d 100644 --- a/tools/verification/rv/tests/rv_mon.t +++ b/tools/verification/rv/tests/rv_mon.t@@ -23,13 +24,34 @@ if [ -d $RVDIR/monitors/wwnr ]; then check "invalid reactor name" \ "$RV mon wwnr -r invalid" 1 "failed to set invalid reactor, is it available?" +check "invalid BPF reactor name" \ + "$RV mon nohz -r invalid" 1 "failed to set invalid reactor, is it available?" + +check "invalid BPF reactor name check available" \ + "$RV mon nohz -r invalid" 1 "available BPF reactors: nop [a-z]\+" \ + "available reactors:"
[Severity: Medium] Since this check for the nohz BPF monitor is guarded only by the presence of the wwnr in-kernel monitor, can it cause spurious failures if the BPF monitors were not built but wwnr is present? If nohz.o is missing, the command will fail with "monitor nohz does not exist" rather than the expected invalid reactor error. -- Sashiko AI review · https://sashiko.dev/#/patchset/20261001152042.124445-1-gmonaco@redhat.com?part=14