On Wed, Sep 23, 2026 at 07:21:07PM +0000, [email protected] 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.
