Message-ID: <CAGxU2F6RkU-6id5Z9wBFKPfmws9CJ00mnBQgCYZasLshLnYn=w@mail.gmail.com>
Date: Thu, 11 Dec 2025 13:55:12 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Melbin K Mathew <mlbnkm1@...il.com>
Cc: stefanha@...hat.com, kvm@...r.kernel.org, netdev@...r.kernel.org, 
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org, mst@...hat.com, 
	jasowang@...hat.com, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org
Subject: Re: [PATCH] vsock/virtio: cap TX credit to local buffer size

On Thu, 11 Dec 2025 at 11:08, Stefano Garzarella <sgarzare@...hat.com> wrote:
>
> On Thu, 11 Dec 2025 at 10:10, Stefano Garzarella <sgarzare@...hat.com> wrote:
> >
> > On Wed, Dec 10, 2025 at 04:00:19PM +0100, Melbin K Mathew wrote:
> > >The virtio vsock transport currently derives its TX credit directly
> > >from peer_buf_alloc, which is set from the remote endpoint's
> > >SO_VM_SOCKETS_BUFFER_SIZE value.
> >
> > Why removing the target tree [net] from the tags?
> >
> > Also this is a v2, so the tags should have been [PATCH net v2], please
> > check it in next versions, more info:
> >
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#subject-line
> >
> > >
> > >On the host side this means that the amount of data we are willing to
> > >queue for a connection is scaled by a guest-chosen buffer size,
> > >rather than the host's own vsock configuration. A malicious guest can
> > >advertise a large buffer and read slowly, causing the host to allocate
> > >a correspondingly large amount of sk_buff memory.
> > >
> > >Introduce a small helper, virtio_transport_peer_buf_alloc(), that
> > >returns min(peer_buf_alloc, buf_alloc), and use it wherever we consume
> > >peer_buf_alloc:
> > >
> > >  - virtio_transport_get_credit()
> > >  - virtio_transport_has_space()
> > >  - virtio_transport_seqpacket_enqueue()
> > >
> > >This ensures the effective TX window is bounded by both the peer's
> > >advertised buffer and our own buf_alloc (already clamped to
> > >buffer_max_size via SO_VM_SOCKETS_BUFFER_MAX_SIZE), so a remote guest
> > >cannot force the host to queue more data than allowed by the host's
> > >own vsock settings.
> > >
> > >On an unpatched Ubuntu 22.04 host (~64 GiB RAM), running a PoC with
> > >32 guest vsock connections advertising 2 GiB each and reading slowly
> > >drove Slab/SUnreclaim from ~0.5 GiB to ~57 GiB and the system only
> > >recovered after killing the QEMU process.
> > >
> > >With this patch applied, rerunning the same PoC yields:
> > >
> > >  Before:
> > >    MemFree:        ~61.6 GiB
> > >    MemAvailable:   ~62.3 GiB
> > >    Slab:           ~142 MiB
> > >    SUnreclaim:     ~117 MiB
> > >
> > >  After 32 high-credit connections:
> > >    MemFree:        ~61.5 GiB
> > >    MemAvailable:   ~62.3 GiB
> > >    Slab:           ~178 MiB
> > >    SUnreclaim:     ~152 MiB
> > >
> > >i.e. only ~35 MiB increase in Slab/SUnreclaim, no host OOM, and the
> > >guest remains responsive.
> >
> > I think we should include here a summary of what you replied to Michael
> > about other transports.
> >
> > I can't find your reply in the archive, but I mean the reply to
> > https://lore.kernel.org/netdev/20251210084318-mutt-send-email-mst@kernel.org/
> >
> > >
> > >Fixes: 06a8fc78367d ("VSOCK: Introduce virtio_vsock_common.ko")
> > >Suggested-by: Stefano Garzarella <sgarzare@...hat.com>
> > >Signed-off-by: Melbin K Mathew <mlbnkm1@...il.com>
> > >---
> > > net/vmw_vsock/virtio_transport_common.c | 27 ++++++++++++++++++++++---
> > > 1 file changed, 24 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > >index dcc8a1d58..02eeb96dd 100644
> > >--- a/net/vmw_vsock/virtio_transport_common.c
> > >+++ b/net/vmw_vsock/virtio_transport_common.c
> > >@@ -491,6 +491,25 @@ void virtio_transport_consume_skb_sent(struct sk_buff *skb, bool consume)
> > > }
> > > EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent);
> > >
> > >+/*
> > >+ * Return the effective peer buffer size for TX credit computation.
> >
> > nit: block comment in this file doesn't leave empty line, so I'd follow
> > it:
> >
> > @@ -491,8 +491,7 @@ void virtio_transport_consume_skb_sent(struct sk_buff *skb, bool consume)
> >   }
> >   EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent);
> >
> > -/*
> > - * Return the effective peer buffer size for TX credit computation.
> > +/* Return the effective peer buffer size for TX credit computation.
> >    *
> >    * The peer advertises its receive buffer via peer_buf_alloc, but we
> >    * cap that to our local buf_alloc (derived from
> >
> > >+ *
> > >+ * The peer advertises its receive buffer via peer_buf_alloc, but we
> > >+ * cap that to our local buf_alloc (derived from
> > >+ * SO_VM_SOCKETS_BUFFER_SIZE and already clamped to buffer_max_size)
> > >+ * so that a remote endpoint cannot force us to queue more data than
> > >+ * our own configuration allows.
> > >+ */
> > >+static u32 virtio_transport_tx_buf_alloc(struct virtio_vsock_sock *vvs)
> > >+{
> > >+      u32 peer  = vvs->peer_buf_alloc;
> > >+      u32 local = vvs->buf_alloc;
> > >+
> > >+      if (peer > local)
> > >+              return local;
> > >+      return peer;
> > >+}
> > >+
> >
> > I think here Michael was suggesting this:
> >
> > @@ -502,12 +502,7 @@ EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent);
> >    */
> >   static u32 virtio_transport_tx_buf_alloc(struct virtio_vsock_sock *vvs)
> >   {
> > -       u32 peer  = vvs->peer_buf_alloc;
> > -       u32 local = vvs->buf_alloc;
> > -
> > -       if (peer > local)
> > -               return local;
> > -       return peer;
> > +       return min(vvs->peer_buf_alloc, vvs->buf_alloc);
> >   }
> >
> >
> > > u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit)
> > > {
> > >       u32 ret;
> > >@@ -499,7 +518,8 @@ u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit)
> > >               return 0;
> > >
> > >       spin_lock_bh(&vvs->tx_lock);
> > >-      ret = vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt);
> > >+      ret = virtio_transport_tx_buf_alloc(vvs) -
> > >+            (vvs->tx_cnt - vvs->peer_fwd_cnt);
> > >       if (ret > credit)
> > >               ret = credit;
> > >       vvs->tx_cnt += ret;
> > >@@ -831,7 +851,7 @@ virtio_transport_seqpacket_enqueue(struct vsock_sock *vsk,
> > >
> > >       spin_lock_bh(&vvs->tx_lock);
> > >
> > >-      if (len > vvs->peer_buf_alloc) {
> > >+      if (len > virtio_transport_tx_buf_alloc(vvs)) {
> > >               spin_unlock_bh(&vvs->tx_lock);
> > >               return -EMSGSIZE;
> > >       }
> > >@@ -882,7 +902,8 @@ static s64 virtio_transport_has_space(struct vsock_sock *vsk)
> > >       struct virtio_vsock_sock *vvs = vsk->trans;
> > >       s64 bytes;
> > >
> > >-      bytes = (s64)vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt);
> > >+      bytes = (s64)virtio_transport_tx_buf_alloc(vvs) -
> > >+            (vvs->tx_cnt - vvs->peer_fwd_cnt);
> >
> > nit: please align this:
> >
> > @@ -903,7 +898,7 @@ static s64 virtio_transport_has_space(struct vsock_sock *vsk)
> >          s64 bytes;
> >
> >          bytes = (s64)virtio_transport_tx_buf_alloc(vvs) -
> > -             (vvs->tx_cnt - vvs->peer_fwd_cnt);
> > +               (vvs->tx_cnt - vvs->peer_fwd_cnt);
> >          if (bytes < 0)
> >                  bytes = 0;
> >
> >
> > Just minor things, but the patch LGTM, thanks!
>
> I just noticed that vsock_test are now failing because one peer (client)
> try to send more than TX buffer while the RX is waiting for the whole
> data.
>
> This should fix the test:
>
> From b69ca1fd3d544345b02cedfbeb362493950a87c1 Mon Sep 17 00:00:00 2001
> From: Stefano Garzarella <sgarzare@...hat.com>
> Date: Thu, 11 Dec 2025 10:55:06 +0100
> Subject: [PATCH 1/1] vsock/test: fix seqpacket message bounds test
>
> From: Stefano Garzarella <sgarzare@...hat.com>
>
> The test requires the sender (client) to send all messages before waking
> up the receiver (server).
> Since virtio-vsock had a bug and did not respect the size of the TX
> buffer, this test worked, but now that we have fixed the bug, it hangs
> because the sender fills the TX buffer before waking up the receiver.
>
> Set the buffer size in the sender (client) as well, as we already do for
> the receiver (server).
>
> Fixes: 5c338112e48a ("test/vsock: rework message bounds test")
> Signed-off-by: Stefano Garzarella <sgarzare@...hat.com>
> ---
>  tools/testing/vsock/vsock_test.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
> index 9e1250790f33..af6665ed19d5 100644
> --- a/tools/testing/vsock/vsock_test.c
> +++ b/tools/testing/vsock/vsock_test.c
> @@ -351,6 +351,7 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
>
>  static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>  {
> +       unsigned long long sock_buf_size;
>         unsigned long curr_hash;
>         size_t max_msg_size;
>         int page_size;
> @@ -363,6 +364,16 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>                 exit(EXIT_FAILURE);
>         }
>
> +       sock_buf_size = SOCK_BUF_SIZE;
> +
> +       setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
> +                           sock_buf_size,
> +                           "setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)");
> +
> +       setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
> +                           sock_buf_size,
> +                           "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
> +
>         /* Wait, until receiver sets buffer size. */
>         control_expectln("SRVREADY");
>
> --
> 2.52.0
>
> Please add that patch to a series (e.g. v3) which includes your patch,
> and that fix for the test.

I saw you sent v3 without this, never mind, I'll post it directly.

Stefano

>
> Maybe we can also add a new test to check exactly the problem you're
> fixing, to avoid regressions.
>
> Thanks,
> Stefano

Powered by blists - more mailing lists