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.
[ ... ]
> @@ -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.
> +
> +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?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-vsock-guest-ns-v2-0-693bd78fde9e%40meta.com
