Re: [PATCH net-next v3 4/7] vsock: add a netlink command to assign the g2h device to a netns

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] Take a namespace that cannot reach the assigned G2H device, on
  a system with vhost-vsock (H2G) and vsock_loopback loaded.
- [Medium] `vsock_g2h_reset_unreachable()` in net/vmw_vsock/af_vsock.c
  resets sockets without lock_sock() while the device stays live.
- [Low] No in-tree transport sets the new capability at this commit.
- [Low] `vsock_g2h_net_assign()` in net/vmw_vsock/af_vsock.c sends the
  success reply before it commits the assignment.

--

vsock: add a netlink command to assign the g2h device to a netns

This patch adds a vsock generic netlink family with two commands.
VSOCK_CMD_DEV_NETNS_SET moves the guest's G2H vsock device into the
caller's network namespace, and VSOCK_CMD_DEV_NETNS_GET reports where the
device is. After a move, connected sockets that can no longer reach the
device are closed with ECONNRESET, and the assignment holds a reference on
the target namespace.

Link: https://lore.kernel.org/all/20200427142518.uwssa6dtasrp3bfc@steredhat/

> diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
> index fe43e85959583..9276479004c0f 100644
> --- a/Documentation/admin-guide/sysctl/net.rst
> +++ b/Documentation/admin-guide/sysctl/net.rst
> @@ -529,6 +529,43 @@ their hosts. The behavior of VSOCK sockets in a network namespace is determined

[ ... ]

> +Support is transport dependent. A G2H transport that does not implement the
> +move refuses the command with ``EOPNOTSUPP``. Of the in-tree guest transports,
> +only virtio-vsock implements it.

[Severity: Low]
This is a minor point. Is this sentence accurate at this commit?

No in-tree transport sets netns_assign_allow yet, so this check in
vsock_g2h_net_assign() always fails:

	if (!vsock_net_assignable(transport_g2h)) {

As a result, VSOCK_CMD_DEV_NETNS_SET returns -EOPNOTSUPP (or -ENODEV) for
every G2H transport. The new exports vsock_g2h_net_get(), vsock_g2h_net_eq()
and vsock_maybe_set_connected() also have no callers yet.

The next patch in the series, "vsock/virtio: support guest device network
namespace", adds .netns_assign_allow = true to virtio_transport and calls
those helpers. So the mismatch only exists at bisect points between the two
patches. Could this sentence move into that patch?

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 8f34bf68f9164..e663be3bbfa0b 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c

[ ... ]

> @@ -564,7 +632,12 @@ static bool vsock_use_local_transport(unsigned int remote_cid)
>  		return true;
>  
>  	if (transport_g2h) {
> -		return remote_cid == transport_g2h->get_local_cid();
> +		u32 cid = __vsock_registered_transport_cid(&transport_g2h, net);
> +
> +		/* The device may be unreachable from @net, in which case
> +		 * @remote_cid is not the local CID.
> +		 */
> +		return cid != VMADDR_CID_ANY && remote_cid == cid;
>  	} else {
>  		return remote_cid == VMADDR_CID_HOST;
>  	}

[Severity: Medium]
Suppose the G2H device has been assigned to a local mode namespace, and
vhost-vsock and vsock_loopback are also loaded. What happens in a namespace
that can no longer reach the device?

In that namespace, IOCTL_VM_SOCKETS_GET_LOCAL_CID in vsock_dev_do_ioctl()
gets VMADDR_CID_ANY from transport_g2h and falls back to transport_h2g:

	cid = vsock_registered_transport_cid(&transport_g2h, net);
	if (cid == VMADDR_CID_ANY)
		cid = vsock_registered_transport_cid(&transport_h2g,
						     net);

vhost_transport_get_local_cid() returns VHOST_VSOCK_DEFAULT_HOST_CID, so the
namespace is told its local CID is 2. __vsock_find_cid(net, 2) is also true
through the transport_h2g && cid == VMADDR_CID_HOST check, so bind() to
CID 2 succeeds.

A connect() to CID 2 then enters the transport_g2h branch above, because the
pointer is non-NULL. It returns false because cid is VMADDR_CID_ANY, so the
remote_cid == VMADDR_CID_HOST loopback fallback in the else branch is never
reached.

vsock_assign_transport() then picks transport_g2h for
remote_cid <= VMADDR_CID_HOST and fails at the new check:

	if (vsock_g2h_unreachable(new_transport, sock_net(sk))) {
		ret = -ENETUNREACH;

Should an unreachable G2H transport be treated the same as an absent one
here, so that connecting to the namespace's own reported CID uses loopback?
The later virtio patch in the series makes this path reachable and does not
change this logic.

[ ... ]

> @@ -742,8 +823,267 @@ bool vsock_find_cid(unsigned int cid)

[ ... ]

> +/* Close every connected socket of @t that can no longer reach the g2h device.
> + */
> +static void vsock_g2h_reset_unreachable(const struct vsock_transport *t)
> +{
> +	struct vsock_sock *vsk, *tmp;
> +	struct sock *sk;
> +	int i;
> +
> +	lockdep_assert_held(&vsock_register_mutex);
> +
> +	spin_lock_bh(&vsock_table_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++) {
> +		list_for_each_entry_safe(vsk, tmp, &vsock_connected_table[i],
> +					 connected_table) {
> +			sk = sk_vsock(vsk);
> +
> +			if (vsk->transport != t ||
> +			    sk->sk_state == TCP_CLOSE ||
> +			    vsock_g2h_reachable_sk(vsk))
> +				continue;
> +
> +			/* If close has already been scheduled for this socket,
> +			 * we can skip.
> +			 */
> +			if (vsk->close_work_scheduled)
> +				continue;
> +
> +			sk->sk_state = TCP_CLOSE;
> +			sk->sk_err = ECONNRESET;
> +			sk_error_report(sk);
> +
> +			__vsock_remove_connected(vsk);

[Severity: Medium]
Can these writes race with the socket's own state machine?

Only vsock_register_mutex and vsock_table_lock are held here, and neither
one excludes lock_sock() holders. close_work_scheduled is read the same way.

The existing lockless resets, virtio_vsock_reset_sock() and
vhost_vsock_reset_orphans(), only run when the device is going away. Here
the device keeps running, so its RX path can work on the same socket at the
same time.

With the later virtio patch, virtio_transport_rx_work() checks
vsock_g2h_net_eq(net) for each packet but does not synchronize with the
swap. That patch's commit message also notes that an assign can land while
rx_work is draining a batch:

CPU1 (assign)                      CPU2 (rx_work)
                                   vsock_g2h_net_eq(net) returns true
vsock_g2h_net_swap()
                                   virtio_transport_recv_pkt()
                                     lock_sock(sk)
                                     sk_state is TCP_ESTABLISHED
vsock_g2h_reset_unreachable()
  sk->sk_state = TCP_CLOSE
  sk->sk_err = ECONNRESET
  __vsock_remove_connected(vsk)
                                     virtio_transport_recv_connected()
                                       virtio_transport_do_close()
                                         sk->sk_state = TCP_CLOSING

If an incoming SHUTDOWN or RST arrives at that point, doesn't the socket end
up in TCP_CLOSING with ECONNRESET set and outside the connected table?

There is a similar window on the send side. vsock_connectible_sendmsg() may
already have passed its sk_err check and could enqueue one more chunk
through the device after the sweep. KCSAN would also likely report these
unannotated stores against the locked writers.

Refcounting looks balanced, because vsock_remove_connected() re-checks table
membership. So the effect appears to be limited to inconsistent socket
state.

[ ... ]

> +static int vsock_g2h_net_assign(struct net *net, struct sk_buff *msg,
> +				struct genl_info *info)
> +{

[ ... ]

> +	ret = -EMSGSIZE;
> +	hdr = genlmsg_iput(msg, info);
> +	if (!hdr)
> +		goto unlock;
> +	genlmsg_end(msg, hdr);
> +
> +	ret = genlmsg_reply(msg, info);
> +	msg = NULL;
> +	if (ret)
> +		goto unlock;
> +
> +	old = vsock_g2h_net_swap(net, &old_tracker);
> +
> +	/* See vsock_maybe_set_connected() comment about synchronizing with
> +	 * connecting sockets.
> +	 */
> +	vsock_g2h_reset_unreachable(transport_g2h);

[Severity: Low]
Is this ordering intended? The success reply goes out before
vsock_g2h_net_swap() and vsock_g2h_reset_unreachable() run.

This means userspace can see VSOCK_CMD_DEV_NETNS_SET succeed while
vsock_g2h_net still points at the old namespace and the established sockets
there are still open. Those sockets' send and receive paths take lock_sock(),
not vsock_register_mutex, so the mutex does not hold them off.

The move also depends on the reply being delivered. genlmsg_reply() does a
MSG_DONTWAIT unicast, so if the requester's receive buffer is full it
returns -EAGAIN and the swap is skipped. Neither the commit message nor the
docs mention this failure mode.

Could the swap and the sweep run first, with the reply sent after them?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-vsock-guest-ns-v3-0-b32447c66b23%40meta.com