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

[Bobby Eshleman <bobbyeshleman@meta.com>]

On Tue, Oct 13, 2026 at 10:51:55PM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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.

pw-bot: cr

> 
> --
> 
> 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