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

On 8/3/26 11:31 AM, [email protected] wrote:
>>      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?

You are right, it is not.

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

This is a really good catch. Indeed arena never matches.

> 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 `\"`?

v2 drops the backslash, fixes README.rst and also updates the changelog 
accordingly.

Thx,
-Vineet

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