On Tue, May 12, 2026 at 10:07:37AM +0200, Stefano Garzarella wrote: > From: Stefano Garzarella <[email protected]> > > After commit 059b7dbd20a6 ("vsock/virtio: fix potential unbounded skb > queue"), virtio_transport_inc_rx_pkt() subtracts per-skb overhead from > buf_alloc when checking whether a new packet fits. This reduces the > effective receive buffer below what the user configured via > SO_VM_SOCKETS_BUFFER_SIZE, causing legitimate data packets to be > silently dropped and applications that rely on the full buffer size > to deadlock. > > Also, the reduced space is not communicated to the remote peer, so > its credit calculation accounts more credit than the receiver will > actually accept, causing data loss (there is no retransmission). > > With this approach we currently have failures in > tools/testing/vsock/vsock_test.c. Test 18 sometimes fails, while > test 22 always fails in this way: > 18 - SOCK_STREAM MSG_ZEROCOPY...hash mismatch > > 22 - SOCK_STREAM virtio credit update + SO_RCVLOWAT...send failed: > Resource temporarily unavailable > > Fix this by using `buf_alloc * 2` as the total budget for payload plus > skb overhead in virtio_transport_inc_rx_pkt(), similar to how SO_RCVBUF > is doubled to reserve space for sk_buff metadata. This preserves the > full buf_alloc for payload under normal operation, while still bounding > the skb queue growth. > > When the total budget (buf_alloc * 2) is exceeded (e.g. under small-packet > flooding where overhead dominates), the connection is reset and local > socket error set to ENOBUFS, so both peers are explicitly notified of > the failure rather than silently losing data. > > With this patch, all tests in tools/testing/vsock/vsock_test.c are > now passing again. > > A solution to handle small-packet overhead efficiently also for > SEQPACKET (we already do that for STREAM) is planned as follow-up work. > This patch is needed in any case to prevent silent data loss, because > even if we reduce the overhead, we can't eliminate it entirely. > > Fixes: 059b7dbd20a6 ("vsock/virtio: fix potential unbounded skb queue") > Signed-off-by: Stefano Garzarella <[email protected]>
Thanks for the patch! I'd like to split this: 1. buf alloc boost 2. reset when out of credits this way we can revert 2 easier later. > --- > v2: > - Close the connection when we can no longer queue new packets instead > of losing data. > - No longer announce the reduced buf_alloc to avoid violating the > spec. [MST] > > v1: https://lore.kernel.org/netdev/[email protected]/ > --- > net/vmw_vsock/virtio_transport_common.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/net/vmw_vsock/virtio_transport_common.c > b/net/vmw_vsock/virtio_transport_common.c > index 9b8014516f4f..f23bf8a11319 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -449,7 +449,10 @@ static bool virtio_transport_inc_rx_pkt(struct > virtio_vsock_sock *vvs, > { > u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * > SKB_TRUESIZE(0); > > - if (skb_overhead + vvs->buf_used + len > vvs->buf_alloc) > + /* Use buf_alloc * 2 as total budget (payload + overhead), similar to > + * how SO_RCVBUF is doubled to reserve space for sk_buff metadata. > + */ > + if (skb_overhead + vvs->buf_used + len > (u64)vvs->buf_alloc * 2) > return false; > > vvs->rx_bytes += len; > @@ -1365,7 +1368,7 @@ virtio_transport_recv_connecting(struct sock *sk, > return err; > } > > -static void > +static bool > virtio_transport_recv_enqueue(struct vsock_sock *vsk, > struct sk_buff *skb) > { > @@ -1380,10 +1383,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk, > spin_lock_bh(&vvs->rx_lock); > > can_enqueue = virtio_transport_inc_rx_pkt(vvs, len); > - if (!can_enqueue) { > - free_pkt = true; > + if (!can_enqueue) > goto out; > - } > > if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SEQ_EOM) > vvs->msg_count++; > @@ -1423,6 +1424,8 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk, > spin_unlock_bh(&vvs->rx_lock); > if (free_pkt) > kfree_skb(skb); > + > + return can_enqueue; > } > > static int > @@ -1435,7 +1438,16 @@ virtio_transport_recv_connected(struct sock *sk, > > switch (le16_to_cpu(hdr->op)) { > case VIRTIO_VSOCK_OP_RW: > - virtio_transport_recv_enqueue(vsk, skb); > + if (!virtio_transport_recv_enqueue(vsk, skb)) { > + /* There is no more space to queue the packet, so let's > + * close the connection; otherwise, we'll lose data. > + */ > + (void)virtio_transport_reset(vsk, skb); > + sk->sk_state = TCP_CLOSE; > + sk->sk_err = ENOBUFS; > + sk_error_report(sk); > + break; > + } > vsock_data_ready(sk); > return err; > case VIRTIO_VSOCK_OP_CREDIT_REQUEST: > -- > 2.54.0
