Re: [PATCH net 1/2] vsock/virtio: collapse receive queue under memory pressure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] If `skb_copy_bits()` fails during receive queue collapse, the error path drops `new_skb` without updating `rx_bytes` and `buf_used`, causing a permanent socket credit leak and stream corruption.
--

commit 7c26a8d9c487a00892dd5ed3c4bb090956e769a6
Author: Stefano Garzarella <sgarzare@redhat.com>

vsock/virtio: collapse receive queue under memory pressure

This patch adds a mechanism to collapse small packets in the receive queue
into larger, compact SKBs when memory pressure prevents enqueueing new
packets. This reduces SKB overhead and avoids connection resets during large
transfers under backpressure.

Closes: https://lore.kernel.org/netdev/618701dd023e$063de350$12b9a9f0$@gmail.com/

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 09475007165b3..304ea424995d4 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
[ ... ]
> @@ -420,6 +420,137 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
[ ... ]
> +/* Called under lock_sock when skb overhead exceeds the budget. */
> +static void virtio_transport_collapse_rx_queue(struct virtio_vsock_sock *vvs)
> +{
[ ... ]
> +		/* Cannot fail since src_off/src_len are within bounds, but if
> +		 * it does, discard new_skb to avoid queuing corrupted data.
> +		 */
> +		if (WARN_ON_ONCE(skb_copy_bits(skb, src_off,
> +					       skb_put(new_skb, src_len),
> +					       src_len))) {
> +			kfree_skb(new_skb);
> +			new_skb = NULL;
> +			goto out;
> +		}
> +
> +		le32_add_cpu(&virtio_vsock_hdr(new_skb)->len, src_len);
> +		virtio_vsock_hdr(new_skb)->flags |= hdr->flags;
> +
> +next:
> +		__skb_unlink(skb, &vvs->rx_queue);
> +		if (keep)
> +			__skb_queue_tail(&new_queue, skb);
> +		else
> +			consume_skb(skb);
> +	}

[Severity: Medium]
If new_skb successfully absorbed data from a previous SKB in an earlier
iteration before skb_copy_bits fails on a subsequent SKB, does
kfree_skb(new_skb) drop that accumulated data without updating vvs->rx_bytes
and vvs->buf_used?

When consume_skb(skb) runs on those earlier iterations, the original SKBs are
freed. If this error path later frees new_skb, the previously merged payloads
are permanently lost, but the socket accounting still includes them. Will
this leak receive buffer credit and eventually cause the socket to stall?

> +out:
> +	if (new_skb)
> +		__skb_queue_tail(&new_queue, new_skb);
> +
> +	skb_queue_splice(&new_queue, &vvs->rx_queue);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626134823.206676-1-sgarzare@redhat.com?part=1