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

[Stefano Garzarella <sgarzare@redhat.com>]

On Fri, Nov 06, 2026 at 12:02:44AM -0500, Oleg Drokin wrote:
>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

Can you elaborate a bit more this? I didn't get how a packet can sit in 
multiple queues...

>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.
>

Whould be nice to have some performance numbers.
Are we affecting latency/throughput in some way?

>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;

When this can happen?

>+	vsk = vsock_sk(skb->sk);
>+
>+	if (le16_to_cpu(hdr->op) != VIRTIO_VSOCK_OP_CREDIT_UPDATE) {
>+		WRITE_ONCE(vsk->credit_update, NULL);

Why?

>+		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;
>+	}
>+

This code is hard to read without comments that explains why we are 
doing some checks...

>+	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;

I don't really like to start to store transport specific stuff in 
vsock_sock. Did you explore any alternative?

If this can't be remove, we should really explain why in the commit 
message and here, or even better provide a generic API that transport 
can use to store private stuff there.

Thanks,
Stefano

>+
> 	/* 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
>