On Wed, May 06, 2026 at 12:50:04PM +0300, Arseniy Krasnov wrote:
05.05.2026 19:37, Bobby Eshleman wrote:
On Tue, May 05, 2026 at 06:11:13PM +0200, Stefano Garzarella wrote:
On Tue, May 05, 2026 at 07:14:36AM -0700, Eric Dumazet wrote:
On Tue, May 5, 2026 at 6:52 AM Stefano Garzarella <[email protected]> wrote:
On Thu, Apr 30, 2026 at 12:26:52PM +0000, Eric Dumazet wrote:
virtio_transport_inc_rx_pkt() checks vvs->rx_bytes + len > vvs->buf_alloc.
virtio_transport_recv_enqueue() skips coalescing for packets
with VIRTIO_VSOCK_SEQ_EOM.
If fed with packets with len == 0 and VIRTIO_VSOCK_SEQ_EOM,
a very large number of packets can be queued
because vvs->rx_bytes stays at 0.
Fix this by estimating the skb metadata size:
(Number of skbs in the queue) * SKB_TRUESIZE(0)
Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
Signed-off-by: Eric Dumazet <[email protected]>
Cc: Arseniy Krasnov <[email protected]>
Cc: Stefan Hajnoczi <[email protected]>
Cc: Stefano Garzarella <[email protected]>
Cc: "Michael S. Tsirkin" <[email protected]>
Cc: Jason Wang <[email protected]>
Cc: Xuan Zhuo <[email protected]>
Cc: "Eugenio Pérez" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/vmw_vsock/virtio_transport_common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c
b/net/vmw_vsock/virtio_transport_common.c
index
416d533f493d7b07e9c77c43f741d28cfcd0953e..9b8014516f4fb1130ae184635fbba4dfee58bd64
100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -447,7 +447,9 @@ static int virtio_transport_send_pkt_info(struct vsock_sock
*vsk,
static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
u32 len)
{
- if (vvs->buf_used + len > vvs->buf_alloc)
+ u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * SKB_TRUESIZE(0);
+
+ if (skb_overhead + vvs->buf_used + len > vvs->buf_alloc)
return false;
I'm not sure about this fix, I mean that maybe this is incomplete.
In virtio-vsock, there is a credit mechanism between the two peers:
https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-4850003
This takes only the payload into account, so it’s true that this problem
exists; however, perhaps we should also inform the other peer of a lower
credit balance, otherwise the other peer will believe it has much more
credit than it actually does, send a large payload, and then the packet
will be discarded and the data lost (there are no retransmissions,
etc.).
I dunno, perhaps revert 077706165717 ("virtio/vsock: don't use skbuff
state to account credit")
and find a better fix then?
IIRC the same issue was there before the commit fixed by that one (commit
71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")), so
not sure about reverting it TBH.
CCing Arseniy and Bobby.
Thanks!
There is always a discrepancy between skb->len and skb->truesize.
You will not be able to announce a 1MB window, and accept one milliion
skb of 1-byte each.
This kind of contract is broken.
Yep, I agree, but before we start discarding data (and losing it), IMHO we
should at least inform the other peer that we're out of space.
@Stefan, @Michael, do you think we can do something in the spec to avoid
this issue and in some way take into account also the metadata in the
credit. I mean to avoid the 1-byte packets flooding.
Thanks,
Stefano
Indeed the old pre-fix skb code would have the same issue.
I can't think of any way around this without extending the spec.
Hi, thanks, agree with Bobby, that accounting metadata (e.g. skb size here) was not
implemented "by
design" in credit logic - another side of data exchange knows nothing about
that. Also the same
situation was before skb implementation was added by Bobby. So looks like need
to update spec may be.
Even if we change the specifications, we still need to work with older
devices, so we should find a solution for this as well.
My main concern is data loss, so I'm considering the following options:
1. Notify the other peer of a smaller buf_alloc from the start, leave
some room for overhead, and when it's running out, notify them that
buf_alloc = 0. This way, the peer realizes it can’t send anything else.
2. Or update buf_alloc each time by removing the overhead, similar to
what’s currently done in virtio_transport_inc_rx_pkt(), but also do it
in virtio_transport_inc_tx_pkt().
As I said, IMO this patch alone is incomplete; we need to communicate
with the peer somehow regarding space. I don’t think including the
overhead in fwd_cnt is spec compliant, since the other peer has no idea
how much overhead is needed, but reducing buf_alloc should be okay, even
though I’m concerned about packets in flight.
As a quick fix, I think option 2 might be the easiest; I’ll run some
tests and send over a patch.
But in the long run, I think we absolutely need to improve memory
management in vsock, perhaps by avoiding custom solutions.
Thanks,
Stefano
