1. Patchew
  2. linux
  3. View series

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

JiaJia posted 1 patch 1 month, 3 weeks ago
There is a newer version of this series
drivers/virtio/virtio_rtc_driver.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
[PATCH v2] virtio: rtc: tear down old virtqueues before restore
Posted by JiaJia 1 month, 3 weeks ago 
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 <physicalmtea@gmail.com>
---
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
Re: [PATCH v2] virtio: rtc: tear down old virtqueues before restore
Posted by Peter Hilber 1 month, 3 weeks ago 
On Fri, Apr 24, 2026 at 11:14:53AM +0800, JiaJia wrote:
> 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 <physicalmtea@gmail.com>

Reviewed-by: Peter Hilber <peter.hilber@oss.qualcomm.com>

What should still be added is the Fixes tag:

	Fixes: 0623c7592768 ("virtio_rtc: Add module and driver core")

Maybe the commit message could also mention that this can happen during
a non-faulty reinitialization, when one of the vp_find_vqs_msix()
attempts is unsuccessful before a later attempt would succeed (as far as
I understand).

Thanks!

Peter
[PATCH v3] virtio: rtc: tear down old virtqueues before restore
Posted by physicalmtea 1 month, 3 weeks ago 
From: JiaJia <physicalmtea@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 <physicalmtea@gmail.com>
Reviewed-by: Peter Hilber <peter.hilber@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
Re: [PATCH v3] virtio: rtc: tear down old virtqueues before restore
Posted by Michael S. Tsirkin 1 month, 3 weeks ago 
On Sat, Apr 25, 2026 at 03:08:40PM +0800, physicalmtea wrote:
> From: JiaJia <physicalmtea@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 <physicalmtea@gmail.com>

Sorry is this just the 1st name or the 1st and the last name?
I think it's preferred to have the full name here.


> Reviewed-by: Peter Hilber <peter.hilber@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
Re: [PATCH v3] virtio: rtc: tear down old virtqueues before restore
Posted by Jia Jia 1 month, 3 weeks ago 
Hi Michael,

JiaJia is my full name. My family name is Jia and my given name is also Jia.

I wrote it as "JiaJia", but "Jia Jia" is probably clearer. I will use "Jia Jia"
in future postings.

Thanks,
Jia Jia
Re: [PATCH v3] virtio: rtc: tear down old virtqueues before restore
Posted by Michael S. Tsirkin 1 month, 3 weeks ago 
On Sun, Apr 26, 2026 at 12:09:06AM +0800, Jia Jia wrote:
> Hi Michael,
> 
> JiaJia is my full name. My family name is Jia and my given name is also Jia.
> 
> I wrote it as "JiaJia", but "Jia Jia" is probably clearer. I will use "Jia Jia"
> in future postings.
> 
> Thanks,
> Jia Jia

Beautiful. Yes, clearer, thank you so much.

-- 
MST
Re: [PATCH v3] virtio: rtc: tear down old virtqueues before restore
Posted by Jia Jia 1 month, 1 week ago 
Hi Michael, Peter,

Gentle ping on this one.

I checked linux-next and mst's vhost tree today and still do not see it
there. Since v3 already has Peter's Reviewed-by and there were no further
technical comments, should I resend it or can it be queued as-is?

Thanks,
Jia Jia
Re: [PATCH v3] virtio: rtc: tear down old virtqueues before restore
Posted by Michael S. Tsirkin 1 month, 1 week ago 
On Thu, May 07, 2026 at 07:24:58PM +0800, Jia Jia wrote:
> Hi Michael, Peter,
> 
> Gentle ping on this one.
> 
> I checked linux-next and mst's vhost tree today and still do not see it
> there. Since v3 already has Peter's Reviewed-by and there were no further
> technical comments, should I resend it or can it be queued as-is?
> 
> Thanks,
> Jia Jia

Hello!
You need to start a new thread with v4 not as a reply to v2 otherwise
lots of machinery scanning lkml thinks it's part of the discussion.

Thanks!

-- 
MST

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.