Re: [PATCH net] vsock/virtio: enable VQs early on probe
On Tue, Mar 22, 2022 at 10:09:06AM -0400, Michael S. Tsirkin wrote: On Tue, Mar 22, 2022 at 03:05:00PM +0100, Stefano Garzarella wrote: On Tue, Mar 22, 2022 at 09:36:14AM -0400, Michael S. Tsirkin wrote: > 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 > > --- > > 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(>event_work, virtio_transport_event_work); > > INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work); > > > > + virtio_device_ready(vdev); > > + > > mutex_lock(>tx_lock); > > vsock->tx_run = true; > > mutex_unlock(>tx_lock); > > Here's the whole code snippet: > > >mutex_lock(>tx_lock); >vsock->tx_run = true; >mutex_unlock(>tx_lock); > >mutex_lock(>rx_lock); >virtio_vsock_rx_fill(vsock); >vsock->rx_run = true; >mutex_unlock(>rx_lock); > >mutex_lock(>event_lock); >virtio_vsock_event_fill(vsock); >vsock->event_run = true; >mutex_unlock(>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(_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. Right, so if you agree I'll move these before virtio_device_ready(). > 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, >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. Yes I see, should I break into 2 patches (one where I move the code already present and this one)? Maybe a single patch is fine since it's the complete solution. Thank you for the detailed explanation, Stefano Two I think since movement can be backported to before the hardening effort. Yep, maybe 3 since seqpacket was added later. Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net] vsock/virtio: enable VQs early on probe
On Tue, Mar 22, 2022 at 03:05:00PM +0100, Stefano Garzarella wrote: > On Tue, Mar 22, 2022 at 09:36:14AM -0400, Michael S. Tsirkin wrote: > > 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 > > > --- > > > 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(>event_work, virtio_transport_event_work); > > > INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work); > > > > > > + virtio_device_ready(vdev); > > > + > > > mutex_lock(>tx_lock); > > > vsock->tx_run = true; > > > mutex_unlock(>tx_lock); > > > > Here's the whole code snippet: > > > > > >mutex_lock(>tx_lock); > >vsock->tx_run = true; > >mutex_unlock(>tx_lock); > > > >mutex_lock(>rx_lock); > >virtio_vsock_rx_fill(vsock); > >vsock->rx_run = true; > >mutex_unlock(>rx_lock); > > > >mutex_lock(>event_lock); > >virtio_vsock_event_fill(vsock); > >vsock->event_run = true; > >mutex_unlock(>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(_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. > > Right, so if you agree I'll move these before virtio_device_ready(). > > > 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, >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. > > Yes I see, should I break into 2 patches (one where I move the code already > present and this one)? > > Maybe a single patch is fine since it's the complete solution. > > Thank you for the detailed explanation, > Stefano Two I think since movement can be backported to before the hardening effort. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net] vsock/virtio: enable VQs early on probe
On Tue, Mar 22, 2022 at 09:36:14AM -0400, Michael S. Tsirkin wrote: 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 --- 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(>event_work, virtio_transport_event_work); INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work); + virtio_device_ready(vdev); + mutex_lock(>tx_lock); vsock->tx_run = true; mutex_unlock(>tx_lock); Here's the whole code snippet: mutex_lock(>tx_lock); vsock->tx_run = true; mutex_unlock(>tx_lock); mutex_lock(>rx_lock); virtio_vsock_rx_fill(vsock); vsock->rx_run = true; mutex_unlock(>rx_lock); mutex_lock(>event_lock); virtio_vsock_event_fill(vsock); vsock->event_run = true; mutex_unlock(>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(_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. Right, so if you agree I'll move these before virtio_device_ready(). 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, >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. Yes I see, should I break into 2 patches (one where I move the code already present and this one)? Maybe a single patch is fine since it's the complete solution. Thank you for the detailed explanation, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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 > --- > 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(>event_work, virtio_transport_event_work); > INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work); > > + virtio_device_ready(vdev); > + > mutex_lock(>tx_lock); > vsock->tx_run = true; > mutex_unlock(>tx_lock); Here's the whole code snippet: mutex_lock(>tx_lock); vsock->tx_run = true; mutex_unlock(>tx_lock); mutex_lock(>rx_lock); virtio_vsock_rx_fill(vsock); vsock->rx_run = true; mutex_unlock(>rx_lock); mutex_lock(>event_lock); virtio_vsock_event_fill(vsock); vsock->event_run = true; mutex_unlock(>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(_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, >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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH net] vsock/virtio: enable VQs early on probe
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. 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 --- 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(>event_work, virtio_transport_event_work); INIT_WORK(>send_pkt_work, virtio_transport_send_pkt_work); + virtio_device_ready(vdev); + mutex_lock(>tx_lock); vsock->tx_run = true; mutex_unlock(>tx_lock); -- 2.35.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization