Re: [PATCH net-next v2 4/6] vsock/virtio: support guest device network namespace

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Wed, Sep 23, 2026 at 07:21:03PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · 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] virtio_transport_rx_work() samples the g2h namespace once per
>   work invocation (`net = vsock_g2h_net_get()` at…
> - [Medium] By setting `.netns_assign_allow = true` and `.reset =
>   virtio_transport_reset` (net/vmw_vsock/virtio_transport.c:590-591)…
> - [Low] virtio_transport_reset_no_sock() is promoted to an exported
>   cross-module API (declared in include/linux/virtio_vsock.h,…
> 
> --
> 
> vsock/virtio: support guest device network namespace
> 
> This makes the virtio-vsock guest device assignable to a network namespace
> and initializes it to init_net.  The RX work now looks up the currently
> assigned namespace and resets received packets while no valid namespace is
> available, and new connected sockets are inserted via the new
> vsock_maybe_set_connected() helper so insertion is atomic with respect to
> namespace assignment.
> 
> A few questions below.
> 
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > index f91704731057e..5d15b6d6bdf7f 100644
> > --- a/include/linux/virtio_vsock.h
> > +++ b/include/linux/virtio_vsock.h
> > @@ -286,6 +286,9 @@ void virtio_transport_inc_tx_pkt(struct virtio_vsock_sock *vvs, struct sk_buff *
> >  u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 wanted);
> >  void virtio_transport_put_credit(struct virtio_vsock_sock *vvs, u32 credit);
> >  void virtio_transport_deliver_tap_pkt(struct sk_buff *skb);
> > +int virtio_transport_reset(struct vsock_sock *vsk, struct sk_buff *skb);
> > +int virtio_transport_reset_no_sock(const struct virtio_transport *t,
> > +				   struct sk_buff *skb, struct net *net);
> >  int virtio_transport_purge_skbs(void *vsk, struct sk_buff_head *list);
> >  int virtio_transport_read_skb(struct vsock_sock *vsk, skb_read_actor_t read_actor);
> >  int virtio_transport_notify_set_rcvlowat(struct vsock_sock *vsk, int val);
> > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > index 4f9aa9c4c3aa5..5ad93af4bd2b1 100644
> > --- a/net/vmw_vsock/virtio_transport.c
> > +++ b/net/vmw_vsock/virtio_transport.c
> 
> [ ... ]
> 
> > @@ -587,6 +587,8 @@ static struct virtio_transport virtio_transport = {
> >  		.seqpacket_has_data       = virtio_transport_seqpacket_has_data,
> >  
> >  		.msgzerocopy_allow        = virtio_transport_msgzerocopy_allow,
> > +		.netns_assign_allow       = true,
> > +		.reset                    = virtio_transport_reset,
> 
> [Severity: Medium]
> 
> Setting .netns_assign_allow = true makes vsock_netns_assignable() true for
> the g2h transport, which activates the reset sweep in
> vsock_g2h_reset_unreachable().  That sweep calls t->reset() while holding
> only vsock_register_mutex and a socket reference, never lock_sock(sk):
> 
> net/vmw_vsock/af_vsock.c:vsock_g2h_reset_unreachable() {
> 	...
> 		sk->sk_state = TCP_CLOSE;
> 		sk->sk_err = ECONNRESET;
> 		sk_error_report(sk);
> 	...
> 	list_for_each_entry_safe(vsk, tmp, &reset_list, pending_reset) {
> 		list_del_init(&vsk->pending_reset);
> 		t->reset(vsk, NULL);
> 	...
> }

Actually, lock_sock() after register mutex can deadlock on the
virtio_transport_recv_listen() path where the lock order is the exact
opposite, why I did not do this.

We might be able to lift the reset_list into vsock_g2h_net_assign, and
then lock_sock() after releasing the register mutex. Will have to
further evaluate...

> 
> virtio_transport_reset() -> virtio_transport_send_pkt_info() addresses the
> RST straight out of the socket:
> 
> 	if (!info->remote_cid) {
> 		dst_cid	= vsk->remote_addr.svm_cid;
> 		dst_port = vsk->remote_addr.svm_port;
> 
> Those fields are normally only stable under lock_sock(sk).
> 
> The sk_error_report() above wakes a blocking connect(), which then runs:
> 
> net/vmw_vsock/af_vsock.c:vsock_connect() {
> 	err = sock_error(sk);
> 	if (err) {
> 		sk->sk_state = TCP_CLOSE;
> 		sock->state = SS_UNCONNECTED;
> 	}
> }
> 
> and returns -ECONNRESET.  If userspace retries connect() immediately, the
> 
> 	memcpy(&vsk->remote_addr, remote_addr,
> 	       sizeof(vsk->remote_addr));
> 
> in vsock_connect() runs before vsock_assign_transport() blocks on
> vsock_register_mutex, so it is not serialized against the sweep that is
> still walking reset_list.
> 
> Can the sweep then read the freshly overwritten remote_addr and send the
> RST to the new destination, so the actual peer of the connection that was
> just torn down never gets a reset and waits for a timeout instead?
> 
> >  
> >  		.notify_poll_in           = virtio_transport_notify_poll_in,
> >  		.notify_poll_out          = virtio_transport_notify_poll_out,
> 
> [ ... ]
> 
> > @@ -633,7 +635,11 @@ static void virtio_transport_rx_work(struct work_struct *work)
> >  {
> >  	struct virtio_vsock *vsock =
> >  		container_of(work, struct virtio_vsock, rx_work);
> > +	struct virtio_transport *t = &virtio_transport;
> >  	struct virtqueue *vq;
> > +	struct net *net;
> > +
> > +	net = vsock_g2h_net_get();
> >  
> >  	mutex_lock(&vsock->rx_lock);
> >  
> > @@ -682,10 +688,14 @@ static void virtio_transport_rx_work(struct work_struct *work)
> >  
> >  			virtio_transport_deliver_tap_pkt(skb);
> >  
> > -			/* Force virtio-transport into global mode since it
> > -			 * does not yet support local-mode namespacing.
> > -			 */
> > -			virtio_transport_recv_pkt(&virtio_transport, skb, NULL);
> > +			/* The virtio send path does not use @net. */
> > +			if (unlikely(!net)) {
> > +				virtio_transport_reset_no_sock(t, skb, NULL);
> > +				kfree_skb(skb);
> > +				continue;
> > +			}
> > +
> > +			virtio_transport_recv_pkt(t, skb, net);
> 
> [Severity: Medium]
> 
> The namespace is sampled once per work invocation and then pinned with a
> hard reference for the whole drain loop:
> 
> 	net = vsock_g2h_net_get();
> 	...
> 	do {
> 		...
> 			virtio_transport_recv_pkt(t, skb, net);
> 	} while (!virtqueue_enable_cb(vq));
> 
> Meanwhile the assign path publishes the new namespace and then sweeps the
> connected table:
> 
> net/vmw_vsock/af_vsock.c:vsock_g2h_net_assign() {
> 	...
> 	rcu_assign_pointer(vsock_g2h_net, net);
> 	vsock_g2h_reset_unreachable(transport_g2h);
> 	...
> }
> 
> As soon as the new pointer is published, a connect() from the newly
> assigned namespace is permitted, because vsock_assign_transport() consults
> vsock_g2h_net_reachable() which already reads it.  If the host RESPONSE for
> that connection is drained by the batch still holding the previous
> namespace, vsock_find_connected_socket_net(&src, &dst, net) does not find
> the socket (vsock_net_check_mode() of the local-mode new net against the
> old net is false), so virtio_transport_recv_pkt() falls into
> 
> 	(void)virtio_transport_reset_no_sock(t, skb, net);
> 
> and the just-established connection is reset.  Is that acceptable for the
> remainder of an in-flight batch, or should the namespace be re-read per
> packet?
> 
> In the other direction, between the rcu_assign_pointer() above and the
> per-socket store in vsock_g2h_reset_unreachable():
> 
> 		sk->sk_state = TCP_CLOSE;
> 		sk->sk_err = ECONNRESET;
> 
> can the in-flight batch still resolve packets against the old namespace and
> deliver payload plus credit updates into sockets that the device no longer
> belongs to?
> 
> The commit message describes the batch granularity only for the dying
> namespace case:
> 
>   "received packets are reset until a new valid namespace has been assigned
>    and/or automatically returned to, and the next RX batch begins"
> 
> Should it also describe what happens to packets of the newly assigned
> namespace while an older batch is still draining?
> 
> >  		}
> >  	} while (!virtqueue_enable_cb(vq));
> >  
> 
> [ ... ]
> 
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index f225f53ed4bab..c24049b2a3866 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> 
> [ ... ]
> 
> > @@ -1315,8 +1315,8 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
> >   * loopback, this is the namespace of the socket. For vhost, this is the
> >   * namespace of the VM (i.e., vhost_vsock).
> >   */
> > -static int virtio_transport_reset_no_sock(const struct virtio_transport *t,
> > -					  struct sk_buff *skb, struct net *net)
> > +int virtio_transport_reset_no_sock(const struct virtio_transport *t,
> > +				   struct sk_buff *skb, struct net *net)

will fix. might have to grab a reference for every pkt, which I was
trying to avoid.

> 
> [Severity: Low]
> 
> This isn't a bug, but the comment above still enumerates only the loopback
> and vhost meanings of @net, while the helper is now exported via
> include/linux/virtio_vsock.h and the new caller in
> virtio_transport_rx_work() passes NULL.  Whether NULL is fine depends on
> the ->send_pkt behind @t: virtio_transport_send_pkt() ignores @net, and
> vhost_transport_send_pkt() passes it to vhost_vsock_get() ->
> vsock_net_check_mode(), which treats NULL as VSOCK_NET_MODE_GLOBAL.  Could
> the comment be extended to say that NULL is permitted and what it means?
> 
> >  {
> >  	struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
> >  	struct virtio_vsock_pkt_info info = {
> > @@ -1355,6 +1355,7 @@ static int virtio_transport_reset_no_sock(const struct virtio_transport *t,
> >  
> >  	return t->send_pkt(reply, net);
> >  }
> > +EXPORT_SYMBOL_GPL(virtio_transport_reset_no_sock);
> 
> [ ... ]


will correct.