The first patch fixes a virtio-spec violation. The other two patches
complete the driver configuration before using the VQs in the probe.
The patch order should simplify backporting in stable branches.
v2:
- patch 1 is not changed from v1
- added 2 patches to complete the driver configuration before using the
VQs in the probe [MST]
v1: https://lore.kernel.org/netdev/[email protected]/
Stefano Garzarella (3):
vsock/virtio: enable VQs early on probe
vsock/virtio: initialize vdev->priv before using VQs
vsock/virtio: read the negotiated features before using VQs
net/vmw_vsock/virtio_transport.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
--
2.35.1
On Wed, Mar 23, 2022 at 09:49:51AM +0100, Stefano Garzarella wrote:
> The first patch fixes a virtio-spec violation. The other two patches
> complete the driver configuration before using the VQs in the probe.
>
> The patch order should simplify backporting in stable branches.
Ok but I think the order is wrong. It should be 2-3-1,
otherwise bisect can pick just 1 and it will have
the issues previous reviw pointed out.
> v2:
> - patch 1 is not changed from v1
> - added 2 patches to complete the driver configuration before using the
> VQs in the probe [MST]
>
> v1: https://lore.kernel.org/netdev/[email protected]/
>
> Stefano Garzarella (3):
> vsock/virtio: enable VQs early on probe
> vsock/virtio: initialize vdev->priv before using VQs
> vsock/virtio: read the negotiated features before using VQs
>
> net/vmw_vsock/virtio_transport.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> --
> 2.35.1
On Wed, Mar 23, 2022 at 09:22:02AM -0400, Michael S. Tsirkin wrote:
>On Wed, Mar 23, 2022 at 09:49:51AM +0100, Stefano Garzarella wrote:
>> The first patch fixes a virtio-spec violation. The other two patches
>> complete the driver configuration before using the VQs in the probe.
>>
>> The patch order should simplify backporting in stable branches.
>
>Ok but I think the order is wrong. It should be 2-3-1,
>otherwise bisect can pick just 1 and it will have
>the issues previous reviw pointed out.
Right, I prioritized simplifying the backport, but obviously
bisectability is priority!
I'll send v3 changing the order in 2-3-1
Thanks,
Stefano
Complete the driver configuration, reading the negotiated features,
before using the VQs and tell the device that the driver is ready in
the virtio_vsock_probe().
Fixes: 53efbba12cc7 ("virtio/vsock: enable SEQPACKET for transport")
Suggested-by: Michael S. Tsirkin <[email protected]>
Signed-off-by: Stefano Garzarella <[email protected]>
---
net/vmw_vsock/virtio_transport.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index fff67ad39087..1244e7cf585b 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -622,6 +622,9 @@ 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);
+ if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET))
+ vsock->seqpacket_allow = true;
+
vdev->priv = vsock;
virtio_device_ready(vdev);
@@ -639,9 +642,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
vsock->event_run = true;
mutex_unlock(&vsock->event_lock);
- if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET))
- vsock->seqpacket_allow = true;
-
rcu_assign_pointer(the_virtio_vsock, vsock);
mutex_unlock(&the_virtio_vsock_mutex);
--
2.35.1
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 <[email protected]>
---
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);
--
2.35.1
When we fill VQs with empty buffers and kick the host, it may send
an interrupt. `vdev->priv` must be initialized before this since it
is used in the virtqueue callback.
Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
Suggested-by: Michael S. Tsirkin <[email protected]>
Signed-off-by: Stefano Garzarella <[email protected]>
---
net/vmw_vsock/virtio_transport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index b1962f8cd502..fff67ad39087 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -622,6 +622,7 @@ 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);
+ vdev->priv = vsock;
virtio_device_ready(vdev);
mutex_lock(&vsock->tx_lock);
@@ -641,7 +642,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
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);
--
2.35.1
On Wed, Mar 23, 2022 at 09:49:53AM +0100, Stefano Garzarella wrote:
> When we fill VQs with empty buffers and kick the host, it may send
> an interrupt. `vdev->priv` must be initialized before this since it
> is used in the virtqueue callback.
>
> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock")
> Suggested-by: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Stefano Garzarella <[email protected]>
> ---
> net/vmw_vsock/virtio_transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index b1962f8cd502..fff67ad39087 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -622,6 +622,7 @@ 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);
>
> + vdev->priv = vsock;
> virtio_device_ready(vdev);
Doh, patch order got me. :)
Stefan
On Wed, Mar 23, 2022 at 09:49:54AM +0100, Stefano Garzarella wrote:
> Complete the driver configuration, reading the negotiated features,
> before using the VQs and tell the device that the driver is ready in
> the virtio_vsock_probe().
>
> Fixes: 53efbba12cc7 ("virtio/vsock: enable SEQPACKET for transport")
> Suggested-by: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Stefano Garzarella <[email protected]>
> ---
> net/vmw_vsock/virtio_transport.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Stefan Hajnoczi <[email protected]>
On Wed, Mar 23, 2022 at 09:49:52AM +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.
>
> 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 <[email protected]>
> ---
> 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);
Can rx and event virtqueue interrupts be lost if they occur before we
assign vdev->priv later in virtio_vsock_probe()?
Stefan