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 b8eab48b6f555e1cbf805a0b83f5c8f89bdd6029 Author: Bobby Eshleman <[email protected]> vsock/virtio: support guest device network namespace This commit adds network namespace assignment support to the guest virtio-vsock driver, restricting reachability to the assigned namespace and atomically checking reachability when inserting sockets into the connected table. > 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. [ ... ] > @@ -682,10 +697,15 @@ 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. > + /* Only grab a reference if the net changes underneath > + * us. > */ > - virtio_transport_recv_pkt(&virtio_transport, skb, NULL); > + if (unlikely(!vsock_g2h_net_eq(net))) { > + put_net_track(net, &ns_tracker); > + net = vsock_g2h_net_get(&ns_tracker); > + } > + > + virtio_transport_recv_pkt(t, skb, net); > } > } while (!virtqueue_enable_cb(vq)); > > diff --git a/net/vmw_vsock/virtio_transport_common.c > b/net/vmw_vsock/virtio_transport_common.c > index 1e762a480df49..7efcbdc7e04fc 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -1468,8 +1468,8 @@ void virtio_transport_release(struct vsock_sock *vsk) > EXPORT_SYMBOL_GPL(virtio_transport_release); > > static int > -virtio_transport_recv_connecting(struct sock *sk, > - struct sk_buff *skb) > +virtio_transport_recv_connecting(struct sock *sk, struct sk_buff *skb, > + struct virtio_transport *t, struct net *net) > { > struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb); > struct vsock_sock *vsk = vsock_sk(sk); > @@ -1478,9 +1478,14 @@ 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)) { > + virtio_transport_reset_no_sock(t, skb, net); > + skerr = ECONNRESET; > + err = -ENETUNREACH; > + goto close; > + } > + > sk->sk_socket->state = SS_CONNECTED; > - vsock_insert_connected(vsk); > sk->sk_state_change(sk); > break; [ ... ] > @@ -1750,7 +1754,7 @@ virtio_transport_recv_listen(struct sock *sk, struct > sk_buff *skb, > */ > if (ret || vchild->transport != &t->transport) { > release_sock(child); > - virtio_transport_reset_no_sock(t, skb, sock_net(sk)); > + virtio_transport_reset_no_sock(t, skb, net); > sock_put(child); > return ret; > } > @@ -1758,7 +1762,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, net); > + sock_put(child); > + return -ENETUNREACH; > + } [Severity: Medium] As reported by Stefano Garzarella on v4 [1], will the VIRTIO_VSOCK_OP_RST replies sent by virtio_transport_reset_no_sock(t, skb, net) still get dropped by virtio_transport_send_pkt() when a namespace assignment races with an incoming packet? In virtio_transport_rx_work(), net is snapshotted via vsock_g2h_net_get() before calling virtio_transport_recv_pkt(t, skb, net). Inside virtio_transport_recv_pkt(), the socket sk is looked up with vsock_find_connected_socket_net() or vsock_find_bound_socket_net(), both of which require vsock_net_check_mode(sock_net(sk), net) to be true. Because a namespace's vsock mode is fixed after initialization, sock_net(sk) (as well as sock_net(child) in virtio_transport_recv_listen()) and the snapshot net passed to virtio_transport_recv_pkt() always have the same reachability with respect to vsock_g2h_net. If a concurrent vsock_g2h_net_assign() changes vsock_g2h_net to an unreachable namespace after the socket lookup in virtio_transport_recv_pkt() (for instance while acquiring lock_sock(sk) or setting up the child socket): 1. In virtio_transport_recv_connecting(), vsock_maybe_set_connected(vsk) returns false because sock_net(sk) is no longer reachable from the new vsock_g2h_net. 2. In virtio_transport_recv_listen(), vsock_assign_transport(vchild, vsk) or vsock_maybe_set_connected(vchild) fails for the same reason. 3. In virtio_transport_recv_pkt(), if vsock_g2h_reset_unreachable() swept sk before lock_sock(sk) completed, sock_flag(sk, SOCK_DONE) is set. All of these paths call: virtio_transport_reset_no_sock(t, skb, net) -> t->send_pkt(reply, net) -> virtio_transport_send_pkt(reply, net) However, net is still the pre-assignment namespace snapshot from virtio_transport_rx_work(). When virtio_transport_send_pkt() checks vsock_g2h_net_reachable(net) against the newly assigned vsock_g2h_net, it evaluates to false, freeing the RST skb and returning -ENETUNREACH without sending the reset to the peer. Should virtio_transport_send_pkt() allow outgoing VIRTIO_VSOCK_OP_RST packets (or should virtio_transport_reset_no_sock() bypass the net reachability check) so the peer receives the reset when a handshake or connection races with namespace assignment? [1] https://lore.kernel.org/r/atjvzn-zPYJjLAhy@sgarzare-redhat -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
