Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs
From: Stefano Garzarella
Date: Mon Oct 23 2023 - 12:11:37 EST
On Mon, Oct 23, 2023 at 06:36:21PM +0300, Alexandru Matei wrote:
On 10/23/2023 6:13 PM, Stefano Garzarella wrote:
On Mon, Oct 23, 2023 at 05:59:45PM +0300, Alexandru Matei wrote:
On 10/23/2023 5:52 PM, Alexandru Matei wrote:
On 10/23/2023 5:29 PM, Stefano Garzarella wrote:
On Mon, Oct 23, 2023 at 05:08:33PM +0300, Alexandru Matei wrote:
Once VQs are filled with empty buffers and we kick the host,
it can send connection requests. If 'the_virtio_vsock' is not
initialized before, replies are silently dropped and do not reach the host.
Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
Signed-off-by: Alexandru Matei <alexandru.matei@xxxxxxxxxx>
---
v2:
- split virtio_vsock_vqs_init in vqs_init and vqs_fill and moved
the_virtio_vsock initialization after vqs_init
net/vmw_vsock/virtio_transport.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index e95df847176b..92738d1697c1 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -559,6 +559,11 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock)
vsock->tx_run = true;
mutex_unlock(&vsock->tx_lock);
+ return 0;
+}
+
+static void virtio_vsock_vqs_fill(struct virtio_vsock *vsock)
What about renaming this function in virtio_vsock_vqs_start() and move also the setting of `tx_run` here?
It works but in this case we also need to move rcu_assign_pointer in virtio_vsock_vqs_start(),
the assignment needs to be right after setting tx_run to true and before filling the VQs.
Why?
If `rx_run` is false, we shouldn't need to send replies to the host IIUC.
If we need this instead, please add a comment in the code, but also in the commit, because it's not clear why.
We need rcu_assign_pointer after setting tx_run to true for connections
that are initiated from the guest -> host.
virtio_transport_connect() calls virtio_transport_send_pkt(). Once
'the_virtio_vsock' is initialized, virtio_transport_send_pkt() will
queue the packet,
but virtio_transport_send_pkt_work() will exit if tx_run is false.
Okay, but in this case we could safely queue &vsock->send_pkt_work after
finishing initialization to send those packets queued earlier.
In the meantime I'll try to see if we can leave the initialization of
`the_virtio_vsock` as the ulitmate step and maybe go out first in the
workers if it's not set.
That way just queue all the workers after everything is done and we
should be fine.
And if we move rcu_assign_pointer then there is no need to split the function in two,
We can move rcu_assign_pointer() directly inside virtio_vsock_vqs_init() after setting tx_run.
Yep, this could be another option, but we need to change the name of that function in this case.
OK, how does virtio_vsock_vqs_setup() sound?
Or virtio_vsock_start() (without vqs)
Stefano
Stefano
Thanks,
Stefano
+{
mutex_lock(&vsock->rx_lock);
virtio_vsock_rx_fill(vsock);
vsock->rx_run = true;
@@ -568,8 +573,6 @@ static int virtio_vsock_vqs_init(struct virtio_vsock *vsock)
virtio_vsock_event_fill(vsock);
vsock->event_run = true;
mutex_unlock(&vsock->event_lock);
-
- return 0;
}
static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
@@ -664,6 +667,7 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
goto out;
rcu_assign_pointer(the_virtio_vsock, vsock);
+ virtio_vsock_vqs_fill(vsock);
mutex_unlock(&the_virtio_vsock_mutex);
@@ -736,6 +740,7 @@ static int virtio_vsock_restore(struct virtio_device *vdev)
goto out;
rcu_assign_pointer(the_virtio_vsock, vsock);
+ virtio_vsock_vqs_fill(vsock);
out:
mutex_unlock(&the_virtio_vsock_mutex);
--
2.34.1