[PATCH v2 0/3] virtio: synchronize callbacks during device reset

[PATCH v2 0/3] virtio: synchronize callbacks during device reset

[Karl Mehltretter]

A virtqueue callback can outlive virtio_reset_device() and race with a
driver freeing the state it uses. The reset helper documents that no
callbacks remain in progress, but that depends on the transport:
virtio-pci waits in vp_reset(), while virtio-mmio does not.

virtio_input has a related ordering problem: it unregisters the input
device before resetting the virtio device, while an event callback may
still be using the input device.

Patch 1 moves the callback wait into the core, using the existing
virtio_synchronize_cbs() operation. Patch 2 fixes the virtio_input teardown
order. Patch 3 adds the missing synchronize_cbs hooks for UML, TmFIFO,
remoteproc and virtio-vdpa. Remoteproc uses one SRCU domain per processor.

The new hooks wait for callbacks already running. UML, TmFIFO and
remoteproc still allow new callbacks after reset; fixing that is separate
work. The event-virtqueue DMA mapping issue is also separate.

Changes in v2:
- Patch 1: move the wait into the core instead of fixing only MMIO, as
  Michael suggested. Remove the duplicate PCI wait, preserve its shutdown
  wait, and fix CCW callback locking. Drop the MMIO polling: reset polling
  for v3 and newer is already in fa8833c085b6 ("virtio-mmio: add support
  for transport version 3").
- Patch 2: keep draining completed events when ready becomes false,
  instead of breaking out, so teardown does not truncate an input packet.
- Patch 3 is new, at Michael's request.

I reran the 120-cycle unbind/rebind test in an arm64 KASAN guest with four
vCPUs and a 5 ms busy delay per event. With patch 2 alone over virtio-mmio,
reset returned with the callback still running in all 84 overlapping
cycles. With the series, it waited in all 103. Over PCI with per-queue
MSI-X, it waited in all 66, including runs with threadirqs. No KASAN or
lockdep reports. These runs predate the per-rproc change, which leaves
the tested MMIO and PCI paths unchanged. They confirmed the missing wait,
but did not reproduce a use-after-free: with evdev attached,
input_unregister_device() waits for an RCU grace period that the IRQ
callback blocks.

That version also passed QEMU input, rebind and shutdown checks on arm64
MMIO and x86-64 PCI, including arm64 RT, KASAN and KCSAN builds, and x86
UP/Tiny SRCU. A legacy INTx NIC was present for an additional x86
shutdown check. The changed objects built with W=1 without warnings on
arm64, x86-64, s390 and SMP UML. The per-rproc version built with W=1
on arm64 KASAN and x86 Tiny SRCU, and passed six remoteproc callback and
lifetime KUnit tests on each, using mock remoteproc devices.

Link: https://lore.kernel.org/r/20260818040433.66986-1-kmehltretter@gmail.com

Karl Mehltretter (3):
  virtio: synchronize callbacks during device reset
  virtio_input: stop callbacks before unregistering input device
  virtio: implement synchronize_cbs for remaining transports

 arch/um/drivers/virtio_uml.c             | 10 ++++++++++
 drivers/platform/mellanox/mlxbf-tmfifo.c | 14 ++++++++++++++
 drivers/remoteproc/remoteproc_core.c     | 10 ++++++++++
 drivers/remoteproc/remoteproc_virtio.c   | 20 +++++++++++++++++---
 drivers/s390/virtio/virtio_ccw.c         |  6 +-----
 drivers/virtio/virtio.c                  |  2 ++
 drivers/virtio/virtio_input.c            |  8 ++++++--
 drivers/virtio/virtio_pci_legacy.c       |  2 --
 drivers/virtio/virtio_pci_modern.c       |  3 ---
 drivers/virtio/virtio_vdpa.c             | 21 ++++++++++++++++++++-
 include/linux/remoteproc.h               |  3 +++
 include/linux/virtio_config.h            |  6 +++---
 12 files changed, 86 insertions(+), 19 deletions(-)

base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3
-- 
2.39.5 (Apple Git-154)

[PATCH v2 1/3] virtio: synchronize callbacks during device reset

[Karl Mehltretter]

virtio_reset_device() promises that vq callbacks have finished when it
returns. virtio-pci waits in vp_reset(), but other transports can return
with a callback still running.

Call virtio_synchronize_cbs() after config->reset() and drop the duplicate
waits from both PCI reset methods. Add the wait to virtio_device_shutdown()
too, since it calls config->reset() directly. Keep the pre-reset call under
CONFIG_VIRTIO_HARDEN_NOTIFICATION so callbacks see vq->broken.

Always take irq_lock in the classic virtio-ccw interrupt handler so it
pairs with synchronize_cbs even without notification hardening. Use
is_thinint to choose the lock: airq_info can stay allocated after a
fallback to classic interrupts.

The transport reset must still stop new callbacks before this wait.

Fixes: d9679d0013a6 ("virtio: wrap config->reset calls")
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/s390/virtio/virtio_ccw.c   | 6 +-----
 drivers/virtio/virtio.c            | 2 ++
 drivers/virtio/virtio_pci_legacy.c | 2 --
 drivers/virtio/virtio_pci_modern.c | 3 ---
 include/linux/virtio_config.h      | 6 +++---
 5 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index bab6cad3fd5c..552d77998012 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -1062,7 +1062,7 @@ static void virtio_ccw_synchronize_cbs(struct virtio_device *vdev)
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	struct airq_info *info = vcdev->airq_info;
 
-	if (info) {
+	if (vcdev->is_thinint && info) {
 		/*
 		 * This device uses adapter interrupts: synchronize with
 		 * vring_interrupt() called by virtio_airq_handler()
@@ -1204,13 +1204,11 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
 			vcdev->err = -EIO;
 	}
 	virtio_ccw_check_activity(vcdev, activity);
-#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
 	/*
 	 * Paired with virtio_ccw_synchronize_cbs() and interrupts are
 	 * disabled here.
 	 */
 	read_lock(&vcdev->irq_lock);
-#endif
 	for_each_set_bit(i, indicators(vcdev),
 			 sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
 		/* The bit clear must happen before the vring kick. */
@@ -1219,9 +1217,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
 		vq = virtio_ccw_vq_by_ind(vcdev, i);
 		vring_interrupt(0, vq);
 	}
-#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
 	read_unlock(&vcdev->irq_lock);
-#endif
 	if (test_bit(0, indicators2(vcdev))) {
 		virtio_config_changed(&vcdev->vdev);
 		clear_bit(0, indicators2(vcdev));
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 75bb4ffe3b87..ad1c50b8a94e 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -264,6 +264,7 @@ void virtio_reset_device(struct virtio_device *dev)
 #endif
 
 	dev->config->reset(dev);
+	virtio_synchronize_cbs(dev);
 }
 EXPORT_SYMBOL_GPL(virtio_reset_device);
 
@@ -424,6 +425,7 @@ void virtio_device_shutdown(struct virtio_device *dev)
 	 * Some devices get wedged if this happens, so reset to make sure it does not.
 	 */
 	dev->config->reset(dev);
+	virtio_synchronize_cbs(dev);
 }
 EXPORT_SYMBOL_GPL(virtio_device_shutdown);
 
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index d9cbb02b35a1..8115aa39e01e 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -98,8 +98,6 @@ static void vp_reset(struct virtio_device *vdev)
 	/* Flush out the status write, and flush in device writes,
 	 * including MSi-X interrupts, if any. */
 	vp_legacy_get_status(&vp_dev->ldev);
-	/* Flush pending VQ/configuration callbacks. */
-	vp_synchronize_vectors(vdev);
 }
 
 static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 6d8ae2a6a8ca..c9e21317c51a 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -559,9 +559,6 @@ static void vp_reset(struct virtio_device *vdev)
 		msleep(1);
 
 	vp_modern_avq_cleanup(vdev);
-
-	/* Flush pending VQ/configuration callbacks. */
-	vp_synchronize_vectors(vdev);
 }
 
 static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 69f84ea85d71..8684a1e268ee 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -71,9 +71,9 @@ struct virtqueue_info {
  *	Returns 0 on success or error status
  * @del_vqs: free virtqueues found by find_vqs().
  * @synchronize_cbs: synchronize with the virtqueue callbacks (optional)
- *      The function guarantees that all memory operations on the
- *      queue before it are visible to the vring_interrupt() that is
- *      called after it.
+ *      Wait for running callbacks to complete. Memory operations on the
+ *      queue before this call must be visible to vring_interrupt() calls
+ *      that follow it.
  *      vdev: the virtio_device
  * @get_features: get the array of feature bits for this device.
  *	vdev: the virtio_device
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 1/3] virtio: synchronize callbacks during device reset

[sashiko-bot]

> virtio_reset_device() promises that vq callbacks have finished when it
> returns. virtio-pci waits in vp_reset(), but other transports can return
> with a callback still running.
> 
> Call virtio_synchronize_cbs() after config->reset() and drop the duplicate
> waits from both PCI reset methods. Add the wait to virtio_device_shutdown()
> too, since it calls config->reset() directly. Keep the pre-reset call under
> CONFIG_VIRTIO_HARDEN_NOTIFICATION so callbacks see vq->broken.
> 
> Always take irq_lock in the classic virtio-ccw interrupt handler so it
> pairs with synchronize_cbs even without notification hardening. Use
> is_thinint to choose the lock: airq_info can stay allocated after a
> fallback to classic interrupts.
> 
> The transport reset must still stop new callbacks before this wait.
> 
> Fixes: d9679d0013a6 ("virtio: wrap config->reset calls")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905152059.89560-1-kmehltretter@gmail.com?part=1

[PATCH v2 2/3] virtio_input: stop callbacks before unregistering input device

[Karl Mehltretter]

virtinput_remove() unregisters the input device before resetting the
virtio device. virtinput_recv_events() drops vi->lock around input_event(),
so clearing vi->ready does not stop a callback that passed the entry check.
It can still use vi->idev, requeue buffers and kick the queue.

Reset first, as virtinput_freeze() already does. With the preceding core
change, reset waits for callbacks before input_unregister_device() can
free vi->idev. Recheck vi->ready after taking the lock again: keep draining
completed events so an input packet is not truncated, but stop requeueing
buffers and kicking the queue.

With evdev attached, input_unregister_handle() currently waits for an RCU
grace period, which also waits out IRQ callbacks. This masks the lifetime
bug on PCI and MMIO, but does not protect sleepable callbacks on other
transports.

Fixes: 271c865161c5 ("Add virtio-input driver.")
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/virtio/virtio_input.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index deec24e8e682..7b654af0a42c 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -49,9 +49,12 @@ static void virtinput_recv_events(struct virtqueue *vq)
 				    le16_to_cpu(event->code),
 				    le32_to_cpu(event->value));
 			spin_lock_irqsave(&vi->lock, flags);
+			if (!vi->ready)
+				continue;
 			virtinput_queue_evtbuf(vi, event);
 		}
-		virtqueue_kick(vq);
+		if (vi->ready)
+			virtqueue_kick(vq);
 	}
 	spin_unlock_irqrestore(&vi->lock, flags);
 }
@@ -350,8 +353,9 @@ static void virtinput_remove(struct virtio_device *vdev)
 	vi->ready = false;
 	spin_unlock_irqrestore(&vi->lock, flags);
 
-	input_unregister_device(vi->idev);
+	/* Callbacks use vi->idev. */
 	virtio_reset_device(vdev);
+	input_unregister_device(vi->idev);
 	while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)
 		kfree(buf);
 	vdev->config->del_vqs(vdev);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 2/3] virtio_input: stop callbacks before unregistering input device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `virtio_reset_device()` in the error path of `virtinput_probe()` before deleting virtqueues, potentially leading to use-after-free or memory corruption by the active hardware/hypervisor.
- [High] The driver leaks DMA mappings for unconsumed buffers in the event virtqueue (`vi->evt`) upon device removal or suspend/freeze.
--

commit 71bbbcca0b03e3c67fa905a8357e9d3ad23e7fe4
Author: Karl Mehltretter <kmehltretter@gmail.com>

virtio_input: stop callbacks before unregistering input device

virtinput_remove() unregisters the input device before resetting the
virtio device. virtinput_recv_events() drops vi->lock around input_event(),
so clearing vi->ready does not stop a callback that passed the entry check.
It can still use vi->idev, requeue buffers and kick the queue.

> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index deec24e8e6828..7b654af0a42cb 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
[ ... ]
> @@ -350,8 +353,9 @@ static void virtinput_remove(struct virtio_device *vdev)
>  	vi->ready = false;
>  	spin_unlock_irqrestore(&vi->lock, flags);
>  
> -	input_unregister_device(vi->idev);
> +	/* Callbacks use vi->idev. */
>  	virtio_reset_device(vdev);
> +	input_unregister_device(vi->idev);
>  	while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)
>  		kfree(buf);
>  	vdev->config->del_vqs(vdev);

[Severity: High]
This is a pre-existing issue, but does this cleanup leak DMA mappings for
the event virtqueue (vi->evt)?

The cleanup loop correctly detaches and unmaps vi->sts using
virtqueue_detach_unused_buf(), but it ignores vi->evt. Since
virtinput_fill_evt() populates vi->evt with buffers using
virtqueue_add_inbuf_cache_clean(), which establishes DMA mappings under
the hood, omitting the detachment step leaves these mappings active.

Does this result in a persistent DMA mapping leak every time the device is
unbound? Furthermore, since the memory for these buffers (vi->evts) is freed,
does this leave active DMA mappings pointing to freed memory, posing a memory
corruption risk if delayed DMA occurs?

Does this exact same DMA mapping leak also occur during the power management
suspend cycle in virtinput_freeze() which performs the same cleanup?

[Severity: High]
This is also a pre-existing issue, but is there a missing
virtio_reset_device() in the error path of virtinput_probe()?

Looking at virtinput_probe(), the device is marked as ready:

drivers/virtio/virtio_input.c:virtinput_probe() {
    ...
	virtio_device_ready(vdev);
	vi->ready = true;
	err = input_register_device(vi->idev);
	if (err)
		goto err_input_register;
    ...
}

If input_register_device() fails, the error path deletes the virtqueues:

drivers/virtio/virtio_input.c:virtinput_probe() {
    ...
err_input_alloc:
	vdev->config->del_vqs(vdev);
    ...
}

Since virtio_device_ready() sets the device status to DRIVER_OK and enables
the host/hardware to start processing virtqueues, does deleting the active
virtqueue structures without first calling virtio_reset_device() to halt
the device risk a use-after-free or memory corruption by the host backend
if it continues to access the freed vring memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905152059.89560-1-kmehltretter@gmail.com?part=2

[PATCH v2 3/3] virtio: implement synchronize_cbs for remaining transports

[Karl Mehltretter]

virtio_reset_device() now calls synchronize_cbs to wait for running
callbacks. Its synchronize_rcu() fallback does not cover workqueue or
sleepable callbacks.

Add the missing hooks. UML waits for its shared IRQ; TmFIFO takes the
existing per-direction locks held around vring_interrupt(). Virtio-vdpa
uses a per-device rwlock around its callbacks, which must already be
hard-IRQ safe.

Remoteproc callbacks can run in hard-IRQ or process context, and rpmsg
callbacks can sleep, so use one SRCU domain per rproc. Enter it before
looking up the queue. Initialize it in rproc_alloc(), returning NULL if
that fails, and clean it up at final release. cleanup_srcu_struct() can
sleep, so document that rproc_put() and rproc_free() may sleep when
dropping the last reference.

These hooks wait for callbacks already running. UML, TmFIFO and
remoteproc still allow new callbacks after reset.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 arch/um/drivers/virtio_uml.c             | 10 ++++++++++
 drivers/platform/mellanox/mlxbf-tmfifo.c | 14 ++++++++++++++
 drivers/remoteproc/remoteproc_core.c     | 10 ++++++++++
 drivers/remoteproc/remoteproc_virtio.c   | 20 +++++++++++++++++---
 drivers/virtio/virtio_vdpa.c             | 21 ++++++++++++++++++++-
 include/linux/remoteproc.h               |  3 +++
 6 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
index 7425a8548141..baca6b09e9ac 100644
--- a/arch/um/drivers/virtio_uml.c
+++ b/arch/um/drivers/virtio_uml.c
@@ -20,6 +20,7 @@
  *
  * Based on Virtio MMIO driver by Pawel Moll, copyright 2011-2014, ARM Ltd.
  */
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -869,6 +870,14 @@ static void vu_reset(struct virtio_device *vdev)
 	vu_dev->status = 0;
 }
 
+static void vu_synchronize_cbs(struct virtio_device *vdev)
+{
+	struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
+
+	if (vu_dev->irq >= 0)
+		synchronize_irq(vu_dev->irq);
+}
+
 static void vu_del_vq(struct virtqueue *vq)
 {
 	struct virtio_uml_vq_info *info = vq->priv;
@@ -1121,6 +1130,7 @@ static const struct virtio_config_ops virtio_uml_config_ops = {
 	.reset = vu_reset,
 	.find_vqs = vu_find_vqs,
 	.del_vqs = vu_del_vqs,
+	.synchronize_cbs = vu_synchronize_cbs,
 	.get_features = vu_get_features,
 	.finalize_features = vu_finalize_features,
 	.bus_name = vu_bus_name,
diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
index 3c6408581373..c260e3a1544e 100644
--- a/drivers/platform/mellanox/mlxbf-tmfifo.c
+++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
@@ -1135,6 +1135,19 @@ static void mlxbf_tmfifo_virtio_reset(struct virtio_device *vdev)
 	tm_vdev->status = 0;
 }
 
+static void mlxbf_tmfifo_virtio_synchronize_cbs(struct virtio_device *vdev)
+{
+	struct mlxbf_tmfifo_vdev *tm_vdev = mlxbf_vdev_to_tmfifo(vdev);
+	struct mlxbf_tmfifo *fifo = tm_vdev->vrings[0].fifo;
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(fifo->spin_lock); i++) {
+		spin_lock_irqsave(&fifo->spin_lock[i], flags);
+		spin_unlock_irqrestore(&fifo->spin_lock[i], flags);
+	}
+}
+
 /* Read the value of a configuration field. */
 static void mlxbf_tmfifo_virtio_get(struct virtio_device *vdev,
 				    unsigned int offset,
@@ -1179,6 +1192,7 @@ static const struct virtio_config_ops mlxbf_tmfifo_virtio_config_ops = {
 	.find_vqs = mlxbf_tmfifo_virtio_find_vqs,
 	.del_vqs = mlxbf_tmfifo_virtio_del_vqs,
 	.reset = mlxbf_tmfifo_virtio_reset,
+	.synchronize_cbs = mlxbf_tmfifo_virtio_synchronize_cbs,
 	.set_status = mlxbf_tmfifo_virtio_set_status,
 	.get_status = mlxbf_tmfifo_virtio_get_status,
 	.get = mlxbf_tmfifo_virtio_get,
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 1ed406714849..1b139d25ab2b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2410,6 +2410,7 @@ static void rproc_type_release(struct device *dev)
 
 	dev_info(&rproc->dev, "releasing %s\n", rproc->name);
 
+	cleanup_srcu_struct(&rproc->vq_srcu);
 	idr_destroy(&rproc->notifyids);
 
 	if (rproc->index >= 0)
@@ -2507,6 +2508,11 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	if (!rproc)
 		return NULL;
 
+	if (init_srcu_struct(&rproc->vq_srcu)) {
+		kfree(rproc);
+		return NULL;
+	}
+
 	rproc->priv = &rproc[1];
 	rproc->auto_boot = true;
 	rproc->elf_class = ELFCLASSNONE;
@@ -2571,6 +2577,8 @@ EXPORT_SYMBOL(rproc_alloc);
  *
  * If no one holds any reference to rproc anymore, then its refcount would
  * now drop to zero, and it would be freed.
+ *
+ * Context: May sleep if this drops the last reference.
  */
 void rproc_free(struct rproc *rproc)
 {
@@ -2586,6 +2594,8 @@ EXPORT_SYMBOL(rproc_free);
  *
  * If no one holds any reference to rproc anymore, then its refcount would
  * now drop to zero, and it would be freed.
+ *
+ * Context: May sleep if this drops the last reference.
  */
 void rproc_put(struct rproc *rproc)
 {
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index d5e9ff045a28..ecc022e354db 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -23,6 +23,7 @@
 #include <linux/err.h>
 #include <linux/kref.h>
 #include <linux/slab.h>
+#include <linux/srcu.h>
 
 #include "remoteproc_internal.h"
 
@@ -89,14 +90,19 @@ static bool rproc_virtio_notify(struct virtqueue *vq)
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
 {
 	struct rproc_vring *rvring;
+	int srcu_idx;
+	irqreturn_t ret;
+
+	srcu_idx = srcu_read_lock(&rproc->vq_srcu);
 
 	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
 
 	rvring = idr_find(&rproc->notifyids, notifyid);
-	if (!rvring || !rvring->vq)
-		return IRQ_NONE;
+	ret = rvring && rvring->vq ? vring_interrupt(0, rvring->vq) : IRQ_NONE;
+
+	srcu_read_unlock(&rproc->vq_srcu, srcu_idx);
 
-	return vring_interrupt(0, rvring->vq);
+	return ret;
 }
 EXPORT_SYMBOL(rproc_vq_interrupt);
 
@@ -242,6 +248,13 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
 	dev_dbg(&vdev->dev, "reset !\n");
 }
 
+static void rproc_virtio_synchronize_cbs(struct virtio_device *vdev)
+{
+	struct rproc *rproc = vdev_to_rproc(vdev);
+
+	synchronize_srcu(&rproc->vq_srcu);
+}
+
 /* provide the vdev features as retrieved from the firmware */
 static u64 rproc_virtio_get_features(struct virtio_device *vdev)
 {
@@ -330,6 +343,7 @@ static const struct virtio_config_ops rproc_virtio_config_ops = {
 	.find_vqs	= rproc_virtio_find_vqs,
 	.del_vqs	= rproc_virtio_del_vqs,
 	.reset		= rproc_virtio_reset,
+	.synchronize_cbs = rproc_virtio_synchronize_cbs,
 	.set_status	= rproc_virtio_set_status,
 	.get_status	= rproc_virtio_get_status,
 	.get		= rproc_virtio_get,
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index de2af696de6c..4f9e70c1332e 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -27,6 +27,7 @@
 struct virtio_vdpa_device {
 	struct virtio_device vdev;
 	struct vdpa_device *vdpa;
+	rwlock_t callback_lock;
 	u64 features;
 };
 
@@ -123,8 +124,24 @@ static irqreturn_t virtio_vdpa_config_cb(void *private)
 static irqreturn_t virtio_vdpa_virtqueue_cb(void *private)
 {
 	struct virtqueue *vq = private;
+	struct virtio_vdpa_device *vd_dev;
+	unsigned long flags;
+	irqreturn_t ret;
 
-	return vring_interrupt(0, vq);
+	vd_dev = to_virtio_vdpa_device(vq->vdev);
+	read_lock_irqsave(&vd_dev->callback_lock, flags);
+	ret = vring_interrupt(0, vq);
+	read_unlock_irqrestore(&vd_dev->callback_lock, flags);
+
+	return ret;
+}
+
+static void virtio_vdpa_synchronize_cbs(struct virtio_device *vdev)
+{
+	struct virtio_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev);
+
+	write_lock_irq(&vd_dev->callback_lock);
+	write_unlock_irq(&vd_dev->callback_lock);
 }
 
 static struct virtqueue *
@@ -439,6 +456,7 @@ static const struct virtio_config_ops virtio_vdpa_config_ops = {
 	.reset		= virtio_vdpa_reset,
 	.find_vqs	= virtio_vdpa_find_vqs,
 	.del_vqs	= virtio_vdpa_del_vqs,
+	.synchronize_cbs = virtio_vdpa_synchronize_cbs,
 	.get_features	= virtio_vdpa_get_features,
 	.finalize_features = virtio_vdpa_finalize_features,
 	.bus_name	= virtio_vdpa_bus_name,
@@ -472,6 +490,7 @@ static int virtio_vdpa_probe(struct vdpa_device *vdpa)
 	vd_dev->vdev.config = &virtio_vdpa_config_ops;
 	vd_dev->vdev.map = vdpa->map;
 	vd_dev->vdpa = vdpa;
+	rwlock_init(&vd_dev->callback_lock);
 
 	vd_dev->vdev.id.device = ops->get_device_id(vdpa);
 	if (vd_dev->vdev.id.device == 0)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index a44368737b39..ad6bccbdfabc 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
+#include <linux/srcu.h>
 #include <linux/virtio.h>
 #include <linux/cdev.h>
 #include <linux/completion.h>
@@ -230,6 +231,7 @@ enum rproc_features {
  * @rvdevs: list of remote virtio devices
  * @subdevs: list of subdevices, to following the running state
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
+ * @vq_srcu: SRCU domain for virtqueue callbacks
  * @index: index of this rproc device
  * @attach_work: workqueue for attaching rproc
  * @crash_handler: workqueue for handling a crash
@@ -276,6 +278,7 @@ struct rproc {
 	struct list_head rvdevs;
 	struct list_head subdevs;
 	struct idr notifyids;
+	struct srcu_struct vq_srcu;
 	int index;
 	struct work_struct attach_work;
 	struct work_struct crash_handler;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 3/3] virtio: implement synchronize_cbs for remaining transports

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] `virtio_vdpa_synchronize_cbs` fails to synchronize with the configuration callback, bypassing the synchronization mechanism entirely.

Pre-existing issues:
- [High] In remoteproc, virtqueues are freed without waiting for concurrent SRCU readers to finish, violating RCU-001 and leading to a Use-After-Free. Additionally, a TOCTOU vulnerability in `rproc_vq_interrupt` allows a NULL pointer dereference.
- [High] In TmFIFO, virtqueues are freed concurrently with an active background workqueue, leading to Use-After-Free. The synchronization added is inherently flawed and fails to wait for the workqueue.
--

commit 9ddf2954e77ee0736af8a3f1e37367f20723435b
Author: Karl Mehltretter <kmehltretter@gmail.com>

virtio: implement synchronize_cbs for remaining transports

virtio_reset_device() now calls synchronize_cbs to wait for running
callbacks. Its synchronize_rcu() fallback does not cover workqueue or
sleepable callbacks.

> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index 3c6408581373b..c260e3a1544ea 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
[ ... ]
> @@ -1135,6 +1135,19 @@ static void mlxbf_tmfifo_virtio_reset(struct virtio_device *vdev)
>  	tm_vdev->status = 0;
>  }
>  
> +static void mlxbf_tmfifo_virtio_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct mlxbf_tmfifo_vdev *tm_vdev = mlxbf_vdev_to_tmfifo(vdev);
> +	struct mlxbf_tmfifo *fifo = tm_vdev->vrings[0].fifo;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fifo->spin_lock); i++) {
> +		spin_lock_irqsave(&fifo->spin_lock[i], flags);
> +		spin_unlock_irqrestore(&fifo->spin_lock[i], flags);
> +	}
> +}

[Severity: High]
This is a pre-existing issue, but does this sequential locking actually
synchronize anything?  Acquiring and releasing locks one by one creates a race
window where callbacks can evade synchronization.

Additionally, does this fail to synchronize with the background workqueue
(fifo->work)?  The workqueue function mlxbf_tmfifo_rxtx_one_desc()
dereferences vring->vq outside of the spinlocks.

When mlxbf_tmfifo_virtio_del_vqs() clears and frees the virtqueue, there is
a use-after-free risk because we haven't flushed the workqueue or properly
synchronized here.

[ ... ]
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index d5e9ff045a28a..ecc022e354db1 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
[ ... ]
> @@ -89,14 +90,19 @@ static bool rproc_virtio_notify(struct virtqueue *vq)
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
>  {
>  	struct rproc_vring *rvring;
> +	int srcu_idx;
> +	irqreturn_t ret;
> +
> +	srcu_idx = srcu_read_lock(&rproc->vq_srcu);
>  
>  	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
>  
>  	rvring = idr_find(&rproc->notifyids, notifyid);
> -	if (!rvring || !rvring->vq)
> -		return IRQ_NONE;
> +	ret = rvring && rvring->vq ? vring_interrupt(0, rvring->vq) : IRQ_NONE;

[Severity: High]
This is a pre-existing issue, but doesn't this double evaluation of
rvring->vq introduce a Time-of-Check to Time-of-Use (TOCTOU) vulnerability?

Because READ_ONCE() is not used, the compiler could reload a NULL pointer if
__rproc_virtio_del_vqs() clears rvring->vq concurrently, leading to a NULL
pointer dereference when calling vring_interrupt().

> +
> +	srcu_read_unlock(&rproc->vq_srcu, srcu_idx);
>  
> -	return vring_interrupt(0, rvring->vq);
> +	return ret;
>  }
>  EXPORT_SYMBOL(rproc_vq_interrupt);
>  
[ ... ]
> @@ -242,6 +248,13 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
>  	dev_dbg(&vdev->dev, "reset !\n");
>  }
>  
> +static void rproc_virtio_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct rproc *rproc = vdev_to_rproc(vdev);
> +
> +	synchronize_srcu(&rproc->vq_srcu);
> +}

[Severity: High]
This is a pre-existing issue, but virtqueues are freed without waiting for
concurrent SRCU readers to finish, violating the RCU-001 rule (Remove Before
Reclaim).

synchronize_srcu() is called here during device reset, but not during
teardown.  In __rproc_virtio_del_vqs(), the virtqueue is removed and
immediately freed via vring_del_virtqueue() without a grace period.

Since the interrupt handler runs in an SRCU read-side critical section, can
this cause a use-after-free if vring_interrupt() executes on freed memory?

[ ... ]
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index de2af696de6ca..4f9e70c1332ef 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
[ ... ]
> @@ -123,8 +124,24 @@ static irqreturn_t virtio_vdpa_config_cb(void *private)
>  static irqreturn_t virtio_vdpa_virtqueue_cb(void *private)
>  {
>  	struct virtqueue *vq = private;
> +	struct virtio_vdpa_device *vd_dev;
> +	unsigned long flags;
> +	irqreturn_t ret;
>  
> -	return vring_interrupt(0, vq);
> +	vd_dev = to_virtio_vdpa_device(vq->vdev);
> +	read_lock_irqsave(&vd_dev->callback_lock, flags);
> +	ret = vring_interrupt(0, vq);
> +	read_unlock_irqrestore(&vd_dev->callback_lock, flags);
> +
> +	return ret;
> +}

[Severity: Medium]
While virtio_vdpa_virtqueue_cb() was updated to acquire callback_lock,
virtio_vdpa_config_cb() (just above this context in the file) was not.

Doesn't this mean virtio_vdpa_synchronize_cbs() fails to synchronize with
configuration changes, bypassing the synchronization mechanism if a config
change occurs concurrently with device reset or teardown?

> +
> +static void virtio_vdpa_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct virtio_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev);
> +
> +	write_lock_irq(&vd_dev->callback_lock);
> +	write_unlock_irq(&vd_dev->callback_lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905152059.89560-1-kmehltretter@gmail.com?part=3