[PATCH net v2 0/3] vsock/virtio: bound what a peer can make the host queue

[PATCH net v2 0/3] vsock/virtio: bound what a peer can make the host queue

[Oleg Drokin]

A virtio-vsock peer that does not consume what it is sent can make the
local side queue memory that no limit covers. On a host the peer is a
guest, and the memory is GFP_KERNEL, charged to no memory cgroup. Two
ways remain on net/main:

 - the send credit trusts the fwd_cnt the peer reports, so a peer that
   never reads but keeps acknowledging gets unlimited credit (patch 2);
 - credit updates are not bounded at all: a peer can solicit them with
   OP_CREDIT_REQUEST, or by sending small packets that a reader drains,
   while posting no rx buffers (patch 3). The same works against a
   guest whose device stops reading the tx virtqueue.

Patch 1 fixes the accounting of packets that vhost drops on rx errors;
patch 2 relies on bytes_unsent being right.

These were found by an LLM-assisted security review of vhost-vsock,
done while porting a project from Xen to KVM. Patches 2 and 3 were
reproduced (below); the error paths of patch 1 were not, as they need a
guest that posts malformed rx buffers. The same review found the
receive skb sizing problem that Daehyeon Ko independently fixed in
d5e912448cd9 ("vhost/vsock: size receive SKBs from declared payload"),
so that part is not in this series.

Nothing changes on the wire, except that fewer credit updates are sent.

Tested in an x86_64 qemu/KVM guest running the kernel under test
(net/main fafbc4051123). For the host side, the "guest" is a user-space
program driving /dev/vhost-vsock directly, talking to plain AF_VSOCK
host sockets. For the guest side, a nested guest running the same
kernel uses a vhost-user vsock device that stops reading its tx
virtqueue:

  case                                      net/main      this series
  lying fwd_cnt, no rx buffers, 2 s of      256 MiB       256 KiB
  non-blocking send() on one socket         queued        (one window)
  20000 OP_CREDIT_REQUEST, no rx buffers    +18.5 MiB     +0
  20000 1-byte packets, each read by host   +18.1 MiB     +24 KiB
  guest, tx virtqueue full, 200             200 credit    2
  OP_CREDIT_REQUEST on one connection       updates

vsock_test from tools/testing/vsock passes over loopback and between a
nested guest and its host. A bidirectional stress test (up to 512
connections streaming both ways, readers draining in small reads) runs
without stalls, and vsock_perf throughput stays within run-to-run noise.
A test that moves sockets between transports in connect() while their
packets are queued passes. With KASAN and lockdep enabled, these tests
show no reports.

The reproducers (a user-space program that drives /dev/vhost-vsock as
the guest, and the vhost-user device) are available on request.

Changes in v2:
 - patch 3: fold a new credit update into the one the socket already has
   queued, rewriting its header under the queue lock, instead of
   tracking it from virtio_vsock_sock with a refcount, an skb destructor
   and transmit hooks (review). vhost and the guest driver are no longer
   touched. A failed add to a full guest tx virtqueue no longer lets
   another update through (Sashiko).
 - patch 2: commit message only, the writer's wakeup is gated by
   sock_writeable().
 - all patches: say how the issues were found; patch 3 gives its test
   results.
 - rebased on net/main fafbc4051123.

v1: https://lore.kernel.org/netdev/20261105000141.3617241-1-green@linuxhacker.ru/

Oleg Drokin (3):
  vhost/vsock: release the accounting of packets dropped on rx errors
  vsock/virtio: bound the send credit by bytes not yet consumed by the
    transport
  vsock/virtio: fold credit updates into the one a socket has queued

 drivers/vhost/vsock.c                   | 23 +++++++--
 include/linux/virtio_vsock.h            | 66 ++++++++++++++++++++++++-
 include/net/af_vsock.h                  |  6 +++
 net/vmw_vsock/virtio_transport_common.c |  8 ++-
 net/vmw_vsock/vsock_loopback.c          |  2 +
 5 files changed, 98 insertions(+), 7 deletions(-)

-- 
2.56.0

[PATCH net v2 1/3] vhost/vsock: release the accounting of packets dropped on rx errors

[Oleg Drokin]

When vhost_transport_do_send_pkt() fails to deliver a packet to the guest
(unexpected output buffers, a buffer smaller than the header, a fault
while copying the header or the payload) it frees the skb with
kfree_skb() and gives up. The success path does more:
virtio_transport_consume_skb_sent() subtracts the payload from the
socket's bytes_unsent (SIOCOUTQ), and for a reply the queued_replies
counter is decremented, restarting the guest's tx queue once it drops
below the ring size.

The error paths skip both. Every such drop leaves SIOCOUTQ too high for
the life of the socket, and every dropped reply permanently takes one
slot of the queued_replies budget, so that after a ring's worth of them
vhost stops processing the guest's tx queue for good.

Release both on the error paths, as the success path does.

Found by an LLM-assisted security review of vhost-vsock, done while
porting a project from Xen to KVM. These error paths were not
reproduced: they need a guest that posts malformed rx buffers.

Fixes: 433fc58e6bf2 ("VSOCK: Introduce vhost_vsock.ko")
Fixes: e6ab45005772 ("vsock/virtio: add SIOCOUTQ support for all virtio based transports")
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 drivers/vhost/vsock.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 88a310390eb0..b0004cb48999 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -104,6 +104,21 @@ static bool vhost_transport_has_remote_cid(struct vsock_sock *vsk, u32 cid)
 	return found;
 }
 
+/* Undo the send accounting of an undelivered skb; true if tx may restart. */
+static bool vhost_vsock_drop_skb(struct vhost_vsock *vsock, struct sk_buff *skb)
+{
+	bool restart_tx = false;
+
+	if (virtio_vsock_skb_reply(skb)) {
+		int val = atomic_dec_return(&vsock->queued_replies);
+
+		restart_tx = val + 1 == vsock->vqs[VSOCK_VQ_TX].num;
+	}
+	virtio_transport_consume_skb_sent(skb, false);
+	kfree_skb(skb);
+	return restart_tx;
+}
+
 static void
 vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 			    struct vhost_virtqueue *vq)
@@ -162,14 +177,14 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 		}
 
 		if (out) {
-			kfree_skb(skb);
+			restart_tx |= vhost_vsock_drop_skb(vsock, skb);
 			vq_err(vq, "Expected 0 output buffers, got %u\n", out);
 			break;
 		}
 
 		iov_len = iov_length(&vq->iov[out], in);
 		if (iov_len < sizeof(*hdr)) {
-			kfree_skb(skb);
+			restart_tx |= vhost_vsock_drop_skb(vsock, skb);
 			vq_err(vq, "Buffer len [%zu] too small\n", iov_len);
 			break;
 		}
@@ -212,7 +227,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 
 		nbytes = copy_to_iter(hdr, sizeof(*hdr), &iov_iter);
 		if (nbytes != sizeof(*hdr)) {
-			kfree_skb(skb);
+			restart_tx |= vhost_vsock_drop_skb(vsock, skb);
 			vq_err(vq, "Faulted on copying pkt hdr\n");
 			break;
 		}
@@ -221,7 +236,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
 					   offset,
 					   &iov_iter,
 					   payload_len)) {
-			kfree_skb(skb);
+			restart_tx |= vhost_vsock_drop_skb(vsock, skb);
 			vq_err(vq, "Faulted on copying pkt buf\n");
 			break;
 		}
-- 
2.56.0

[PATCH net v2 2/3] vsock/virtio: bound the send credit by bytes not yet consumed by the transport

[Oleg Drokin]

The send credit is

    min(peer_buf_alloc, buf_alloc) - (tx_cnt - peer_fwd_cnt)

where peer_fwd_cnt is taken unchecked from every header the peer sends.
A peer that never consumes what it is sent but keeps reporting
fwd_cnt == tx_cnt gets unlimited credit: every send() is accepted and
the skbs pile up in the transport's queue (on a host, vhost's
send_pkt_queue, for a guest that posts no rx buffers). The skbs are
GFP_KERNEL allocations charged to no memory cgroup. The peer learns
tx_cnt easily when the host side relays a stream it also controls.

Commit 8ee784fdf006 ("vsock/virtio: cap TX credit to local buffer size")
bounded peer_buf_alloc, but not peer_fwd_cnt.

bytes_unsent (maintained for SIOCOUTQ) counts what was handed to the
transport and not yet consumed by the device. It is local knowledge, so
bounding it by the same window limits a lying peer to one window of
queued data per socket.

For an honest peer, bytes_unsent can exceed tx_cnt - peer_fwd_cnt only
by data the peer already has but the transport has not yet released:

 - on a host, the delivered part of an skb that vhost splits across rx
   buffers smaller than the skb: the peer acknowledges the pieces, but
   bytes_unsent drops only once the whole skb is delivered, so at most
   one skb (64 KiB);
 - in a guest, tx buffers the host has consumed but
   virtio_transport_tx_work() has not reaped yet: vhost passes the
   packet to the host socket before it marks the buffer used, so the
   host's credit update can arrive first.

Either way the window shrinks only until the transport catches up;
nothing stalls and nothing changes on the wire. The writer is woken by
sock_wfree() once enough of the queued skbs are freed, as it already is
when credit comes back.

With a user-space vhost-vsock "guest" that posts no rx buffers and
acknowledges everything, one host socket queued 256 MiB in under 2 s
(SIOCOUTQ 268304384, where the test stops); with this patch it stays at
one window (SIOCOUTQ 262144).

Found by an LLM-assisted security review of vhost-vsock, done while
porting a project from Xen to KVM, and reproduced as above.

Fixes: 06a8fc78367d ("VSOCK: Introduce virtio_vsock_common.ko")
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 net/vmw_vsock/virtio_transport_common.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 1e762a480df4..74a8fdf825d5 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1031,14 +1031,17 @@ EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_has_data);
 
 static s64 virtio_transport_has_space(struct virtio_vsock_sock *vvs)
 {
+	s64 tx_buf_size = virtio_transport_tx_buf_size(vvs);
 	s64 bytes;
 
 	/* Use s64 arithmetic so if the peer shrinks peer_buf_alloc while
 	 * we have bytes in flight (tx_cnt - peer_fwd_cnt), the subtraction
 	 * does not underflow.
 	 */
-	bytes = (s64)virtio_transport_tx_buf_size(vvs) -
-		(vvs->tx_cnt - vvs->peer_fwd_cnt);
+	bytes = tx_buf_size - (vvs->tx_cnt - vvs->peer_fwd_cnt);
+
+	/* peer_fwd_cnt is peer-controlled, bytes_unsent is not. */
+	bytes = min_t(s64, bytes, tx_buf_size - (s64)vvs->bytes_unsent);
 	if (bytes < 0)
 		bytes = 0;
 
-- 
2.56.0

[PATCH net v2 3/3] vsock/virtio: fold credit updates into the one a socket has queued

[Oleg Drokin]

A credit update carries no data, so the send credit never limits it,
and the peer can solicit one at will: with OP_CREDIT_REQUEST, or by
sending data that a local reader drains (a read that empties the
receive queue sends one). A peer that posts no rx buffers can therefore
make the other side queue credit updates without limit, about 1 KiB of
unaccounted slab each for a 44-byte packet.

Every packet carries fwd_cnt and buf_alloc, so while the socket's newest
packet in a transport's send queue is a credit update, a new one adds
nothing but newer counters. Record that packet, and the queue it is in,
in vsock_sock. When another credit update for the socket is queued to
the same queue, copy its fwd_cnt and buf_alloc into the queued one,
under the queue lock, and free it. Queuing any other packet of the
socket clears the record, and so does removing the recorded packet from
the queue (dequeue, purge, loopback's splice), under the same lock and
before the transport reads the header. So the counters are only ever
written into a packet that has not gone out, and the fwd_cnt the peer
sees never goes back.

The queue has to be recorded because vhost has one per device, and a
socket's packets can sit in two of them at once (a guest restarted with
the same CID). The send queues are shared by every connection on a
device, so looking only at the tail is not enough: a peer with two
connections can alternate its requests so that the tail is always the
other connection's update.

A packet that a transport dequeues and puts back at the head (vhost
with no rx buffer, the guest driver with a full tx virtqueue) is not
recorded again, as the socket may have queued packets after it. A peer
can thus make a connection queue at most two credit updates in a send
queue, plus one per packet the socket itself queues. The folded updates
are not sent, so vsockmon does not see them.

Counting credit updates in queued_replies instead would not work: most
of them are sent by local reads, not in answer to the peer, and under
bidirectional traffic vhost would stop reading the guest's tx queue
while the guest stops reading its rx queue, each waiting for the other.

With a user-space vhost-vsock "guest" that posts no rx buffers, 20000
OP_CREDIT_REQUEST made the host queue a credit update for each
(+18.5 MiB of slab); with this patch it queues two. A guest whose
device stops reading its tx virtqueue queued one per request (200 for
200); with this patch, two.

Found by an LLM-assisted security review of vhost-vsock, done while
porting a project from Xen to KVM, and reproduced as above.

Fixes: 06a8fc78367d ("VSOCK: Introduce virtio_vsock_common.ko")
Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
---
 include/linux/virtio_vsock.h            | 66 ++++++++++++++++++++++++-
 include/net/af_vsock.h                  |  6 +++
 net/vmw_vsock/virtio_transport_common.c |  1 +
 net/vmw_vsock/vsock_loopback.c          |  2 +
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index f91704731057..0a5477a0443c 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -99,12 +99,70 @@ virtio_vsock_skb_queue_head(struct sk_buff_head *list, struct sk_buff *skb)
 	spin_unlock_bh(&list->lock);
 }
 
+/* Called with list->lock held. Returns true if @skb, a credit update, was
+ * folded into the socket's newest packet in @list and must be freed.
+ */
+static inline bool
+__virtio_vsock_skb_fold_credit_update(struct sk_buff_head *list,
+				      struct sk_buff *skb)
+{
+	struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
+	struct virtio_vsock_hdr *old_hdr;
+	struct vsock_sock *vsk;
+	struct sk_buff *old;
+
+	if (!skb->sk)
+		return false;
+	vsk = vsock_sk(skb->sk);
+
+	if (le16_to_cpu(hdr->op) != VIRTIO_VSOCK_OP_CREDIT_UPDATE) {
+		WRITE_ONCE(vsk->credit_update, NULL);
+		return false;
+	}
+
+	old = READ_ONCE(vsk->credit_update);
+	if (!old || READ_ONCE(vsk->credit_update_queue) != list) {
+		WRITE_ONCE(vsk->credit_update_queue, list);
+		WRITE_ONCE(vsk->credit_update, skb);
+		return false;
+	}
+
+	old_hdr = virtio_vsock_hdr(old);
+	if ((s32)(le32_to_cpu(hdr->fwd_cnt) - le32_to_cpu(old_hdr->fwd_cnt)) >= 0) {
+		old_hdr->fwd_cnt = hdr->fwd_cnt;
+		old_hdr->buf_alloc = hdr->buf_alloc;
+	}
+	return true;
+}
+
+/* Called with the lock held of the queue @skb is being removed from. Every
+ * removal from a send queue must call it, as the helpers below do.
+ */
+static inline void __virtio_vsock_skb_untrack(struct sk_buff *skb)
+{
+	struct vsock_sock *vsk;
+
+	if (!skb->sk)
+		return;
+	vsk = vsock_sk(skb->sk);
+	if (READ_ONCE(vsk->credit_update) == skb)
+		WRITE_ONCE(vsk->credit_update, NULL);
+}
+
+/* May free @skb: the caller must not touch it afterwards. */
 static inline void
 virtio_vsock_skb_queue_tail(struct sk_buff_head *list, struct sk_buff *skb)
 {
+	bool folded;
+
 	spin_lock_bh(&list->lock);
-	__skb_queue_tail(list, skb);
+	folded = __virtio_vsock_skb_fold_credit_update(list, skb);
+	if (!folded)
+		__skb_queue_tail(list, skb);
 	spin_unlock_bh(&list->lock);
+
+	if (folded)
+		consume_skb(skb);
 }
 
 static inline struct sk_buff *virtio_vsock_skb_dequeue(struct sk_buff_head *list)
@@ -113,6 +171,8 @@ static inline struct sk_buff *virtio_vsock_skb_dequeue(struct sk_buff_head *list
 
 	spin_lock_bh(&list->lock);
 	skb = __skb_dequeue(list);
+	if (skb)
+		__virtio_vsock_skb_untrack(skb);
 	spin_unlock_bh(&list->lock);
 
 	return skb;
@@ -120,7 +180,11 @@ static inline struct sk_buff *virtio_vsock_skb_dequeue(struct sk_buff_head *list
 
 static inline void virtio_vsock_skb_queue_purge(struct sk_buff_head *list)
 {
+	struct sk_buff *skb;
+
 	spin_lock_bh(&list->lock);
+	skb_queue_walk(list, skb)
+		__virtio_vsock_skb_untrack(skb);
 	__skb_queue_purge(list);
 	spin_unlock_bh(&list->lock);
 }
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index e8481f12312e..7ef6f0c8dd55 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -70,6 +70,12 @@ struct vsock_sock {
 	u64 buffer_max_size;
 	bool recvmsg_inq;
 
+	/* virtio: queued credit update to fold into, and its send queue.
+	 * Here, as connect() may free trans while packets are queued.
+	 */
+	struct sk_buff *credit_update;
+	struct sk_buff_head *credit_update_queue;
+
 	/* Private to transport. */
 	void *trans;
 };
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 74a8fdf825d5..ba814e1c0344 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1911,6 +1911,7 @@ int virtio_transport_purge_skbs(void *vsk, struct sk_buff_head *queue)
 			continue;
 
 		__skb_unlink(skb, queue);
+		__virtio_vsock_skb_untrack(skb);
 		__skb_queue_tail(&freeme, skb);
 
 		if (virtio_vsock_skb_reply(skb))
diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
index 8068d1b6e851..bffdc0b1321c 100644
--- a/net/vmw_vsock/vsock_loopback.c
+++ b/net/vmw_vsock/vsock_loopback.c
@@ -131,6 +131,8 @@ static void vsock_loopback_work(struct work_struct *work)
 
 	spin_lock_bh(&vsock->pkt_queue.lock);
 	skb_queue_splice_init(&vsock->pkt_queue, &pkts);
+	skb_queue_walk(&pkts, skb)
+		__virtio_vsock_skb_untrack(skb);
 	spin_unlock_bh(&vsock->pkt_queue.lock);
 
 	while ((skb = __skb_dequeue(&pkts))) {
-- 
2.56.0