Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] vsock/virtio: virtio_transport_send_pkt() drops RST replies on netns change --
commit 8627fb28e7c64d1d9b02b254ab9dca7140468f14 Author: Bobby Eshleman <[email protected]> vsock/virtio: support guest device network namespace This commit enables network namespace assignment for the virtio-vsock guest device, updating reachability checks in the virtio transport and re-reading the assigned namespace per packet in virtio_transport_rx_work(). It also uses vsock_maybe_set_connected() when transitioning sockets to the connected state so sockets that lose device reachability during namespace reassignment are reset. > diff --git a/net/vmw_vsock/virtio_transport.c > b/net/vmw_vsock/virtio_transport.c > index eced6929405df..06df82b130cff 100644 > --- a/net/vmw_vsock/virtio_transport.c > +++ b/net/vmw_vsock/virtio_transport.c > @@ -256,6 +256,15 @@ virtio_transport_send_pkt(struct sk_buff *skb, struct > net *net) > goto out_rcu; > } > > + /* A socket reset by an assign can still send credit updates from > + * recvmsg() and setsockopt(SO_RCVLOWAT). > + */ > + if (!vsock_g2h_net_reachable(net)) { > + kfree_skb(skb); > + len = -ENETUNREACH; > + goto out_rcu; > + } > + > /* If send_pkt_queue is empty, we can safely bypass this queue > * because packet order is maintained and (try) to put the packet > * on the virtqueue using virtio_transport_send_skb_fast_path. [ ... ] > diff --git a/net/vmw_vsock/virtio_transport_common.c > b/net/vmw_vsock/virtio_transport_common.c > index 1e762a480df49..0c864db2bc65b 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -1478,9 +1478,13 @@ virtio_transport_recv_connecting(struct sock *sk, > > switch (le16_to_cpu(hdr->op)) { > case VIRTIO_VSOCK_OP_RESPONSE: > - sk->sk_state = TCP_ESTABLISHED; > + if (!vsock_maybe_set_connected(vsk)) { > + skerr = ECONNRESET; > + err = -ENETUNREACH; > + goto destroy; > + } > + > sk->sk_socket->state = SS_CONNECTED; > - vsock_insert_connected(vsk); > sk->sk_state_change(sk); > break; > case VIRTIO_VSOCK_OP_INVALID: [ ... ] > @@ -1758,7 +1760,13 @@ virtio_transport_recv_listen(struct sock *sk, struct > sk_buff *skb, > if (virtio_transport_space_update(child, skb)) > child->sk_write_space(child); > > - vsock_insert_connected(vchild); > + if (!vsock_maybe_set_connected(vchild)) { > + release_sock(child); > + virtio_transport_reset_no_sock(t, skb, sock_net(sk)); > + sock_put(child); > + return -ENETUNREACH; > + } > + > vsock_enqueue_accept(sk, child); > virtio_transport_send_response(vchild, skb); [Severity: Medium] Will virtio_transport_send_pkt() drop the VIRTIO_VSOCK_OP_RST packets sent when a connection handshake races with a device namespace change? In net/vmw_vsock/af_vsock.c, vsock_maybe_set_connected() checks reachability via vsock_g2h_reachable_sk(): spin_lock_bh(&vsock_table_lock); reachable = vsock_g2h_reachable_sk(vsk); if (reachable) { sk_vsock(vsk)->sk_state = TCP_ESTABLISHED; __vsock_insert_connected(list, vsk); } spin_unlock_bh(&vsock_table_lock); For virtio_transport, vsock_g2h_reachable_sk(vsk) returns false when vsock_g2h_net_reachable(sock_net(sk_vsock(vsk))) is false. When vsock_maybe_set_connected() returns false in virtio_transport_recv_connecting(), jumping to destroy calls: virtio_transport_reset(vsk, skb) -> virtio_transport_send_pkt_info(vsk, &info) /* info.net = sock_net(sk) */ -> virtio_transport_send_pkt(skb, info.net) Similarly, in virtio_transport_recv_listen(), when vsock_maybe_set_connected() or vsock_assign_transport() fails after a namespace change, it calls: virtio_transport_reset_no_sock(t, skb, sock_net(sk)) -> virtio_transport_send_pkt(reply, sock_net(sk)) The same also happens in virtio_transport_recv_pkt() if an established socket is swept to TCP_CLOSE by vsock_g2h_reset_unreachable() before lock_sock(sk) acquires the socket lock, hitting the default case and calling virtio_transport_reset_no_sock(t, skb, net). Because virtio_transport_send_pkt() in net/vmw_vsock/virtio_transport.c now checks !vsock_g2h_net_reachable(net) and frees the skb with -ENETUNREACH, won't all of these outgoing VIRTIO_VSOCK_OP_RST replies be dropped before reaching the host peer, leaving a guest-initiated connection half-open on the host or causing a host-initiated connect() to hang until timeout? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
