[PATCH v3] virtio: rtc: tear down old virtqueues before restore

[physicalmtea]

From: JiaJia <physicalm...@gmail.com>

virtio_device_restore() resets the device and restores the negotiated
features before calling ->restore(). viortc_freeze() intentionally
leaves the existing virtqueues in place so the alarm queue can still
wake the system, but viortc_restore() immediately calls
viortc_init_vqs() without first deleting those old queues.

If virtqueue reinitialization fails on virtio-pci, the transport error
path can run vp_del_vqs() against a newly allocated vp_dev->vqs array
while vdev->vqs still contains the old virtqueues. vp_del_vqs() then
looks up queue state through the new array and can dereference a NULL
info pointer in vp_del_vq(), crashing the guest kernel during restore.

This can also happen during a non-faulty reinitialization, when one of
the vp_find_vqs_msix() attempts is unsuccessful before a later attempt
would succeed.

Delete the stale virtqueues before rebuilding them. If restore fails
before virtio_device_ready(), reuse the remove path to stop the device.
Once the device is ready, return errors directly instead of deleting the
virtqueues again.

Fixes: 0623c7592768 ("virtio_rtc: Add module and driver core")
Signed-off-by: JiaJia <physicalm...@gmail.com>
Reviewed-by: Peter Hilber <peter.hil...@oss.qualcomm.com>
---
v3:
  - Add Fixes tag.
  - Add Peter's Reviewed-by tag.
  - Clarify commit message regarding reinitialization scenarios.

Apologies for missing the Fixes tag, and thanks for catching that! 
I've also updated the commit message to include the scenario you 
mentioned.

 drivers/virtio/virtio_rtc_driver.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_rtc_driver.c 
b/drivers/virtio/virtio_rtc_driver.c
index a57d5e06e..4419735b0 100644
--- a/drivers/virtio/virtio_rtc_driver.c
+++ b/drivers/virtio/virtio_rtc_driver.c
@@ -1257,6 +1257,15 @@ static int viortc_init_vqs(struct viortc_dev *viortc)
        return 0;
 }
 
+static void __viortc_remove(struct viortc_dev *viortc)
+{
+       struct virtio_device *vdev = viortc->vdev;
+
+       viortc_clocks_deinit(viortc);
+       virtio_reset_device(vdev);
+       vdev->config->del_vqs(vdev);
+}
+
 /**
  * viortc_probe() - probe a virtio_rtc virtio device
  * @vdev: virtio device
@@ -1282,7 +1291,7 @@ static int viortc_probe(struct virtio_device *vdev)
 
        ret = viortc_init_vqs(viortc);
        if (ret)
-               return ret;
+               goto err_reset_vdev;
 
        virtio_device_ready(vdev);
 
@@ -1329,10 +1338,7 @@ static void viortc_remove(struct virtio_device *vdev)
 {
        struct viortc_dev *viortc = vdev->priv;
 
-       viortc_clocks_deinit(viortc);
-
-       virtio_reset_device(vdev);
-       vdev->config->del_vqs(vdev);
+       __viortc_remove(viortc);
 }
 
 static int viortc_freeze(struct virtio_device *dev)
@@ -1353,9 +1359,11 @@ static int viortc_restore(struct virtio_device *dev)
        bool notify = false;
        int ret;
 
+       dev->config->del_vqs(dev);
+
        ret = viortc_init_vqs(viortc);
        if (ret)
-               return ret;
+               goto err_remove;
 
        alarm_viortc_vq = &viortc->vqs[VIORTC_ALARMQ];
        alarm_vq = alarm_viortc_vq->vq;
@@ -1364,7 +1372,7 @@ static int viortc_restore(struct virtio_device *dev)
                ret = viortc_populate_vq(viortc, alarm_viortc_vq,
                                         VIORTC_ALARMQ_BUF_CAP, false);
                if (ret)
-                       return ret;
+                       goto err_remove;
 
                notify = virtqueue_kick_prepare(alarm_vq);
        }
@@ -1372,8 +1380,12 @@ static int viortc_restore(struct virtio_device *dev)
        virtio_device_ready(dev);
 
        if (notify && !virtqueue_notify(alarm_vq))
-               ret = -EIO;
+               return -EIO;
+
+       return 0;
 
+err_remove:
+       __viortc_remove(viortc);
        return ret;
 }
 
-- 
2.34.1