Re: [PATCH v2] vsock/virtio: initialize the_virtio_vsock before using VQs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux