Messages in this thread | | | Date | Tue, 22 Mar 2022 09:36:14 -0400 | From | "Michael S. Tsirkin" <> | Subject | Re: [PATCH net] vsock/virtio: enable VQs early on probe |
| |
On Tue, Mar 22, 2022 at 11:38:23AM +0100, Stefano Garzarella wrote: > virtio spec requires drivers to set DRIVER_OK before using VQs. > This is set automatically after probe returns, but virtio-vsock > driver uses VQs in the probe function to fill rx and event VQs > with new buffers.
So this is a spec violation. absolutely.
> Let's fix this, calling virtio_device_ready() before using VQs > in the probe function. > > Fixes: 0ea9e1d3a9e3 ("VSOCK: Introduce virtio_transport.ko") > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > net/vmw_vsock/virtio_transport.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c > index 5afc194a58bb..b1962f8cd502 100644 > --- a/net/vmw_vsock/virtio_transport.c > +++ b/net/vmw_vsock/virtio_transport.c > @@ -622,6 +622,8 @@ static int virtio_vsock_probe(struct virtio_device *vdev) > INIT_WORK(&vsock->event_work, virtio_transport_event_work); > INIT_WORK(&vsock->send_pkt_work, virtio_transport_send_pkt_work); > > + virtio_device_ready(vdev); > + > mutex_lock(&vsock->tx_lock); > vsock->tx_run = true; > mutex_unlock(&vsock->tx_lock);
Here's the whole code snippet:
mutex_lock(&vsock->tx_lock); vsock->tx_run = true; mutex_unlock(&vsock->tx_lock);
mutex_lock(&vsock->rx_lock); virtio_vsock_rx_fill(vsock); vsock->rx_run = true; mutex_unlock(&vsock->rx_lock);
mutex_lock(&vsock->event_lock); virtio_vsock_event_fill(vsock); vsock->event_run = true; mutex_unlock(&vsock->event_lock);
if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET)) vsock->seqpacket_allow = true;
vdev->priv = vsock; rcu_assign_pointer(the_virtio_vsock, vsock);
mutex_unlock(&the_virtio_vsock_mutex);
I worry that this is not the only problem here: seqpacket_allow and setting of vdev->priv at least after device is active look suspicious. E.g.:
static void virtio_vsock_event_done(struct virtqueue *vq) { struct virtio_vsock *vsock = vq->vdev->priv;
if (!vsock) return; queue_work(virtio_vsock_workqueue, &vsock->event_work); }
looks like it will miss events now they will be reported earlier. One might say that since vq has been kicked it might send interrupts earlier too so not a new problem, but there's a chance device actually waits until DRIVER_OK to start operating.
> -- > 2.35.1
| |