Re: [PATCH net v3] vsock/virtio: cap TX credit to local buffer size

[Melbin K Mathew <mlbnkm1@gmail.com>]

On 12/12/2025 10:40, Stefano Garzarella wrote:
> On Fri, Dec 12, 2025 at 09:56:28AM +0000, Melbin Mathew Antony wrote:
>> Hi Stefano, Michael,
>>
>> Thanks for the suggestions and guidance.
> 
> You're welcome, but please avoid top-posting in the future:
> https://www.kernel.org/doc/html/latest/process/submitting- 
> patches.html#use-trimmed-interleaved-replies-in-email-discussions
> 
Sure. Thanks
>>
>> I’ve drafted a 4-part series based on the recap. I’ve included the
>> four diffs below for discussion. Can wait for comments, iterate, and
>> then send the patch series in a few days.
>>
>> ---
>>
>> Patch 1/4 — vsock/virtio: make get_credit() s64-safe and clamp negatives
>>
>> virtio_transport_get_credit() was doing unsigned arithmetic; if the
>> peer shrinks its window, the subtraction can underflow and look like
>> “lots of credit”. This makes it compute “space” in s64 and clamp < 0
>> to 0.
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c
>> b/net/vmw_vsock/virtio_transport_common.c
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -494,16 +494,23 @@ 
>> EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent);
>> u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 
>> credit)
>> {
>> + s64 bytes;
>>  u32 ret;
>>
>>  if (!credit)
>>  return 0;
>>
>>  spin_lock_bh(&vvs->tx_lock);
>> - ret = vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt);
>> - if (ret > credit)
>> - ret = credit;
>> + bytes = (s64)vvs->peer_buf_alloc -
> 
> Why not just calling virtio_transport_has_space()?
virtio_transport_has_space() takes struct vsock_sock *, while 
virtio_transport_get_credit() takes struct virtio_vsock_sock *, so I 
cannot directly call has_space() from get_credit() without changing 
signatures.

Would you be OK if I factor the common “space” calculation into a small 
helper that operates on struct virtio_vsock_sock * and is used by both 
paths? Something like:

/* Must be called with vvs->tx_lock held. Returns >= 0. */
static s64 virtio_transport_tx_space(struct virtio_vsock_sock *vvs)
{
	s64 bytes;

	bytes = (s64)vvs->peer_buf_alloc -
		((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt);
	if (bytes < 0)
		bytes = 0;

	return bytes;
}

Then:

get_credit() would do bytes = virtio_transport_tx_space(vvs); ret = 
min_t(u32, credit, (u32)bytes);

has_space() would use the same helper after obtaining vvs = vsk->trans;

Does that match what you had in mind, or would you prefer a different 
factoring?

> 
>> + ((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt);
>> + if (bytes < 0)
>> + bytes = 0;
>> +
>> + ret = min_t(u32, credit, (u32)bytes);
>>  vvs->tx_cnt += ret;
>>  vvs->bytes_unsent += ret;
>>  spin_unlock_bh(&vvs->tx_lock);
>>
>>  return ret;
>> }
>>
>>
>> ---
>>
>> Patch 2/4 — vsock/virtio: cap TX window by local buffer (helper + use
>> everywhere in TX path)
>>
>> Cap the effective advertised window to min(peer_buf_alloc, buf_alloc)
>> and use it consistently in TX paths (get_credit, has_space,
>> seqpacket_enqueue).
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c
>> b/net/vmw_vsock/virtio_transport_common.c
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -491,6 +491,16 @@ 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.
>> + *
>> + * The peer advertises its receive buffer via peer_buf_alloc, but we 
>> cap it
>> + * to our local buf_alloc (derived from SO_VM_SOCKETS_BUFFER_SIZE and
>> + * already clamped to buffer_max_size).
>> + */
>> +static u32 virtio_transport_tx_buf_alloc(struct virtio_vsock_sock *vvs)
>> +{
>> + return min(vvs->peer_buf_alloc, vvs->buf_alloc);
>> +}
>>
>> u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 
>> credit)
>> {
>>  s64 bytes;
>> @@ -502,7 +512,8 @@ u32 virtio_transport_get_credit(struct
>> virtio_vsock_sock *vvs, u32 credit)
>>  return 0;
>>
>>  spin_lock_bh(&vvs->tx_lock);
>> - bytes = (s64)vvs->peer_buf_alloc -
>> + bytes = (s64)virtio_transport_tx_buf_alloc(vvs) -
>>  ((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt);
>>  if (bytes < 0)
>>  bytes = 0;
>> @@ -834,7 +845,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;
>>  }
>> @@ -884,7 +895,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) -
>> + ((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt);
>>  if (bytes < 0)
>>  bytes = 0;
>>
>>  return bytes;
>> }
>>
>>
>> ---
>>
>> Patch 3/4 — vsock/test: fix seqpacket msg bounds test (set client buf 
>> too)
> 
> Please just include in the series the patch I sent to you.
> 
Thanks. I'll use your vsock_test.c patch as-is for 3/4
>>
>> After fixing TX credit bounds, the client can fill its TX window and
>> block before it wakes the server. Setting the buffer on the client
>> makes the test deterministic again.
>>
>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/ 
>> vsock_test.c
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -353,6 +353,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;
>> @@ -366,6 +367,18 @@ 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");
>>
>>
>> ---
>>
>> Patch 4/4 — vsock/test: add stream TX credit bounds regression test
>>
>> This directly guards the original failure mode for stream sockets: if
>> the peer advertises a large window but the sender’s local policy is
>> small, the sender must stall quickly (hit EAGAIN in nonblocking mode)
>> rather than queueing megabytes.
> 
> Yeah, using nonblocking mode LGTM!
> 
>>
>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/ 
>> vsock_test.c
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -349,6 +349,7 @@
>> #define SOCK_BUF_SIZE (2 * 1024 * 1024)
>> +#define SMALL_SOCK_BUF_SIZE (64 * 1024ULL)
>> #define MAX_MSG_PAGES 4
>>
>> /* Insert new test functions after test_stream_msg_peek_server, before
>>  * test_seqpacket_msg_bounds_client (around line 352) */
>>
>> +static void test_stream_tx_credit_bounds_client(const struct 
>> test_opts *opts)
>> +{
>> + ... /* full function as provided */
>> +}
>> +
>> +static void test_stream_tx_credit_bounds_server(const struct 
>> test_opts *opts)
>> +{
>> + ... /* full function as provided */
>> +}
>>
>> @@ -2224,6 +2305,10 @@
>>  .run_client = test_stream_msg_peek_client,
>>  .run_server = test_stream_msg_peek_server,
>>  },
>> + {
>> + .name = "SOCK_STREAM TX credit bounds",
>> + .run_client = test_stream_tx_credit_bounds_client,
>> + .run_server = test_stream_tx_credit_bounds_server,
>> + },
> 
> Please put it at the bottom. Tests are skipped by index, so we don't 
> want to change index of old tests.
> 
> Please fix your editor, those diffs are hard to read without tabs/spaces.
seems like some issue with my email client. Hope it is okay now
> 
> Thanks,
> Stefano
>