* [PATCH net v2 1/3] vsock/virtio: enable VQs early on probe
2022-03-23 8:49 [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them Stefano Garzarella
@ 2022-03-23 8:49 ` Stefano Garzarella
2022-03-23 13:44 ` Stefan Hajnoczi
2022-03-23 8:49 ` [PATCH net v2 2/3] vsock/virtio: initialize vdev->priv before using VQs Stefano Garzarella
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Stefano Garzarella @ 2022-03-23 8:49 UTC (permalink / raw)
To: netdev
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, Asias He,
Stefan Hajnoczi, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
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 <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);
--
2.35.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net v2 1/3] vsock/virtio: enable VQs early on probe
2022-03-23 8:49 ` [PATCH net v2 1/3] vsock/virtio: enable VQs early on probe Stefano Garzarella
@ 2022-03-23 13:44 ` Stefan Hajnoczi
2022-03-23 13:54 ` Stefano Garzarella
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2022-03-23 13:44 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Asias He, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
[-- Attachment #1.1: Type: text/plain, Size: 1202 bytes --]
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 <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);
Can rx and event virtqueue interrupts be lost if they occur before we
assign vdev->priv later in virtio_vsock_probe()?
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net v2 1/3] vsock/virtio: enable VQs early on probe
2022-03-23 13:44 ` Stefan Hajnoczi
@ 2022-03-23 13:54 ` Stefano Garzarella
0 siblings, 0 replies; 10+ messages in thread
From: Stefano Garzarella @ 2022-03-23 13:54 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Asias He, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
On Wed, Mar 23, 2022 at 01:44:42PM +0000, Stefan Hajnoczi wrote:
>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 <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);
>
>Can rx and event virtqueue interrupts be lost if they occur before we
>assign vdev->priv later in virtio_vsock_probe()?
Yep, as Michael suggested I'll fix the patch order in the next version.
Thanks,
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net v2 2/3] vsock/virtio: initialize vdev->priv before using VQs
2022-03-23 8:49 [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them Stefano Garzarella
2022-03-23 8:49 ` [PATCH net v2 1/3] vsock/virtio: enable VQs early on probe Stefano Garzarella
@ 2022-03-23 8:49 ` Stefano Garzarella
2022-03-23 13:45 ` Stefan Hajnoczi
2022-03-23 8:49 ` [PATCH net v2 3/3] vsock/virtio: read the negotiated features " Stefano Garzarella
2022-03-23 13:22 ` [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them Michael S. Tsirkin
3 siblings, 1 reply; 10+ messages in thread
From: Stefano Garzarella @ 2022-03-23 8:49 UTC (permalink / raw)
To: netdev
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, Asias He,
Stefan Hajnoczi, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
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 <mst@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net v2 2/3] vsock/virtio: initialize vdev->priv before using VQs
2022-03-23 8:49 ` [PATCH net v2 2/3] vsock/virtio: initialize vdev->priv before using VQs Stefano Garzarella
@ 2022-03-23 13:45 ` Stefan Hajnoczi
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2022-03-23 13:45 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Asias He, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
[-- Attachment #1.1: Type: text/plain, Size: 1093 bytes --]
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 <mst@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 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
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net v2 3/3] vsock/virtio: read the negotiated features before using VQs
2022-03-23 8:49 [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them Stefano Garzarella
2022-03-23 8:49 ` [PATCH net v2 1/3] vsock/virtio: enable VQs early on probe Stefano Garzarella
2022-03-23 8:49 ` [PATCH net v2 2/3] vsock/virtio: initialize vdev->priv before using VQs Stefano Garzarella
@ 2022-03-23 8:49 ` Stefano Garzarella
2022-03-23 13:45 ` Stefan Hajnoczi
2022-03-23 13:22 ` [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them Michael S. Tsirkin
3 siblings, 1 reply; 10+ messages in thread
From: Stefano Garzarella @ 2022-03-23 8:49 UTC (permalink / raw)
To: netdev
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, Asias He,
Stefan Hajnoczi, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
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 <mst@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net v2 3/3] vsock/virtio: read the negotiated features before using VQs
2022-03-23 8:49 ` [PATCH net v2 3/3] vsock/virtio: read the negotiated features " Stefano Garzarella
@ 2022-03-23 13:45 ` Stefan Hajnoczi
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2022-03-23 13:45 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Asias He, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
[-- Attachment #1.1: Type: text/plain, Size: 590 bytes --]
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 <mst@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> net/vmw_vsock/virtio_transport.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them
2022-03-23 8:49 [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them Stefano Garzarella
` (2 preceding siblings ...)
2022-03-23 8:49 ` [PATCH net v2 3/3] vsock/virtio: read the negotiated features " Stefano Garzarella
@ 2022-03-23 13:22 ` Michael S. Tsirkin
2022-03-23 13:43 ` Stefano Garzarella
3 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2022-03-23 13:22 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, netdev, linux-kernel, virtualization, Asias He,
Stefan Hajnoczi, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
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/20220322103823.83411-1-sgarzare@redhat.com/
>
> 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them
2022-03-23 13:22 ` [PATCH net v2 0/3] vsock/virtio: enable VQs early on probe and finish the setup before using them Michael S. Tsirkin
@ 2022-03-23 13:43 ` Stefano Garzarella
0 siblings, 0 replies; 10+ messages in thread
From: Stefano Garzarella @ 2022-03-23 13:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, Asias He,
Stefan Hajnoczi, Jakub Kicinski, Arseny Krasnov, Paolo Abeni,
David S. Miller
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread