Re: [PATCH v2 14/15] tools/rv: Add selftest for rv bpf monitors

[sashiko-bot@kernel.org]

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 <gmonaco@redhat.com>

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.

> 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?

> 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