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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <wssxyvbgq3a3icydzxsbj5bliqd67xreffaqqusfia2suxrjdk@gcke3jemvycx>
Date: Mon, 15 Dec 2025 15:15:22 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Melbin K Mathew <mlbnkm1@...il.com>
Cc: "Michael S. Tsirkin" <mst@...hat.com>, stefanha@...hat.com, 
	kvm@...r.kernel.org, netdev@...r.kernel.org, virtualization@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, 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 net v3] vsock/virtio: cap TX credit to local buffer size

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