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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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 (local)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-vsock-guest-ns-v5-0-48179b9b66b8@meta.com?part=5