Re: [PATCH net-next v2 6/6] selftests/vsock: test the netns assign privilege checks

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Wed, Sep 23, 2026 at 07:21:07PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 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] Both new selftests
>   (`test_ns_guest_assign_g2h_netns_no_cap_net_admin_fails`,…
> - [Low] The two new shared-VM tests omit the `vm_reset_g2h()` cleanup
>   that every other ns_guest_assign_g2h_netns_* test in USE_SHARED_VM…
> 
> --
> 
> selftests/vsock: test the netns assign privilege checks
> 
> Adds two vmtest.sh cases that attempt VSOCK_CMD_DEV_NETNS_SET without
> CAP_NET_ADMIN and from an unprivileged user namespace, and enables
> CONFIG_USER_NS in the vsock selftest config.
> 
> > diff --git a/tools/testing/selftests/vsock/vmtest.sh b/tools/testing/selftests/vsock/vmtest.sh
> > index e94cc2fd90ccd..4f42bcdac3f6c 100755
> > --- a/tools/testing/selftests/vsock/vmtest.sh
> > +++ b/tools/testing/selftests/vsock/vmtest.sh
> 
> [ ... ]
> 
> > @@ -187,6 +195,8 @@ readonly TEST_DESCS=(
> >  	ns_guest_assign_g2h_netns_host_connect_ok
> >  	ns_guest_assign_g2h_netns_reset_on_ns_delete_ok
> >  	ns_guest_assign_g2h_netns_old_conn_send_fails
> > +	ns_guest_assign_g2h_netns_no_cap_net_admin_fails
> > +	ns_guest_assign_g2h_netns_unpriv_user_ns_fails
> >  )
> >  readonly NS_MODES=("local" "global")
> 
> [Severity: Low]
> Both new tests are added to USE_SHARED_VM but neither calls
> vm_reset_g2h() on any exit path, unlike every other
> ns_guest_assign_g2h_netns_* test in that list.
> 
> Should these follow the sibling pattern?  If the privilege check under
> test is broken, the assign succeeds and the guest's single g2h device is
> left owned by the transient namespace created by unshare, while the
> harness moves on to the next test in the same VM.
> 
> The kernel does restore this by itself once that namespace is reaped:
> 
> net/vmw_vsock/af_vsock.c:vsock_g2h_net_reset() {
> 	if (likely(rcu_access_pointer(vsock_g2h_net) != net))
> 		return;
> 
> 	mutex_lock(&vsock_register_mutex);
> 	if (rcu_access_pointer(vsock_g2h_net) == net)
> 		rcu_assign_pointer(vsock_g2h_net, &init_net);
> 	...
> }
> 
> registered as .pre_exit in vsock_pernet_ops, and the next test's
> vm_ns_assign_g2h() overwrites the pointer anyway.  The residual window is
> the asynchronous teardown that ..._reset_on_ns_delete_ok already documents
> ("the namespace itself is dismantled from a workqueue, so the device does
> not come back the same instant").  Since the new tests are appended last
> in TEST_NAMES and USE_SHARED_VM, a default run is unaffected; this only
> shows up with selective or reordered invocation.

Yes, these tests should call it regardless. Relying on cleanup makes the
pass/fail race-y.

> 
> [ ... ]
> 
> > @@ -1832,6 +1842,48 @@ test_ns_guest_assign_g2h_netns_reset_on_ns_delete_ok() {
> >  	return "${KSFT_PASS}"
> >  }
> >  
> > +test_ns_guest_assign_g2h_netns_no_cap_net_admin_fails() {
> > +	local cmd="unshare -n setpriv --bounding-set=-net_admin"
> > +	local rc
> > +
> > +	vm_ssh "init_ns" -- "${cmd}" python3 /root/ynl/cli.py --no-schema \
> > +		--spec /root/ynl/vsock.yaml --do dev-netns-set &>/dev/null
> > +	rc=$?
> > +
> > +	if [[ "${rc}" -eq 0 ]]; then
> > +		log_host "assign unexpectedly succeeded without CAP_NET_ADMIN"
> > +		return "${KSFT_FAIL}"
> > +	fi
> > +
> > +	return "${KSFT_PASS}"
> > +}
> 
> [Severity: Medium]
> Can this test pass without the capability check ever being reached?  The
> only assertion is rc -eq 0 => FAIL, output is discarded with &>/dev/null,
> and there is no positive control, so any nonzero status reads as
> KSFT_PASS.
> 
> A few concrete sources of a nonzero status that are not -EPERM:
> 
> net/vmw_vsock/af_vsock.c:vsock_nl_dev_netns_set_doit() {
> 	int err = vsock_g2h_net_assign(genl_info_net(info));
> 
> 	if (err == -ENODEV)
> 		NL_SET_ERR_MSG(info->extack,
> 			       "no guest-to-host transport is loaded");
> 	else if (err == -EOPNOTSUPP)
> 	...
> }
> 
> and cli.py collapses all of those to the same status as -EPERM:
> 
> tools/net/ynl/pyynl/cli.py:main() {
>     except NlError as e:
>         print(e)
>         sys.exit(1)
> }
> 
> vm_ssh() also returns ssh's own status, so a transport failure (255) or a
> remote shell failure (127, e.g. an installed util-linux that does not
> accept setpriv --bounding-set=-net_admin) is indistinguishable from the
> expected refusal.  A python-side failure such as a missing
> /root/ynl/vsock.yaml exits 1 too.
> 
> Would it be worth asserting the privileged case succeeds first?  The file
> already has the helper the sibling tests use as a precondition:
> 
> 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"
> }
> 
> On the changelog note "ynl's cli.py does not report the errno": is that
> accurate?  NlError.__str__() in tools/net/ynl/pyynl/lib/ynl.py does:
> 
>         msg += os.strerror(self.error)
> 
> so the error string is printed and the test could match "Operation not
> permitted" instead of sending everything to /dev/null.

I considered this but felt like grepping stderr/stdout is somewhat
unnatural. My plan was to later propose that cli.py returns the err via
exit code, and then update these tests.

First checking the privileged assign sounds like a reasonable middle
ground to rule out the other error states.

> 
> > +
> > +test_ns_guest_assign_g2h_netns_unpriv_user_ns_fails() {
> > +	local unpriv_uid=65534
> > +	local unpriv
> > +	local rc
> > +
> > +	unpriv="setpriv --reuid=${unpriv_uid} --regid=${unpriv_uid}"
> > +	unpriv="${unpriv} --clear-groups"
> > +
> > +	if ! vm_ssh "init_ns" -- "${unpriv} unshare -U true"; then
> > +		log_host "unprivileged user namespaces unavailable, skipping"
> > +		return "${KSFT_SKIP}"
> > +	fi
> > +
> > +	vm_ssh "init_ns" -- "${unpriv} unshare -Urn" \
> > +		python3 /root/ynl/cli.py --no-schema \
> > +		--spec /root/ynl/vsock.yaml --do dev-netns-set &>/dev/null
> > +	rc=$?
> > +
> > +	if [[ "${rc}" -eq 0 ]]; then
> > +		log_host "assign unexpectedly succeeded for unprivileged user"
> > +		return "${KSFT_FAIL}"
> > +	fi
> > +
> > +	return "${KSFT_PASS}"
> > +}
> 
> [Severity: Medium]
> Same rc != 0 => KSFT_PASS inference here, plus the availability probe is
> not the command being measured:
> 
> 	probe:    ${unpriv} unshare -U true
> 	measured: ${unpriv} unshare -Urn python3 /root/ynl/cli.py ...
> 
> If the extra -r/-n setup or the python startup under uid 65534 fails, the
> result is KSFT_PASS rather than KSFT_SKIP or KSFT_FAIL, and the child user
> namespace never actually holds CAP_NET_ADMIN, which is the condition the
> commit message says is being tested:
> 
>     the other confirms that CAP_NET_ADMIN in an unprivileged user ns
>     alone is insufficient.
> 
> Would checking the reported error string, or asserting the privileged
> assign works in the same test, make these two cases fail if
> GENL_ADMIN_PERM is ever dropped from VSOCK_CMD_DEV_NETNS_SET?

Yep, can follow the same approach as above.