Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] kselftest/vsock: missing TEST_FILES breaks make install - [Medium] netlink: vsock dev-netns-set violates uAPI by not replying - [Medium] kselftest/vsock: subshell termination leaks ssh and socat processes --
commit 3122f274adb12a4633dc3d5fee699cdb3ce877cf Author: Bobby Eshleman <[email protected]> selftests/vsock: test the guest vsock device network namespace Add tests for guest vsock device namespace assignment, including ns destruction and the device moving between namespaces. > 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 > readonly TEST_GUEST_PORT=51000 > readonly TEST_HOST_PORT=50000 [Severity: Medium] Does this break running the tests from an installed location (via make install)? Since cli.py and vsock.yaml are not added to TEST_FILES in the selftests Makefile, they won't be copied during installation. When check_deps() runs in the installed environment, it will fail to find these files at the hardcoded ${KERNEL_CHECKOUT} paths and unconditionally skip the test suite. [ ... ] > +vm_ns_assign_g2h() { > + local ns=$1 > + local nspid=$2 > + > + vm_ns_exec "${ns}" "${nspid}" "python3 /root/ynl/cli.py --no-schema \ > + --spec /root/ynl/vsock.yaml --do dev-netns-set" > +} [Severity: Medium] Does dev-netns-set in vsock.yaml violate the Netlink subsystem guidelines? The operation in vsock.yaml is defined as "do: {}" with no reply, but the guidelines require that all new operations must reply with a full message (such as the assigned netns ID) rather than just an ACK to ensure future extensibility. [ ... ] > +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=$! [Severity: Medium] Does this leak the ssh client and remote socat processes? vm_ns_exec is a bash function, so backgrounding it means $! captures the local subshell's PID rather than the ssh client's PID. When terminate_pids sends SIGTERM to the subshell, the subshell exits, but bash does not forward the signal to its children (the ssh client). The orphaned ssh process keeps the connection open. Because no PTY is allocated, the remote sshd does not send SIGHUP, leaving the remote socat process running and holding the port open, which could cause EADDRINUSE failures in subsequent shared VM tests. [ ... ] > +test_ns_guest_assign_g2h_netns_old_conn_send_fails() { > + local gap=$(( WAIT_PERIOD * 3 )) > + local port=12346 > + local outfile > + local result > + local sender > + local nspid > + local pid > + > + nspid=$(vm_ns_start "init_ns") > + if [[ -z "${nspid}" ]]; then > + log_host "failed to create a namespace inside the guest" > + return "${KSFT_FAIL}" > + fi > + > + 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=$! [Severity: Medium] Similarly here, does backgrounding the vm_ssh bash function cause it to orphan the ssh and socat processes when sender is killed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
