Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 4 · Low: 2 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] check_deps() in tools/testing/selftests/vsock/vmtest.sh now
hard-requires two files that live outside the selftests directory —…
- [Medium] vm_ns_start() (vmtest.sh:588-596) reports success even when
the guest-side namespace setup fails.
- [Medium] test_ns_guest_assign_g2h_netns_old_conn_send_fails()
(vmtest.sh:1594-1653) orders three concurrent actors — the guest…
- [Medium] host_send_to_guest() (vmtest.sh:1582-1591) and
test_ns_guest_assign_g2h_netns_old_conn_send_fails()…
- [Low] guest_send_to_host() (vmtest.sh:1568) and host_send_to_guest()
(vmtest.sh:1591) release their background listeners with…
- [Low] vm_ns_start() (vmtest.sh:594-595) backgrounds `unshare -n sleep
infinity` in the guest and immediately echoes `$!`, with no…
Pre-existing issues:
- [Low] The new helpers splice an unquoted path into a string handed to
an inner shell: `timeout "${WAIT_PERIOD}" bash -c 'while [[ !
--
selftests/vsock: test the guest vsock device network namespace
This adds six new shared-VM tests to vmtest.sh that exercise assigning the
guest vsock device to a network namespace, using tools/net/ynl/pyynl/cli.py
plus Documentation/netlink/specs/vsock.yaml to drive dev-netns-set. The
dependency checks in check_deps() gain unshare, python3 and PyYAML.
A few questions about the new harness code below.
> diff --git a/tools/testing/selftests/vsock/vmtest.sh
> b/tools/testing/selftests/vsock/vmtest.sh
> index 310dfc2a39ad5..e94cc2fd90ccd 100755
> --- a/tools/testing/selftests/vsock/vmtest.sh
> +++ b/tools/testing/selftests/vsock/vmtest.sh
> @@ -17,6 +17,8 @@ readonly KERNEL_CHECKOUT=$(realpath
> "${SCRIPT_DIR}"/../../../../)
> source "${SCRIPT_DIR}"/../kselftest/ktap_helpers.sh
>
> readonly VSOCK_TEST="${SCRIPT_DIR}"/vsock_test
> +readonly YNL_CLI="${KERNEL_CHECKOUT}"/tools/net/ynl/pyynl/cli.py
> +readonly
> VSOCK_SPEC="${KERNEL_CHECKOUT}"/Documentation/netlink/specs/vsock.yaml
[ ... ]
> @@ -314,6 +347,18 @@ check_deps() {
> printf " Please build the kselftest vsock target.\n"
> exit "${KSFT_SKIP}"
> fi
> +
> + if ! python3 -c "import yaml" &>/dev/null; then
> + echo -e "skip: python3 yaml module not found!\n"
> + exit "${KSFT_SKIP}"
> + fi
> +
> + for dep in "${YNL_CLI}" "${VSOCK_SPEC}"; do
> + if [[ ! -r "${dep}" ]]; then
> + printf "skip: %s not found!\n" "${dep}"
> + exit "${KSFT_SKIP}"
> + fi
> + done
> }
[Severity: Medium]
Does this turn the whole suite into a skip when vmtest.sh is run from an
installed tree? Both new dependencies are anchored at KERNEL_CHECKOUT
(SCRIPT_DIR/../../../../), so they only resolve inside a kernel source
tree, and check_deps() exits before any test is selected:
readonly YNL_CLI="${KERNEL_CHECKOUT}"/tools/net/ynl/pyynl/cli.py
readonly
VSOCK_SPEC="${KERNEL_CHECKOUT}"/Documentation/netlink/specs/vsock.yaml
tools/testing/selftests/vsock/Makefile still ships only:
TEST_PROGS += vmtest.sh
TEST_GEN_FILES := vsock_test
There is no TEST_FILES entry for cli.py, its pyynl/lib package, or
vsock.yaml, so after "make -C tools/testing/selftests install" the new
check can never be satisfied, and the 24 pre-existing tests that do not
need ynl at all are skipped too. Would adding these to TEST_FILES (the way
tools/testing/selftests/net/ovpn/Makefile lists
../../../../net/ynl/pyynl/cli.py) work here, or alternatively limiting the
skip to the six new tests instead of the whole script?
[ ... ]
> @@ -528,6 +584,59 @@ vm_wait_for_ssh() {
> done
> }
>
> +# Create a local mode namespace in the VM and echo the pid holding it open.
> +vm_ns_start() {
> + local ns=$1
> +
> + vm_ssh "${ns}" -- \
> + "echo local > /proc/sys/net/vsock/child_ns_mode" &>/dev/null
> +
> + vm_ssh "${ns}" -- "unshare -n sleep infinity" \
> + '>/dev/null 2>&1 & echo $!'
> +}
[Severity: Medium]
Can vm_ns_start() ever report failure? The remote shell runs
unshare -n sleep infinity >/dev/null 2>&1 & echo $!
so the pid is printed at fork time, regardless of whether unshare actually
ran (missing binary, missing privilege, immediate exit). The write to
/proc/sys/net/vsock/child_ns_mode is also discarded with &>/dev/null and
its status is never looked at.
That makes the guard used by all six new tests unable to detect the
condition its message names:
nspid=$(vm_ns_start "init_ns")
if [[ -z "${nspid}" ]]; then
log_host "failed to create a namespace inside the guest"
return "${KSFT_FAIL}"
fi
For the two negative tests, test_ns_guest_local_connect_to_host_fails()
and test_ns_guest_assign_g2h_netns_init_ns_connect_fails(), the pass
condition is an empty outfile:
if [[ "${result}" == TEST ]]; then
return "${KSFT_FAIL}"
fi
return "${KSFT_PASS}"
An empty outfile is also what a broken harness produces, since vm_ns_exec()
discards stderr and does not check the status of nsenter, and
wait_for_listener() always returns 0. Would it make sense to verify the
namespace exists before testing, for example comparing
readlink /proc/<nspid>/ns/net against /proc/self/ns/net, so a setup failure
cannot be reported as ok?
[Severity: Low]
Separately, is there anything ordering the unshare against its first use?
The pid is echoed as soon as the child is forked, and the consumer is a
later, separate ssh invocation:
vm_ssh "${ns}" -- nsenter -t "${nspid}" -n sh -c "'${cmd}'"
If the guest child has not reached unshare(CLONE_NEWNET) yet,
/proc/<nspid>/ns/net is still the guest's initial netns, so dev-netns-set
targets the initial namespace and the _ok tests fail while the _fails tests
assert nothing. The window looks small, because a fresh SSH connection has
to be set up first, but nothing structurally prevents it.
> +
> +# Returns once the holder is gone, so that the namespace is unreferenced and
> +# the kernel can start tearing it down.
> +vm_ns_stop() {
> + local ns=$1
> + local nspid=$2
> +
> + vm_ssh "${ns}" <<-EOF &>/dev/null
> + kill ${nspid}
> + for ((i = 0; i < ${WAIT_PERIOD_MAX}; i++)); do
> + kill -0 ${nspid} 2>/dev/null || break
> + sleep 1
> + done
> + EOF
> +}
> +
> +# Runs in the guest's initial namespace when <nspid> is empty. The command
> must
> +# not contain single quotes.
> +vm_ns_exec() {
> + local ns=$1
> + local nspid=$2
> + local cmd=$3
> +
> + if [[ -z "${nspid}" ]]; then
> + vm_ssh "${ns}" -- "${cmd}"
> + return
> + fi
> +
> + vm_ssh "${ns}" -- nsenter -t "${nspid}" -n sh -c "'${cmd}'"
> +}
[Severity: Low]
This isn't a bug introduced by this patch, and the same idiom already
exists in vm_start() and check_ns_delete_doesnt_break_connection(), but the
quoting here rests on a comment rather than on the code: vm_ns_exec()
builds a remote root command as sh -c "'${cmd}'" and the new wait loops
splice an unquoted path into an inner shell:
timeout "${WAIT_PERIOD}" \
bash -c 'while [[ ! -s '"${outfile}"' ]]; do sleep 1; done'
Whitespace or shell metacharacters in the path (for instance through TMPDIR
affecting mktemp) would be interpreted by the inner shell and the wait loop
would return immediately on a parse error. Would passing the path as a
positional argument to bash -c be preferable?
[ ... ]
> @@ -1421,6 +1546,292 @@ test_ns_delete_both_ok() {
> check_ns_delete_doesnt_break_connection "both"
> }
>
> +# Send a string from the guest to a host listener and leave what the host
> +# received in <outfile>.
> +guest_send_to_host() {
> + local ns=$1
> + local nspid=$2
> + local port=$3
> + local outfile=$4
> + local cmd="echo TEST | socat -u STDIN VSOCK-CONNECT:2:${port}"
> + local pid
> +
> + socat -u VSOCK-LISTEN:"${port}" STDOUT > "${outfile}" 2>/dev/null &
> + pid=$!
> + host_wait_for_listener "${ns}" "${port}" "vsock"
> +
> + vm_ns_exec "${ns}" "${nspid}" "${cmd}" 2>/dev/null
> +
> + timeout "${WAIT_PERIOD}" \
> + bash -c 'while [[ ! -s '"${outfile}"' ]]; do sleep 1; done'
> +
> + terminate_pids "${pid}"
> +}
[Severity: Low]
Can the host listener still hold the port when the next test binds it?
terminate_pids() only signals and returns:
terminate_pids() {
local pid
for pid in "$@"; do
kill -SIGTERM "${pid}" &>/dev/null || :
done
}
There is no wait, and the six new shared-VM tests reuse hard-coded ports
(12345 in four tests, 12345..12349 and 12346 in the last two) back to back
in the same VM. socat's stderr goes to /dev/null so an EADDRINUSE is
invisible, and host_wait_for_listener() would then be satisfied by the
stale listener because wait_for_listener() just greps
ss --listening --vsock for the port, leaving the new test reading an outfile
nothing writes. Would a wait on the pid, a bind-success check, or unique
per-test ports close that?
> +
> +# Send a string from the host to a listener in the guest and leave what the
> +# guest received in <outfile>.
> +host_send_to_guest() {
> + local ns=$1
> + local nspid=$2
> + local port=$3
> + local outfile=$4
> + local cmd="socat -u VSOCK-LISTEN:${port} STDOUT"
> + local dst="VSOCK-CONNECT:${VSOCK_CID}:${port}"
> + local pid
> +
> + vm_ns_exec "${ns}" "${nspid}" "${cmd}" > "${outfile}" 2>/dev/null &
> + pid=$!
> + vm_ns_wait_for_listener "${ns}" "${nspid}" "${port}" "vsock"
> +
> + echo TEST | socat -u STDIN "${dst}" 2>/dev/null
> +
> + timeout "${WAIT_PERIOD}" \
> + bash -c 'while [[ ! -s '"${outfile}"' ]]; do sleep 1; done'
> +
> + terminate_pids "${pid}"
> +}
[Severity: Medium]
Which process does pid=$! name here? vm_ns_exec() is a shell function with
several commands, so backgrounding it makes bash fork a subshell, and $! is
that subshell rather than ssh. terminate_pids() then SIGTERMs only the
wrapper, while the ssh child and the guest-side
socat -u VSOCK-LISTEN:<port> STDOUT keep running.
The same applies to the sender in
test_ns_guest_assign_g2h_netns_old_conn_send_fails():
vm_ssh "init_ns" -- \
"(echo FIRST; sleep ${gap}; echo SECOND) |" \
"socat -u STDIN VSOCK-CONNECT:2:${port}" &>/dev/null &
sender=$!
If that is right, then the v2 changelog entry
"Terminate the guest socat sender too, it sometimes outlived the
test and kept holding on to a port the next test binds"
does not have the described effect, since killing a local wrapper subshell
cannot terminate the remote process.
A leftover guest socat also keeps a reference on the namespace created by
vm_ns_start(), so the postcondition documented on vm_ns_stop()
# Returns once the holder is gone, so that the namespace is
unreferenced and
# the kernel can start tearing it down.
may not hold, which is exactly what
test_ns_guest_assign_g2h_netns_reset_on_ns_delete_ok() relies on. Would
killing the remote command explicitly (or using ssh with a process group /
pkill inside the guest) be more reliable?
> +
> +test_ns_guest_assign_g2h_netns_old_conn_send_fails() {
> + local gap=$(( WAIT_PERIOD * 3 ))
> + local port=12346
[ ... ]
> + outfile=$(mktemp)
> + socat -u VSOCK-LISTEN:"${port}" STDOUT > "${outfile}" 2>/dev/null &
> + pid=$!
> + host_wait_for_listener "init_ns" "${port}" "vsock"
> +
> + # Send a message, wait, then send another. While waiting, assign the
> + # device to a namespace. Confirm the second message does not arrive.
> + vm_ssh "init_ns" -- \
> + "(echo FIRST; sleep ${gap}; echo SECOND) |" \
> + "socat -u STDIN VSOCK-CONNECT:2:${port}" &>/dev/null &
> + sender=$!
> +
> + sleep "${WAIT_PERIOD}"
> +
> + if ! vm_ns_assign_g2h "init_ns" "${nspid}"; then
[Severity: Medium]
Is a fixed sleep enough to order these three actors? The guest sender, the
host listener and the assign are only sequenced by sleeps, and nothing
confirms FIRST actually arrived before the assign (unlike
guest_send_to_host(), which polls the outfile).
If ssh plus socat startup in the VM takes longer than WAIT_PERIOD (3s), the
assign happens first, the connection is never established, the outfile stays
empty, and the test reports
log_host "no connection before the assign: [${result}]"
against a correctly behaving kernel.
> + log_host "failed to assign the vsock device to the guest ns"
> + terminate_pids "${pid}" "${sender}"
> + rm -f "${outfile}"
> + vm_ns_stop "init_ns" "${nspid}"
> + vm_reset_g2h
> + return "${KSFT_FAIL}"
> + fi
> +
> + # Let the second write happen and land, if it is going to.
> + sleep $(( gap + WAIT_PERIOD ))
[Severity: Medium]
And in the other direction, can the assign lose the race against the
sender's gap? vm_ns_assign_g2h() is an ssh connection plus nsenter plus a
python3 interpreter start and a PyYAML spec parse. If that exceeds the
sender's sleep of 9s, SECOND is delivered before the device moves and the
test reports "old connection still delivered after the assign" for a kernel
that is behaving as intended.
Could the same polling already used in guest_send_to_host() be applied
here, waiting for FIRST in the outfile before the assign and for the assign
to complete before the second write is expected?
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-vsock-guest-ns-v2-0-693bd78fde9e%40meta.com
