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. 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. Signed-off-by: JiaJia <[email protected]> --- Hi Peter, thanks for taking a look and for pointing this out. I took another look at the restore path and agree that deleting the virtqueues from that error path can race with `viortc_cb_alarmq()` once the device has been readied. This v2 follows the direction you suggested. It factors out `__viortc_remove()`, reuses that path for restore failures before `virtio_device_ready()`, routes `viortc_init_vqs()` failures in probe through `err_reset_vdev` for consistent teardown, drops the `viortc_del_vqs()` helper and the `viortc_msg_xfer()` NULL-vq guard, and avoids deleting the virtqueues again after `virtio_device_ready()`. Thanks, JiaJia v2: - reuse the remove path for restore failures before virtio_device_ready() - route viortc_init_vqs() failures in probe through err_reset_vdev - drop the viortc_del_vqs() helper and the viortc_msg_xfer() NULL-vq guard - avoid deleting virtqueues again after virtio_device_ready() 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
