On Wed, Sep 23, 2026 at 07:21:05PM +0000, [email protected] wrote:
> 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?
>
> [ ... ]
valid, will fix.
>
> > @@ -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
valid, will fix.
>
> 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?
Sure, if that is the only way the test could break. Will look into and
fix / find a better way.
>
> [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.
ssh is unlikely to win this race, but wierder things have happend. I
guess we can wait until the ns exists before proceeding.
>
> > +
> > +# 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?
won't fix in this series.
>
> [ ... ]
>
> > @@ -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?
will fix by waiting on pid.
>
> > +
> > +# 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?
>
This is probably the right thing to do... though I'm not a big fan of
having a custom wait_for_X function for every single type of event.
I'll explore if its possible to have more generic/reusable coordination,
or otherwise fix this as is.
