On Sun, Dec 14, 2025 at 06:38:22AM +0000, Melbin K Mathew wrote:
On 12/12/2025 12:26, Stefano Garzarella wrote:
On Fri, Dec 12, 2025 at 11:40:03AM +0000, Melbin K Mathew wrote:
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:
Why not just change the signature of virtio_transport_has_space()?
Thanks, that is cleaner.
For Patch 1 i'll change virtio_transport_has_space() to take
struct virtio_vsock_sock * and call it from both
virtio_transport_stream_has_space() and virtio_transport_get_credit().
/*
* Return available peer buffer space for TX (>= 0).
*
* Use s64 arithmetic so that if the peer shrinks peer_buf_alloc while
* we have bytes in flight (tx_cnt - peer_fwd_cnt), the subtraction does
* not underflow into a large positive value as it would with u32.
*
* Must be called with vvs->tx_lock held.
*/
static s64 virtio_transport_has_space(struct virtio_vsock_sock *vvs)
{
s64 bytes;
bytes = (s64)vvs->peer_buf_alloc -
((s64)vvs->tx_cnt - (s64)vvs->peer_fwd_cnt);
wait, why casting also the counters?
they are supposed to wrap, so should be fine to avoid the cast there.
Please, avoid too many changes in a single patch.
if (bytes < 0)
bytes = 0;
return bytes;
}
s64 virtio_transport_stream_has_space(struct vsock_sock *vsk)
{
struct virtio_vsock_sock *vvs = vsk->trans;
s64 bytes;
spin_lock_bh(&vvs->tx_lock);
bytes = virtio_transport_has_space(vvs);
spin_unlock_bh(&vvs->tx_lock);
return bytes;
}
u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit)
{
u32 ret;
if (!credit)
return 0;
spin_lock_bh(&vvs->tx_lock);
ret = min_t(u32, credit, (u32)virtio_transport_has_space(vvs));
min_t() is supposed to be use exactly to avoid to cast each member, so
why adding the cast to the value returned by
virtio_transport_has_space() ?
vvs->tx_cnt += ret;
vvs->bytes_unsent += ret;
spin_unlock_bh(&vvs->tx_lock);
return ret;
}
Does this look right?
Pretty much yes, a part some comments, but I'd like to see the final
solution.
Thanks,
Stefano
