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 05:24:21PM -0700, Bobby Eshleman wrote:
> 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.

pw-bot: cr