netdev - Re: [PATCH 1/2] vsock/virtio: Move rx_buf_nr and rx_buf_max

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <yvdbqfahgu76eczt5c4n76akbhh4h2ofemd46kv6kia4xipeqr@tfucpayw7cqg>
Date: Mon, 10 Feb 2025 17:16:03 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Junnan Wu <junnan01.wu@...sung.com>
Cc: stefanha@...hat.com, kvm@...r.kernel.org, 
	virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org, mindong.zhao@...sung.com, 
	q1.huang@...sung.com, ying01.gao@...sung.com, ying123.xu@...sung.com
Subject: Re: [PATCH 1/2] vsock/virtio: Move rx_buf_nr and rx_buf_max_nr
 initialization position

On Fri, Feb 07, 2025 at 01:20:32PM +0800, Junnan Wu wrote:
>From: Ying Gao <ying01.gao@...sung.com>
>
>In function virtio_vsock_probe, it initializes the variables
>"rx_buf_nr" and "rx_buf_max_nr",
>but in function virtio_vsock_restore it doesn't.
>
>Move the initizalition position into function virtio_vsock_vqs_start.
>
>Once executing s2r twice in a row without

s2r ? suspend 2 ram?

Please define the acronym, it was hard for me to understand (the code 
helped me).

>initializing rx_buf_nr and rx_buf_max_nr,
>the rx_buf_max_nr increased to three times vq->num_free,
>at this time, in function virtio_transport_rx_work,
>the conditions to fill rx buffer
>(rx_buf_nr < rx_buf_max_nr / 2) can't be met.
>

Please add a Fixes tag, in this case I think it should be:

Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume")

but please, double check.

>Signed-off-by: Ying Gao <ying01.gao@...sung.com>
>Signed-off-by: Junnan Wu <junnan01.wu@...sung.com>
>---
> net/vmw_vsock/virtio_transport.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

I find the commit title/description a bit hard to understand, please 
take a look at: 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

In this case I'd write something like this:

   vsock/virtio: initialize rx_buf_nr and rx_buf_max_nr when resuming

   [Describe the symptom]
   When executing suspend/resume twice in a row, ...

   [Describe the issue]
   `rx_buf_nr` and `rx_buf_max_nr` are initialized only in
   virtio_vsock_probe(), but they should be reset whenever virtqueues
   are recreated, like after a suspend/resume. ...

   [Desribe the fix, what this patch does]
   Move the `rx_buf_nr` and `rx_buf_max_nr` initialization in
   virtio_vsock_vqs_init(), so we are sure that they are properly
   initialized, every time we initialize the virtqueues, either when we
   load the driver or after a suspend/resume. ...

>
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index b58c3818f284..9eefd0fba92b 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -688,6 +688,8 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock)

I think it is better to move the initialization of those fields in 
virtio_vsock_vqs_init().

> 	mutex_unlock(&vsock->tx_lock);
>
> 	mutex_lock(&vsock->rx_lock);
>+	vsock->rx_buf_nr = 0;
>+	vsock->rx_buf_max_nr = 0;
> 	virtio_vsock_rx_fill(vsock);
> 	vsock->rx_run = true;
> 	mutex_unlock(&vsock->rx_lock);
>@@ -779,8 +781,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
>
> 	vsock->vdev = vdev;
>
>-	vsock->rx_buf_nr = 0;
>-	vsock->rx_buf_max_nr = 0;
> 	atomic_set(&vsock->queued_replies, 0);

Should we also move `queued_replies` ?

Thanks,
Stefano

>
> 	mutex_init(&vsock->tx_lock);
>-- 
>2.34.1
>
>