Re: [bpf-next 4/4] selftests/bpf: vmtest.sh: preserve command quoting when running in the VM

[email protected]
Newsgroups gmane.linux.kernel,gmane.linux.kernel.bpf
Message-ID <6d96174316041032ba75e8b9901245772bd4aad2873b795fd71df038bf909763@mail.kernel.org>
> diff --git a/tools/testing/selftests/bpf/vmtest.sh b/tools/testing/selftests/bpf/vmtest.sh
> index 9ca8022853933..6a3d026d76bd6 100755
> --- a/tools/testing/selftests/bpf/vmtest.sh
> +++ b/tools/testing/selftests/bpf/vmtest.sh
> @@ -428,8 +428,17 @@ main()
>
>  	if [[ $# -eq 0  && "${debug_shell}" == "no" ]]; then
>  		echo "No command specified, will run ${DEFAULT_COMMAND} in the vm"
> -	else
> -		command="$@"
> +	elif [[ $# -gt 0 ]]; then
> +		# Quote each argument so the command survives into the guest: the
> +		# host expands ${command} into the generated init script, which
> +		# the guest bash then parses as shell source. Without the %q
> +		# escapes an argument with a space or a glob (e.g. -a 'verifier_*')
> +		# is re-split and expanded against /root/bpf there.
> +		#
> +		# Skip this when there is no command: printf '%q ' would still
> +		# apply the format once and emit '', which is not the empty
> +		# command that -s (debug shell) expects.
> +		command=$(printf '%q ' "$@")
>  	fi
>
>  	local kconfig_file="${OUTPUT_DIR}/latest.config"

The commit message states:

>     Note this makes the trailing command strictly an argv rather than a shell
>     snippet: passing it pre-quoted as one word, e.g.
>
>             vmtest.sh -- "./test_progs -t foo"
>
>     no longer works, and neither does embedding guest-side shell syntax such
>     as ';' or a redirection. Neither form is documented - usage() and
>     README.rst both show the command unquoted - and 'sh -c ...' still works.

Is the claim about README.rst accurate? Looking at
tools/testing/selftests/bpf/README.rst lines 106-115, there is a
documented vmtest.sh invocation whose trailing command is explicitly
quoted:

  $ PLATFORM=riscv64 CROSS_COMPILE=riscv64-linux-gnu- \
    tools/testing/selftests/bpf/vmtest.sh \
    -l <path of local rootfs image> -- \
    ./test_progs -d \
        \"$(cat tools/testing/selftests/bpf/DENYLIST.riscv64 | ... | tr -s '\n' ',' )\"

The `\"` are escaped in the user's host shell, so vmtest.sh's main()
receives argv[3] as the literal 10-character string `",exceptions,"`
including the double quotes.

Under the old `command="$@"`, the guest bash's second parse of the init
script performed quote removal on those literal quotes, so test_progs
received `-d ,exceptions,`. That is exactly what the `\"` in the recipe
are for: they survive the host and are consumed by the guest.

With `command=$(printf '%q ' "$@")` the quotes are escaped
(`\",exceptions,\"`) and reach the binary verbatim, so test_progs now
receives `-d ",exceptions,"`.

Downstream that argument goes to parse_arg() ARG_TEST_NAME_GLOB_DENYLIST
-> parse_test_list() (tools/testing/selftests/bpf/testing_helpers.c),
which strtok_r()s on ',' and hands each field to insert_test() with
is_glob_pattern=true. So the filter list becomes three globs instead of
one: ["] [exceptions] ["], two of which can never match a test name in
glob_match() (test_progs.c).

For DENYLIST.riscv64 as it stands today the damage is confined to those
two junk filters, because the file's first line is a comment and
therefore reduces to an empty leading field.

The same recipe applied to a denylist file whose first line is a real
entry silently drops that entry. With DENYLIST.asan-shaped input:

  OLD guest -> test_progs -d [*arena*,task_local_data,uprobe_multi_test,]
    filters: [*arena*] [task_local_data] [uprobe_multi_test]
  NEW guest -> test_progs -d ["*arena*,task_local_data,uprobe_multi_test,"]
    filters: ["*arena*] [task_local_data] [uprobe_multi_test] ["]

`"*arena*` matches nothing, so the *arena* tests are no longer denied
and run anyway, a silent loss of denylist coverage rather than an error.

Should the commit message be corrected to acknowledge that README.rst
does document a quoted invocation that breaks with this change, and
should README.rst lines 109-115 be updated in the same patch to remove
the now-harmful `\"`?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30836252778
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.