[PATCH v2 0/2] virtio: Always reset vhost devices

[PATCH v2 0/2] virtio: Always reset vhost devices

[Hanna Czenczek]

Hi,

As explained in patch 2 (the main one) of this series, we currently
don’t issue the RESET_DEVICE command to vhost back-ends, even though we
fully intend to do so.

The problem is that sending this command is gated behind a vhost_started
check, but at that point (during the device reset process), the device
is actually stopped, and so vhost_started is false.  We still want to
send RESET_DEVICE there, so patch 2 removes the vhost_started condition.

This means that we need to be able to call VirtioDeviceClass.get_vhost()
when vhost_started is false.  For most .get_vhost() implementations,
that’s perfectly fine; but three of them (crypto, gpu, net) dereference
some pointers, so if any of them is NULL, we have to explicitly return
NULL in those implementations.  That’s what patch 1 is for.


This time, I’ve run `make check` with ubsan; I can confirm that v1
generated errors for vhost-net, but with patch 1 added, it’s clean.
I’ve also run the CI pipeline:
https://gitlab.com/hreitz/qemu/-/pipelines/1384524949
Specifically, clang-system passed:
https://gitlab.com/hreitz/qemu/-/jobs/7406688234
The only failure is msys2-64bit, which timed out (re-tried repeatedly),
but judging from https://gitlab.com/qemu-project/qemu/-/pipelines, I
think that’s expected.


v2: Added patch 1, left patch 2 unchanged.


Hanna Czenczek (2):
  virtio: Allow .get_vhost() without vhost_started
  virtio: Always reset vhost devices

 include/hw/virtio/virtio.h  |  1 +
 hw/display/vhost-user-gpu.c |  2 +-
 hw/net/virtio-net.c         | 19 +++++++++++++++++--
 hw/virtio/virtio-crypto.c   | 18 +++++++++++++++---
 hw/virtio/virtio.c          |  8 ++++++--
 5 files changed, 40 insertions(+), 8 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] virtio: Allow .get_vhost() without vhost_started

[Hanna Czenczek]

Historically, .get_vhost() was probably only called when
vdev->vhost_started is true.  However, we now decidedly want to call it
also when vhost_started is false, specifically so we can issue a reset
to the vhost back-end while device operation is stopped.

Some .get_vhost() implementations dereference some pointers (or return
offsets from them) that are probably guaranteed to be non-NULL when
vhost_started is true, but not necessarily otherwise.  This patch makes
all such implementations check all such pointers, returning NULL if any
is NULL.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 include/hw/virtio/virtio.h  |  1 +
 hw/display/vhost-user-gpu.c |  2 +-
 hw/net/virtio-net.c         | 19 +++++++++++++++++--
 hw/virtio/virtio-crypto.c   | 18 +++++++++++++++---
 4 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 7512afbc84..ff29088232 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -221,6 +221,7 @@ struct VirtioDeviceClass {
     int (*post_load)(VirtIODevice *vdev);
     const VMStateDescription *vmsd;
     bool (*primary_unplug_pending)(void *opaque);
+    /* May be called even when vdev->vhost_started is false */
     struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
     void (*toggle_device_iotlb)(VirtIODevice *vdev);
 };
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 63c64ddde6..5056ffd618 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -642,7 +642,7 @@ vhost_user_gpu_device_realize(DeviceState *qdev, Error **errp)
 static struct vhost_dev *vhost_user_gpu_get_vhost(VirtIODevice *vdev)
 {
     VhostUserGPU *g = VHOST_USER_GPU(vdev);
-    return &g->vhost->dev;
+    return g->vhost ? &g->vhost->dev : NULL;
 }
 
 static Property vhost_user_gpu_properties[] = {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 8f30972708..874ed9c032 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3890,8 +3890,23 @@ static bool dev_unplug_pending(void *opaque)
 static struct vhost_dev *virtio_net_get_vhost(VirtIODevice *vdev)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
-    NetClientState *nc = qemu_get_queue(n->nic);
-    struct vhost_net *net = get_vhost_net(nc->peer);
+    NetClientState *nc;
+    struct vhost_net *net;
+
+    if (!n->nic) {
+        return NULL;
+    }
+
+    nc = qemu_get_queue(n->nic);
+    if (!nc) {
+        return NULL;
+    }
+
+    net = get_vhost_net(nc->peer);
+    if (!net) {
+        return NULL;
+    }
+
     return &net->dev;
 }
 
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index bbe8aa4b99..3296b7f219 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -1247,9 +1247,21 @@ static bool virtio_crypto_guest_notifier_pending(VirtIODevice *vdev, int idx)
 static struct vhost_dev *virtio_crypto_get_vhost(VirtIODevice *vdev)
 {
     VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
-    CryptoDevBackend *b = vcrypto->cryptodev;
-    CryptoDevBackendClient *cc = b->conf.peers.ccs[0];
-    CryptoDevBackendVhost *vhost_crypto = cryptodev_get_vhost(cc, b, 0);
+    CryptoDevBackend *b;
+    CryptoDevBackendClient *cc;
+    CryptoDevBackendVhost *vhost_crypto;
+
+    b = vcrypto->cryptodev;
+    if (!b) {
+        return NULL;
+    }
+
+    cc = b->conf.peers.ccs[0];
+    vhost_crypto = cryptodev_get_vhost(cc, b, 0);
+    if (!vhost_crypto) {
+        return NULL;
+    }
+
     return &vhost_crypto->dev;
 }
 
-- 
2.45.2

[PATCH v2 2/2] virtio: Always reset vhost devices

[Hanna Czenczek]

Requiring `vhost_started` to be true for resetting vhost devices in
`virtio_reset()` seems like the wrong condition: Most importantly, the
preceding `virtio_set_status(vdev, 0)` call will (for vhost devices) end
up in `vhost_dev_stop()` (through vhost devices' `.set_status`
implementations), setting `vdev->vhost_started = false`.  Therefore, the
gated `vhost_reset_device()` call is unreachable.

`vhost_started` is not documented, so it is hard to say what exactly it
is supposed to mean, but judging from the fact that `vhost_dev_start()`
sets it and `vhost_dev_stop()` clears it, it seems like it indicates
whether there is a vhost back-end, and whether that back-end is
currently running and processing virtio requests.

Making a reset conditional on whether the vhost back-end is processing
virtio requests seems wrong; in fact, it is probably better to reset it
only when it is not currently processing requests, which is exactly the
current order of operations in `virtio_reset()`: First, the back-end is
stopped through `virtio_set_status(vdev, 0)`, then we want to send a
reset.

Therefore, we should drop the `vhost_started` condition, but in its
stead we then have to verify that we can indeed send a reset to this
vhost device, by not just checking `k->get_vhost != NULL` (introduced by
commit 95e1019a4a9), but also that the vhost back-end is connected
(`hdev = k->get_vhost(); hdev != NULL && hdev->vhost_ops != NULL`).

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 hw/virtio/virtio.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 583a224163..35dfc01074 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2150,8 +2150,12 @@ void virtio_reset(void *opaque)
         vdev->device_endian = virtio_default_endian();
     }
 
-    if (vdev->vhost_started && k->get_vhost) {
-        vhost_reset_device(k->get_vhost(vdev));
+    if (k->get_vhost) {
+        struct vhost_dev *hdev = k->get_vhost(vdev);
+        /* Only reset when vhost back-end is connected */
+        if (hdev && hdev->vhost_ops) {
+            vhost_reset_device(hdev);
+        }
     }
 
     if (k->reset) {
-- 
2.45.2

Re: [PATCH v2 1/2] virtio: Allow .get_vhost() without vhost_started

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

On Tue, Jul 23, 2024 at 06:39:39PM +0200, Hanna Czenczek wrote:
> Historically, .get_vhost() was probably only called when
> vdev->vhost_started is true.  However, we now decidedly want to call it
> also when vhost_started is false, specifically so we can issue a reset
> to the vhost back-end while device operation is stopped.
> 
> Some .get_vhost() implementations dereference some pointers (or return
> offsets from them) that are probably guaranteed to be non-NULL when
> vhost_started is true, but not necessarily otherwise.  This patch makes
> all such implementations check all such pointers, returning NULL if any
> is NULL.
> 
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  include/hw/virtio/virtio.h  |  1 +
>  hw/display/vhost-user-gpu.c |  2 +-
>  hw/net/virtio-net.c         | 19 +++++++++++++++++--
>  hw/virtio/virtio-crypto.c   | 18 +++++++++++++++---
>  4 files changed, 34 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]