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

[Stefano Garzarella <sgarzare@redhat.com>]

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

Why this Fixes tag?

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

Can we add a comment to explain this check?

>+	}

nit: try to follow the style, e.g. leave a blank line here.

>+	virtio_transport_consume_skb_sent(skb, false);

mmm, this entire function is very similar to the code we have in 
vhost_transport_do_send_pkt() where we also call 
virtio_transport_consume_skb_sent() but with consume=true.

What about renaming this entire function to vhost_vsock_release_skb(), 
adding a new `delivered` parameter to be passed to 
virtio_transport_consume_skb_sent() (and to be used to call or not 
kfree_skb()) ?

Thanks,
Stefano

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