Re: [PATCH net-next v6 7/8] selftests/vsock: test the guest vsock device network namespace

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] test_ns_guest_assign_g2h_netns_pins_ns_ok in
  tools/testing/selftests/vsock/vmtest.sh checks less than its…
- [Low] The ynl CLI's output goes straight onto the script's stdout,
  which is also the KTAP stream.
- [Low] vm_ns_stop() (vmtest.sh:657-667) waits for the namespace holder
  to go away with `kill ${nspid}` followed by up to WAIT_PERIOD_MAX (60)…

--

selftests/vsock: test the guest vsock device network namespace

This adds vmtest.sh tests for assigning the guest vsock device to a
network namespace. They cover namespace destruction and the device moving
between namespaces. The tests use the ynl cli.py and unshare, and a
missing dependency skips only the ns_guest_ tests.

> diff --git a/tools/testing/selftests/vsock/vmtest.sh b/tools/testing/selftests/vsock/vmtest.sh
> index 310dfc2a39ad5..a800b63dfbb11 100755
> --- a/tools/testing/selftests/vsock/vmtest.sh
> +++ b/tools/testing/selftests/vsock/vmtest.sh

[ ... ]

> @@ -528,6 +618,82 @@ vm_wait_for_ssh() {
>  	done
>  }
>  
> +# Create a local mode namespace in the VM and echo the pid holding it open.
> +vm_ns_start() {

[ ... ]

> +	nspid=$(vm_ssh "${ns}" -- "unshare -n sleep infinity" \
> +		'>/dev/null 2>&1 & echo $!')
> +	if [[ -z "${nspid}" ]]; then
> +		return
> +	fi

[ ... ]

> +# 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
> +}

[Severity: Low]
Can this loop run for the full WAIT_PERIOD_MAX seconds even after the
holder has exited?

vm_ns_start() starts the holder in the background from an ssh shell that
exits right away. The sleep process is then reparented to the guest's
PID 1, and nothing in the script waits on it.

kill -0 succeeds on an unreaped zombie. If the guest init does not reap
adopted orphans, every vm_ns_stop() call would run all 60 iterations,
even though the task has already dropped its netns reference.

vm_ns_stop() runs at least once in each of the seven ns_guest_ tests.
That could add 7+ minutes, against the 1200 second timeout in
tools/testing/selftests/vsock/settings. Whether this happens depends on
whether virtme-ng's init reaps orphans, and I could not confirm that.

Would checking /proc/${nspid}/status for state Z, or comparing
/proc/${nspid}/ns/net, remove that dependency?

[ ... ]

> +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"
> +}
> +
> +vm_reset_g2h() {
> +	vm_ns_assign_g2h "init_ns" "" &>/dev/null
> +}

[Severity: Low]
Does the cli.py output end up mixed into the KTAP stream here?

dev-netns-set has no reply attributes. For the empty response, ynl.py
_ops() appends None, and cli.py main() then pprints it:

tools/net/ynl/pyynl/cli.py:main() {
    ...
            reply = ynl.do(args.do, attrs, args.flags)
            output(reply)
    ...
    except NlError as e:
        print(e)
}

As a result, every successful assign prints a bare "None" line. On
failure, cli.py prints the NlError to stdout as well.

Only vm_reset_g2h() redirects this output. Five tests call
"if ! vm_ns_assign_g2h ...", and test_ns_guest_assign_g2h_netns_by_pid_ok()
calls cli.py directly. In all of these, the output passes through ssh
onto the script's stdout, in between the "ok N <test>" lines that
check_result() prints. Assign failures also never reach ${LOG} through
log_host.

Should these calls redirect the output, or capture it and pass it to
log_host?

[ ... ]

> @@ -1421,6 +1603,354 @@ test_ns_delete_both_ok() {

[ ... ]

> +test_ns_guest_assign_g2h_netns_pins_ns_ok() {

[ ... ]

> +	vm_ns_stop "init_ns" "${nspid}"
> +	sleep "${WAIT_PERIOD}"
> +
> +	outfile=$(mktemp)
> +	guest_send_to_host "init_ns" "" "${port}" "${outfile}"
> +	result=$(cat "${outfile}")
> +	if [[ "${result}" == TEST ]]; then
> +		log_host "device returned to the init ns instead of pinning it"
> +		rm -f "${outfile}"
> +		vm_reset_g2h
> +		return "${KSFT_FAIL}"
> +	fi
> +
> +	# Assigning it back is what releases the deleted namespace.
> +	vm_reset_g2h
> +
> +	guest_send_to_host "init_ns" "" "$(( port + 1 ))" "${outfile}"
> +	result=$(cat "${outfile}")
> +	rm -f "${outfile}"
> +
> +	if [[ "${result}" != TEST ]]; then
> +		log_host "device did not come back on reassign: [${result}]"
> +		return "${KSFT_FAIL}"
> +	fi

[Severity: Low]
Does this test check what its TEST_DESCS entry says?

  "Check the assignment keeps the ns alive when deleted, and that
  reassigning releases it."

Both assertions are connectivity checks run from the guest's init ns.

The first one only shows that the device did not return to init_net
after the holder exited. A dangling or freed netns would also make that
connect fail, and would only be caught if it happened to oops or warn.

The second one only shows that init_net works again after
vm_reset_g2h(). Nothing checks that the reference taken by
get_net_track() in vsock_g2h_net_swap() is later dropped by
put_net_track() in vsock_g2h_net_release(). So the claim in the comment
above vm_reset_g2h() is never verified.

If a later change leaked the netns reference on reassign, would this
test still return KSFT_PASS? Could the description be narrowed, or the
test extended to check that the namespace is actually released?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-vsock-guest-ns-v6-0-3737ce4673c0%40meta.com