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 <[email protected]> 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/[email protected]?part=7
