Re: [PATCH RFC 7/8] vsock: preserve host-to-guest datagram boundaries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vsock/virtio: uncondensed 64 KiB rx skbs exhaust sk_rcvbuf on small packets
--

commit 7675c425557daffdbb774353bcdf90cea6ee4417
Author: Sergio Lopez <slp@redhat.com>

vsock: preserve host-to-guest datagram boundaries

This patch preserves host-to-guest datagram boundaries by dropping
oversized datagrams in vhost and allocating full-size paged RX buffers
in virtio_transport when datagrams are negotiated.

> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 14ce0836bc395..d256274088b38 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c

[ ... ]

> @@ -312,22 +313,31 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
>  static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
>  {
>  	int total_len = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
> -	struct scatterlist pkt, *p;
>  	struct virtqueue *vq;
>  	struct sk_buff *skb;
>  	int ret;
>  
>  	vq = vsock->vqs[VSOCK_VQ_RX];
> +	/* A datagram must fit in one RX buffer, including its header. */
> +	if (vsock->dgram_allow)
> +		total_len = VIRTIO_VSOCK_SKB_HEADROOM +
> +			    VIRTIO_VSOCK_MAX_PKT_BUF_SIZE;
>  
>  	do {
> -		skb = virtio_vsock_alloc_linear_skb(total_len, GFP_KERNEL);
> +		skb = virtio_vsock_alloc_skb(total_len, GFP_KERNEL);
>  		if (!skb)
>  			break;
>  
>  		memset(skb->head, 0, VIRTIO_VSOCK_SKB_HEADROOM);
> -		sg_init_one(&pkt, virtio_vsock_hdr(skb), total_len);
> -		p = &pkt;
> -		ret = virtqueue_add_sgs(vq, &p, 0, 1, skb, GFP_KERNEL);
> +		/* Post the header and the entire payload as one RX buffer. */
> +		virtio_vsock_skb_put(skb, total_len - VIRTIO_VSOCK_SKB_HEADROOM);
> +		skb_push(skb, VIRTIO_VSOCK_SKB_HEADROOM);
> +		sg_init_table(vsock->in_bufs, ARRAY_SIZE(vsock->in_bufs));
> +		ret = skb_to_sgvec(skb, vsock->in_bufs, 0, total_len);
> +		skb_pull(skb, VIRTIO_VSOCK_SKB_HEADROOM);
> +		if (ret >= 0)
> +			ret = virtqueue_add_inbuf(vq, vsock->in_bufs, ret, skb,
> +						  GFP_KERNEL);
>  		if (ret < 0) {
>  			kfree_skb(skb);
>  			break;
> @@ -698,7 +708,7 @@ static void virtio_transport_rx_work(struct work_struct *work)
>  
>  			/* Drop short/long packets */
>  			if (unlikely(len < sizeof(*hdr) ||
> -				     len > virtio_vsock_skb_len(skb))) {
> +				     len > sizeof(*hdr) + skb->len)) {
>  				kfree_skb(skb);
>  				continue;
>  			}
> @@ -710,8 +720,7 @@ static void virtio_transport_rx_work(struct work_struct *work)
>  				continue;
>  			}
>  
> -			if (payload_len)
> -				virtio_vsock_skb_put(skb, payload_len);
> +			pskb_trim_unique(skb, payload_len);

[Severity: Medium]
Can allocating 64 KiB nonlinear RX buffers here leave skb->truesize at ~66 KiB
after pskb_trim_unique() and prematurely exhaust sk->sk_rcvbuf on small
packets?

When vsock->dgram_allow is true, virtio_vsock_rx_fill() calls
virtio_vsock_alloc_skb() with total_len = VIRTIO_VSOCK_SKB_HEADROOM +
VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (44 + 65536 bytes):

include/linux/virtio_vsock.h:virtio_vsock_alloc_skb() {
	if (size <= SKB_WITH_OVERHEAD(PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
		return virtio_vsock_alloc_linear_skb(size, mask);

	size -= VIRTIO_VSOCK_SKB_HEADROOM;
	return __virtio_vsock_alloc_skb_with_frags(VIRTIO_VSOCK_SKB_HEADROOM,
						   size, mask);
}

On 4 KiB and 16 KiB page systems, this calls alloc_skb_with_frags() with a
44-byte linear header request and 65536 bytes in page fragments. Because
skb->head comes from skb_small_head_cache, reserving 44 bytes leaves only
about 276 bytes of linear tailroom (skb->end - skb->tail), while all 64 KiB of
payload capacity is placed in page fragments with skb->truesize set to ~66 KiB.

When virtio_transport_rx_work() receives a packet and calls
pskb_trim_unique(skb, payload_len) -> pskb_trim() -> ___pskb_trim(), trailing
unused fragments are unreferenced without reducing skb->truesize, and
skb_condense() is called:

net/core/skbuff.c:skb_condense() {
	if (skb->data_len) {
		if (skb->data_len > skb->end - skb->tail ||
		    skb_cloned(skb) || !skb_frags_readable(skb))
			return;

		/* Nice, we can free page frag(s) right now */
		__pskb_pull_tail(skb, skb->data_len);
	}
	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
}

Whenever payload_len exceeds skb->end - skb->tail (~276 bytes), skb_condense()
returns early without pulling data into skb->head, freeing the first 32 KiB
page fragment, or updating skb->truesize.

In virtio_transport_recv_pkt(), sock_queue_rcv_skb(sk, skb) then charges the
full ~66 KiB skb->truesize against sk->sk_rcvbuf (212992 bytes by default), so
queuing just 4 datagrams of ~300 bytes will fill sk_rcvbuf and drop subsequent
datagrams, while also pinning 32 KiB page fragments per packet across
SOCK_DGRAM, SOCK_STREAM, and SOCK_SEQPACKET receive queues sharing the RX
virtqueue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-vsock-dgram-support-v1-0-e457cf7cee78@sinrega.org?part=7