[PATCH v3 0/6] virtio: fix and add callback synchronization hooks

[PATCH v3 0/6] virtio: fix and add callback synchronization hooks

[Karl Mehltretter]

This is v3 of the callback synchronization series. It is based on
Michael S. Tsirkin's linux-next branch at f49e6cf91942 ("virtio:
synchronize callbacks after device reset"), which already contains the
core change and the virtio_input teardown reorder from v2.

Patches 1 and 2 fix two bugs in virtio-ccw's existing
synchronize_cbs() hook. After a fallback from adapter to classic
interrupts it selects the wrong lock, and the classic interrupt handler
only takes the matching lock when notification hardening is enabled.
Patch 3 adds SRCU tracking for remoteproc callbacks, which can sleep.
Patches 4 and 5 replace the RCU fallback with synchronization against
the UML IRQ and the TmFIFO callback locks. The fallback already covers
these IRQ handlers and spinlock sections. Patch 6 adds SRCU tracking
in virtio_vdpa so callback synchronization does not depend on the
context in which the vDPA driver invokes the callback.

The UML, TmFIFO and remoteproc reset paths do not themselves prevent
new virtqueue callbacks. UML and TmFIFO only clear a status field, and
remoteproc does not wait for the remote side to acknowledge the reset.
Without notification hardening or driver-specific teardown protection,
callbacks can still start after reset. The new synchronization hooks
do not fix that. The TmFIFO hook also does not synchronize with the
rest of the FIFO work item outside the callback locks.

The virtio_input loop change from v2 (continue instead of break, so
events the device already completed are still delivered) and the
event buffer leak are sent separately.

Changes in v3:
- Rebased on Michael's linux-next branch. Dropped the core change and
  the virtio_input patch, which are there already.
- Split the virtio-ccw fixes out of the core patch, one per bug, and
  the transport patch into one patch per transport. The CCW and TmFIFO
  changes are functionally unchanged from v2.
- Patch 2: added a Fixes tag and described the existing shutdown
  case. Removed the dependency note on the core reset change.
- remoteproc: read the queue pointer once in rproc_vq_interrupt(), and
  synchronize with callbacks in __rproc_virtio_del_vqs() before
  freeing the queues (Sashiko). Place vq_srcu next to rvdevs so the
  hunk also applies to mainline, which added attach_work after index.
- virtio_uml: compare against UM_IRQ_ALLOC instead of a bare negative
  check.
- virtio_vdpa: use SRCU instead of a per-device rwlock, so the
  callback tracking uses per-CPU counters, and cover the config
  callback (Sashiko).
- Rewrote the commit messages. Corrected the RCU fallback description
  for UML, TmFIFO and the vDPA simulators. Dropped the claimed simulator
  reset race: the simulators disable bottom halves around virtqueue
  callbacks and serialize reset with the worker's mutex.

Changes in v2:
- Moved callback synchronization from virtio-pci into the core, as
  Michael suggested, and added the missing synchronize_cbs() hooks.

Testing: the changed objects build with W=1 without warnings, with
clang on arm64, x86-64 and s390 and with gcc on SMP UML, and the
patches also apply to current mainline and linux-next. The runtime
tests from v2 were not repeated on this version: the input, rebind and
shutdown checks on arm64 MMIO and x86 PCI covered code that is
unchanged here, and the remoteproc and virtio_vdpa hooks have only
been build-tested. No remoteproc, TmFIFO or s390 hardware was
available.

v2: https://lore.kernel.org/r/20260905152059.89560-1-kmehltretter@gmail.com
v1: https://lore.kernel.org/r/20260818040433.66986-1-kmehltretter@gmail.com

Karl Mehltretter (6):
  virtio_ccw: fix synchronize_cbs() after interrupt fallback
  virtio_ccw: always take irq_lock in the classic interrupt handler
  remoteproc: implement synchronize_cbs() for virtio devices
  um: virtio_uml: implement synchronize_cbs()
  platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()
  virtio_vdpa: implement synchronize_cbs()

 arch/um/drivers/virtio_uml.c             | 10 +++++++
 drivers/platform/mellanox/mlxbf-tmfifo.c | 15 ++++++++++
 drivers/remoteproc/remoteproc_core.c     | 12 ++++++++
 drivers/remoteproc/remoteproc_virtio.c   | 37 +++++++++++++++++++-----
 drivers/s390/virtio/virtio_ccw.c         |  6 +---
 drivers/virtio/virtio_vdpa.c             | 34 ++++++++++++++++++++--
 include/linux/remoteproc.h               |  3 ++
 7 files changed, 102 insertions(+), 15 deletions(-)


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

[PATCH v3 1/6] virtio_ccw: fix synchronize_cbs() after interrupt fallback

[Karl Mehltretter]

virtio_ccw_synchronize_cbs() assumes the device uses adapter interrupts
when vcdev->airq_info is not NULL. If registering adapter indicators
fails, virtio_ccw_find_vqs() falls back to classic interrupts but can
leave airq_info set.

With VIRTIO_HARDEN_NOTIFICATION enabled,
virtio_ccw_int_handler() invokes callbacks under vcdev->irq_lock.
synchronize_cbs() instead takes the indicator area lock, so it does
not synchronize with those callbacks.

Use vcdev->is_thinint, which the fallback clears, to select the lock.
Keep the airq_info check because is_thinint can be set before the
first find_vqs() call, when airq_info is still NULL.

Fixes: 3a232277c1ac ("virtio-ccw: implement synchronize_cbs()")
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/s390/virtio/virtio_ccw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index bab6cad3fd5c..b90299205698 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()
-- 
2.39.5 (Apple Git-154)

[PATCH v3 2/6] virtio_ccw: always take irq_lock in the classic interrupt handler

[Karl Mehltretter]

virtio_ccw_int_handler() takes vcdev->irq_lock around its
vring_interrupt() calls only when VIRTIO_HARDEN_NOTIFICATION is enabled.

The virtio shutdown path calls synchronize_cbs() even when notification
hardening is disabled. virtio_reset_device() now does so after the
transport reset too.

For classic interrupts, synchronize_cbs() takes vcdev->irq_lock for
writing, but the interrupt handler does not take the matching read
lock in that configuration. synchronize_cbs() can return while a
callback is still running.

Take the read lock unconditionally so the write side can synchronize
with the callbacks.

Fixes: 8bd2fa086a04 ("virtio: break and reset virtio devices on device_shutdown()")
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/s390/virtio/virtio_ccw.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index b90299205698..552d77998012 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -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));
-- 
2.39.5 (Apple Git-154)

[PATCH v3 3/6] remoteproc: implement synchronize_cbs() for virtio devices

[Karl Mehltretter]

Platform drivers invoke rproc_vq_interrupt() from hard-IRQ handlers,
threaded handlers, and work items. Because rpmsg callbacks may sleep,
the virtio core's synchronize_rcu() fallback does not synchronize with
callbacks across all these contexts. A device reset can therefore
complete while a callback is still running.

Add an SRCU domain per rproc. Protect both the queue lookup and
vring_interrupt() with it, and synchronize the domain in the new hook.

__rproc_virtio_del_vqs() can race with rproc_vq_interrupt() too. Clear
all queue pointers and synchronize the SRCU domain before freeing the
queues, so callers that already found a queue can finish using it.
Read rvring->vq once to avoid a second load after deletion starts.

The SRCU domain has the same lifetime as struct rproc. Its cleanup can
sleep, so document that rproc_free() and rproc_put() must not drop the
last reference from atomic context.

Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/remoteproc/remoteproc_core.c   | 12 ++++++++
 drivers/remoteproc/remoteproc_virtio.c | 37 +++++++++++++++++++++-----
 include/linux/remoteproc.h             |  3 +++
 3 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f003be006b1b..6756f2fe4ec5 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2367,6 +2367,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)
@@ -2464,6 +2465,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;
@@ -2526,6 +2532,9 @@ 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: Any context, but the last reference must not be dropped from
+ * atomic context.
  */
 void rproc_free(struct rproc *rproc)
 {
@@ -2541,6 +2550,9 @@ 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: Any context, but the last reference must not be dropped from
+ * atomic context.
  */
 void rproc_put(struct rproc *rproc)
 {
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index d5e9ff045a28..7fefb4bd7adc 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"
 
@@ -88,15 +89,23 @@ static bool rproc_virtio_notify(struct virtqueue *vq)
  */
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
 {
+	irqreturn_t ret = IRQ_NONE;
 	struct rproc_vring *rvring;
+	struct virtqueue *vq;
+	int idx;
 
 	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
 
+	idx = srcu_read_lock(&rproc->vq_srcu);
+
 	rvring = idr_find(&rproc->notifyids, notifyid);
-	if (!rvring || !rvring->vq)
-		return IRQ_NONE;
+	vq = rvring ? READ_ONCE(rvring->vq) : NULL;
+	if (vq)
+		ret = vring_interrupt(0, vq);
 
-	return vring_interrupt(0, rvring->vq);
+	srcu_read_unlock(&rproc->vq_srcu, idx);
+
+	return ret;
 }
 EXPORT_SYMBOL(rproc_vq_interrupt);
 
@@ -153,7 +162,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 
 	vq->num_max = num;
 
-	rvring->vq = vq;
+	WRITE_ONCE(rvring->vq, vq);
 	vq->priv = rvring;
 
 	/* Update vring in resource table */
@@ -165,14 +174,20 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 
 static void __rproc_virtio_del_vqs(struct virtio_device *vdev)
 {
+	struct rproc *rproc = vdev_to_rproc(vdev);
 	struct virtqueue *vq, *n;
 	struct rproc_vring *rvring;
 
-	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
+	list_for_each_entry(vq, &vdev->vqs, list) {
 		rvring = vq->priv;
-		rvring->vq = NULL;
-		vring_del_virtqueue(vq);
+		WRITE_ONCE(rvring->vq, NULL);
 	}
+
+	/* Synchronize with rproc_vq_interrupt() callers that found a queue. */
+	synchronize_srcu(&rproc->vq_srcu);
+
+	list_for_each_entry_safe(vq, n, &vdev->vqs, list)
+		vring_del_virtqueue(vq);
 }
 
 static void rproc_virtio_del_vqs(struct virtio_device *vdev)
@@ -242,6 +257,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 +352,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/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 7c1546d48008..93a182b1868a 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -41,6 +41,7 @@
 #include <linux/cdev.h>
 #include <linux/completion.h>
 #include <linux/idr.h>
+#include <linux/srcu.h>
 #include <linux/of.h>
 #include <linux/rsc_table.h>
 
@@ -256,6 +257,7 @@ enum rproc_features {
  * @mappings: list of iommu mappings we initiated, needed on shutdown
  * @bootaddr: address of first instruction to boot rproc with (optional)
  * @rvdevs: list of remote virtio devices
+ * @vq_srcu: SRCU domain for the virtqueue callbacks of @rvdevs
  * @subdevs: list of subdevices, to following the running state
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
  * @index: index of this rproc device
@@ -298,6 +300,7 @@ struct rproc {
 	struct list_head mappings;
 	u64 bootaddr;
 	struct list_head rvdevs;
+	struct srcu_struct vq_srcu;
 	struct list_head subdevs;
 	struct idr notifyids;
 	int index;
-- 
2.39.5 (Apple Git-154)

[PATCH v3 4/6] um: virtio_uml: implement synchronize_cbs()

[Karl Mehltretter]

virtio_uml invokes virtqueue callbacks from vu_interrupt() and
vu_req_interrupt(), which share vu_dev->irq. The core's
synchronize_rcu() fallback already covers these IRQ handlers.

Implement synchronize_cbs() with synchronize_irq() to synchronize with
that IRQ instead of waiting for an RCU grace period.

Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 arch/um/drivers/virtio_uml.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
index 7425a8548141..1f2e62e29a27 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 != UM_IRQ_ALLOC)
+		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,
-- 
2.39.5 (Apple Git-154)

[PATCH v3 5/6] platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()

[Karl Mehltretter]

TmFIFO invokes virtqueue callbacks from its FIFO work item under
spin_lock_irqsave(). The core's synchronize_rcu() fallback already
covers those sections.

Implement synchronize_cbs() using the existing per-direction locks
instead of waiting for an RCU grace period. fifo->spin_lock[0] covers
the transmit callbacks, including the callback from the timeout
handler. fifo->spin_lock[1] covers the receive callbacks. Take and
release each lock in turn to synchronize with callbacks running
under that lock.

Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/platform/mellanox/mlxbf-tmfifo.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
index 3c6408581373..664bec20a7ae 100644
--- a/drivers/platform/mellanox/mlxbf-tmfifo.c
+++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
@@ -1135,6 +1135,20 @@ static void mlxbf_tmfifo_virtio_reset(struct virtio_device *vdev)
 	tm_vdev->status = 0;
 }
 
+/* Synchronize with callbacks running in the FIFO work item. */
+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 +1193,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,
-- 
2.39.5 (Apple Git-154)

[PATCH v3 6/6] virtio_vdpa: implement synchronize_cbs()

[Karl Mehltretter]

virtio_vdpa relies on the core's synchronize_rcu() fallback for callback
synchronization. That covers IRQ handlers and sections with interrupts,
preemption or bottom halves disabled. virtio_vdpa does not enforce any
of those contexts when a vDPA driver invokes a callback.

Add an SRCU domain per device and enter it around the virtqueue and
config callbacks. Implement synchronize_cbs() with synchronize_srcu()
so it covers the callbacks regardless of the calling context.

SRCU uses per-CPU reader accounting, avoiding a single callback lock
shared by all queues. Cover the config callback too, as virtio-pci
does for its config vector.

Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/virtio/virtio_vdpa.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index 6bcf4567a929..f9559b6b2e0b 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/srcu.h>
 #include <linux/uuid.h>
 #include <linux/group_cpus.h>
 #include <linux/virtio.h>
@@ -27,6 +28,7 @@
 struct virtio_vdpa_device {
 	struct virtio_device vdev;
 	struct vdpa_device *vdpa;
+	struct srcu_struct cb_srcu;
 	u64 features;
 };
 
@@ -114,8 +116,11 @@ static bool virtio_vdpa_notify_with_data(struct virtqueue *vq)
 static irqreturn_t virtio_vdpa_config_cb(void *private)
 {
 	struct virtio_vdpa_device *vd_dev = private;
+	int idx;
 
+	idx = srcu_read_lock(&vd_dev->cb_srcu);
 	virtio_config_changed(&vd_dev->vdev);
+	srcu_read_unlock(&vd_dev->cb_srcu, idx);
 
 	return IRQ_HANDLED;
 }
@@ -123,8 +128,22 @@ 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 = to_virtio_vdpa_device(vq->vdev);
+	irqreturn_t ret;
+	int idx;
+
+	idx = srcu_read_lock(&vd_dev->cb_srcu);
+	ret = vring_interrupt(0, vq);
+	srcu_read_unlock(&vd_dev->cb_srcu, idx);
+
+	return ret;
+}
+
+static void virtio_vdpa_synchronize_cbs(struct virtio_device *vdev)
+{
+	struct virtio_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev);
 
-	return vring_interrupt(0, vq);
+	synchronize_srcu(&vd_dev->cb_srcu);
 }
 
 static struct virtqueue *
@@ -440,6 +459,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,
@@ -454,6 +474,7 @@ static void virtio_vdpa_release_dev(struct device *_d)
 	struct virtio_vdpa_device *vd_dev =
 	       container_of(vdev, struct virtio_vdpa_device, vdev);
 
+	cleanup_srcu_struct(&vd_dev->cb_srcu);
 	kfree(vd_dev);
 }
 
@@ -467,6 +488,11 @@ static int virtio_vdpa_probe(struct vdpa_device *vdpa)
 	if (!vd_dev)
 		return -ENOMEM;
 
+	if (init_srcu_struct(&vd_dev->cb_srcu)) {
+		kfree(vd_dev);
+		return -ENOMEM;
+	}
+
 	vd_dev->vdev.dev.parent = vdpa->map ? &vdpa->dev :
 				  vdpa_get_map(vdpa).dma_dev;
 	vd_dev->vdev.dev.release = virtio_vdpa_release_dev;
@@ -489,10 +515,12 @@ static int virtio_vdpa_probe(struct vdpa_device *vdpa)
 	return 0;
 
 err:
-	if (reg_dev)
+	if (reg_dev) {
 		put_device(&vd_dev->vdev.dev);
-	else
+	} else {
+		cleanup_srcu_struct(&vd_dev->cb_srcu);
 		kfree(vd_dev);
+	}
 	return ret;
 }
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v3 1/6] virtio_ccw: fix synchronize_cbs() after interrupt fallback

[sashiko-bot]

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

Pre-existing issues:
- [Critical] virtio_ccw_reset() silently fails to reset the device if ccw allocation fails, risking use-after-free and DMA corruption.
- [High] virtio_ccw_del_vqs silently aborts on memory allocation failure, permanently leaking all virtqueues and their DMA memory.
- [High] Silent allocation failure in virtio_ccw_drop_indicator leaves stale virtqueue pointers in global routing tables, leading to UAF when adapter interrupts fire.
--

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

virtio_ccw: fix synchronize_cbs() after interrupt fallback

virtio_ccw_synchronize_cbs() assumes the device uses adapter interrupts
when vcdev->airq_info is not NULL. If registering adapter indicators
fails, virtio_ccw_find_vqs() falls back to classic interrupts but can
leave airq_info set. This patch uses vcdev->is_thinint to properly
select the lock.

> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index bab6cad3fd5ce..b90299205698b 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()

[ ... ]

> static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> 				      struct ccw1 *ccw)
> {
> 	int ret;
> 	struct virtio_thinint_area *thinint_area = NULL;
> 	struct airq_info *airq_info = vcdev->airq_info;
> 	dma64_t *indicatorp = NULL;
> 
> 	if (vcdev->is_thinint) {
> 		thinint_area = ccw_device_dma_zalloc(vcdev->cdev,
> 						     sizeof(*thinint_area),
> 						     &ccw->cda);
> 		if (!thinint_area)
> 			return;

[Severity: High]
This is a pre-existing issue, but returning early here on allocation failure
skips deregistering indicators and removing stale pointers from the global
airq_areas array.

If ccw_device_dma_zalloc() fails, the caller virtio_ccw_del_vqs() continues
and frees all virtqueues, while the global airq_areas array retains pointers
to them.

A subsequent adapter interrupt in virtio_airq_handler() could then fetch the
stale pointer:

	vring_interrupt(0, (void *)airq_iv_get_ptr(info->aiv, ai));

and trigger a use-after-free on the freed virtqueue memory.

Does this early return need to gracefully handle indicator deregistration
when allocation fails?

[ ... ]

> static void virtio_ccw_del_vqs(struct virtio_device *vdev)
> {
> 	struct virtqueue *vq, *n;
> 	struct ccw1 *ccw;
> 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> 
> 	ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw), NULL);
> 	if (!ccw)
> 		return;

[Severity: High]
This isn't a bug introduced by this patch, but returning early on allocation
failure skips the teardown loop entirely.

This leaks all virtqueues and their DMA memory. Since the virtqueues are not
deregistered from the host or removed from the global airq_areas routing
array, the virtio core will subsequently free the parent virtio_ccw_device.

A later adapter interrupt could invoke vring_interrupt() on the leaked
virtqueue, holding a stale pointer to the freed device and potentially
causing a use-after-free.

Should this fallback to a safe teardown path if ccw allocation fails?

[ ... ]

> static void virtio_ccw_reset(struct virtio_device *vdev)
> {
> 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> 	struct ccw1 *ccw;
> 
> 	ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw), NULL);
> 	if (!ccw)
> 		return;

[Severity: Critical]
This is a pre-existing issue, but this silent return bypasses sending the
CCW_CMD_VDEV_RESET command to the host if allocation fails.

The virtio core assumes the device is fully quiesced after virtio_ccw_reset()
returns and proceeds to free virtqueues and data buffers. Since the device on
the host was never actually reset, the hardware state machine remains active
and may continue to execute pending DMA operations into the now-freed memory,
causing critical data corruption.

How should we ensure the device is safely quiesced if ccw allocation fails?

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

Re: [PATCH v3 4/6] um: virtio_uml: implement synchronize_cbs()

[sashiko-bot]

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

Pre-existing issues:
- [High] Premature clearing of dynamically allocated IRQ bit in um_free_irq() causes unintended IRQ sharing and race conditions.
- [High] Resource leak and potential Use-After-Free on vhost_user_init() error path.
--

commit 88d671cc22ee3a67541ca7191122aa3e1f6d8c9f
Author: Karl Mehltretter <kmehltretter@gmail.com>

um: virtio_uml: implement synchronize_cbs()

This commit implements synchronize_cbs() for virtio_uml by using
synchronize_irq() instead of waiting for an RCU grace period. This
synchronizes with the IRQ handlers used by virtqueue callbacks.

> diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> index 7425a8548141a..1f2e62e29a277 100644
> --- a/arch/um/drivers/virtio_uml.c
> +++ b/arch/um/drivers/virtio_uml.c

[ ... ]

> @@ -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 != UM_IRQ_ALLOC)
> +		synchronize_irq(vu_dev->irq);
> +}
> +
>  static void vu_del_vq(struct virtqueue *vq)
>  {
>  	struct virtio_uml_vq_info *info = vq->priv;

[Severity: High]
This isn't a bug introduced by this patch, but it is a pre-existing issue in
the codebase. 

When a virtqueue is deleted during teardown, vu_del_vq() calls
um_free_irq(vu_dev->irq, vq) to remove the callback for that specific queue. 

However, looking at um_free_irq() in arch/um/kernel/irq.c, it lacks reference
counting and unconditionally clears the dynamically allocated IRQ bit:

arch/um/kernel/irq.c:um_free_irq() {
	free_irq(irq, dev);
	clear_bit(irq, irqs_allocated);
}

Since virtio_uml shares a single dynamically allocated IRQ (vu_dev->irq)
across multiple virtqueues and the slave request channel using IRQF_SHARED,
could this premature clearing of the IRQ bit mark it as free for global
reallocation while still in use? 

It seems concurrent requests for a new dynamic IRQ from other devices could be
assigned this prematurely freed IRQ, resulting in unintended forced sharing or
-EBUSY probe failures.

[ ... ]

[Severity: High]
This is also a pre-existing issue, but there appears to be a resource leak
and potential use-after-free in the probe error path of virtio_uml.

In virtio_uml_probe(), vhost_user_init() is called. If the slave request
channel is negotiated, vhost_user_init_slave_req() creates a pipe, assigns
req_fds[0] to vu_dev->req_fd, and requests an IRQ handler via
um_request_irq_tt():

arch/um/drivers/virtio_uml.c:vhost_user_init() {
	if (vu_dev->protocol_features &
			BIT_ULL(VHOST_USER_PROTOCOL_F_SLAVE_REQ)) {
		rc = vhost_user_init_slave_req(vu_dev);
		if (rc)
			return rc;
	}

	if (vu_dev->protocol_features &
			BIT_ULL(VHOST_USER_PROTOCOL_F_MQ)) {
		rc = vhost_user_get_queue_num(vu_dev, &vu_dev->max_vqs);
		if (rc)
			return rc;
}

If a subsequent step like vhost_user_get_queue_num() fails, it returns the
error code directly. Back in virtio_uml_probe(), the error path is taken:

arch/um/drivers/virtio_uml.c:virtio_uml_probe() {
error_init:
	os_close_file(vu_dev->sock);
error_free:
	kfree(vu_dev);
	return rc;
}

Does this error path free vu_dev without unregistering the active IRQ
handler or closing the vu_dev->req_fd file descriptor? 

If an untrusted userspace backend forces the probe to fail, could the leaked
IRQ handler subsequently fire and access the freed vu_dev memory, causing a
use-after-free?

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

Re: [PATCH v3 2/6] virtio_ccw: always take irq_lock in the classic interrupt handler

[sashiko-bot]

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

Pre-existing issues:
- [High] The virtio interrupt handlers pass retrieved virtqueue pointers directly to `vring_interrupt()` without a NULL check. A malicious or buggy host can trigger a kernel panic by providing an invalid indicator or interrupt vector.
- [High] Lost configuration change notifications due to clearing the indicator bit after processing.
--

commit 40545ed73d4381a07a7934fddf3ac60ecd1f90d7
Author: Karl Mehltretter <kmehltretter@gmail.com>
Subject: virtio_ccw: always take irq_lock in the classic interrupt handler

This commit unconditionally takes the read lock on vcdev->irq_lock in 
virtio_ccw_int_handler() so the write side can properly synchronize with
the callbacks even when notification hardening is disabled.

> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index b90299205698b..552d779980129 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -1204,13 +1204,11 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,

[ ... ]

>  	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);

[Severity: High]
This is a pre-existing issue, but is it possible for virtio_ccw_vq_by_ind()
to return a NULL virtqueue pointer if a malicious or buggy host provides an
invalid indicator?

Passing an unvalidated NULL pointer directly to vring_interrupt() can cause a
kernel panic when it is dereferenced inside more_used():

drivers/virtio/virtio_ring.c:vring_interrupt() {
    ...
    if (!more_used(vq)) {
        pr_debug("virtqueue interrupt with no work for %p\n", vq);
    ...
}

Similarly, in virtio_airq_handler(), airq_iv_get_ptr() can return NULL which
is unconditionally passed to vring_interrupt().

Could we add a NULL check before calling the interrupt handler?

>  	}
> -#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));
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but could this sequence lead to
lost configuration change notifications?

Because indicators2 is shared DMA memory updated by the host, if the host
triggers another configuration change in the window between test_bit() and
clear_bit(), it sets the bit in memory and raises a new interrupt.

However, the guest will then blindly execute clear_bit() and wipe out the
newly set notification flag. When the newly queued interrupt handler runs,
it will find the bit cleared and do nothing, permanently losing the event.

Would using test_and_clear_bit() before calling virtio_config_changed()
close this race window?

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

Re: [PATCH v3 6/6] virtio_vdpa: implement synchronize_cbs()

[sashiko-bot]

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

Pre-existing issues:
- [High] Possible Use-After-Free of virtqueue memory in callback during find_vqs error handling
--

commit 05a7e5947030bc4c942e05c96bf1d657ed426d56
Author: Karl Mehltretter <kmehltretter@gmail.com>

virtio_vdpa: implement synchronize_cbs()

virtio_vdpa relies on the core's synchronize_rcu() fallback for callback
synchronization. This commit adds an SRCU domain per device and implements
synchronize_cbs() with synchronize_srcu() to cover the callbacks regardless
of the calling context.

> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index 6bcf4567a9292..f9559b6b2e0b6 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
[ ... ]
> @@ -123,8 +128,22 @@ 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 = to_virtio_vdpa_device(vq->vdev);

[Severity: High]
This is a pre-existing issue, but could this callback be invoked
concurrently on a freed vq, leading to a use-after-free when
dereferencing vq->vdev?

During driver probe, if virtio_vdpa_find_vqs() successfully creates some
virtqueues but fails on a subsequent one, it jumps to err_setup_vq and
calls virtio_vdpa_del_vqs():

drivers/virtio/virtio_vdpa.c:virtio_vdpa_find_vqs() {
    ...
err_setup_vq:
	virtio_vdpa_del_vqs(vdev);
    ...
}

This tears down the previously created virtqueues. However, the callback is
not cleared with set_vq_cb(..., NULL), nor does it synchronize with pending
or concurrently running interrupt handlers (such as vp_vdpa_vq_handler).

Could a spurious or device-initiated interrupt occurring right before or
during vring_del_virtqueue(vq) race and cause virtio_vdpa_virtqueue_cb()
to dereference the freed vq structure when accessing vq->vdev?

> +	irqreturn_t ret;
> +	int idx;
> +
> +	idx = srcu_read_lock(&vd_dev->cb_srcu);
> +	ret = vring_interrupt(0, vq);
> +	srcu_read_unlock(&vd_dev->cb_srcu, idx);
> +
> +	return ret;
> +}
> +

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

Re: [PATCH v3 5/6] platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Device reset and `synchronize_cbs()` fail to stop the background work handler, leading to a race condition and Use-After-Free during virtqueue removal.
--

commit 7ff4c967b176976015826d77ef33286ed4bdfbba
Author: Karl Mehltretter <kmehltretter@gmail.com>

platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()

This commit implements synchronize_cbs() for the mlxbf-tmfifo driver. It uses
the existing per-direction spinlocks to synchronize with virtqueue callbacks
running in the FIFO work item instead of relying on synchronize_rcu().

> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index 3c6408581373b..664bec20a7aeb 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -1135,6 +1135,20 @@ static void mlxbf_tmfifo_virtio_reset(struct virtio_device *vdev)
>  	tm_vdev->status = 0;
>  }
>  
> +/* Synchronize with callbacks running in the FIFO work item. */
> +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: Critical]
This is a pre-existing issue, but does toggling the spinlocks here actually
guarantee that the work handler has finished and won't execute again?

If a privileged user unbinds the virtio device via sysfs while traffic is
actively processing, the driver core calls virtio_reset_device(), which
invokes mlxbf_tmfifo_virtio_reset() and mlxbf_tmfifo_virtio_synchronize_cbs().

Because mlxbf_tmfifo_virtio_reset() fails to halt or cancel the background
work item, the core proceeds to free the virtqueues in
mlxbf_tmfifo_virtio_del_vqs().

The concurrently running work handler (mlxbf_tmfifo_work_rxtx()) can then
check the virtqueue pointer outside the spinlocks and proceed to use it:

drivers/platform/mellanox/mlxbf-tmfifo.c:mlxbf_tmfifo_work_rxtx() {
...
	vring = &tm_vdev->vrings[queue_id];
	if (vring->vq)
		mlxbf_tmfifo_rxtx(vring, is_rx);
...
}

And finally dereference the freed virtqueue in mlxbf_tmfifo_rxtx(), causing a
use-after-free:

drivers/platform/mellanox/mlxbf-tmfifo.c:mlxbf_tmfifo_rxtx() {
...
	/* Notify upper layer that packet is done. */
	spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
	vring_interrupt(0, vring->vq);
	spin_unlock_irqrestore(&fifo->spin_lock[is_rx], flags);
...
}

This issue wasn't introduced by this patch, as the former synchronize_rcu()
fallback also failed to protect the non-RCU work handler, but this patch fails
to correct the vulnerability.

Should the background work item be canceled during reset to prevent this?

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

Re: [PATCH v3 3/6] remoteproc: implement synchronize_cbs() for virtio devices

[sashiko-bot]

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

Pre-existing issues:
- [High] idr_find() is called without rcu_read_lock() in preemptible contexts, leading to potential use-after-free of radix tree nodes.
- [High] The virtqueue pointer is published before its initialization is complete and without necessary memory barriers, exposing uninitialized memory to concurrent readers.
- [High] The added SRCU synchronization leaves the rvring structure vulnerable to Use-After-Free during device teardown.
- [High] Error path in rp_find_vq() erroneously removes the vring IDR entry without clearing it, permanently breaking the vring and causing a double-free on teardown.
- [High] rproc_add() error path causes a use-after-free and a double-free of the rproc structure.
- [High] Missing cancel_work_sync() for the crash handler work item before freeing the rproc struct.
--

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

remoteproc: implement synchronize_cbs() for virtio devices

Platform drivers invoke rproc_vq_interrupt() from hard-IRQ handlers,
threaded handlers, and work items. Because rpmsg callbacks may sleep,
the virtio core's synchronize_rcu() fallback does not synchronize with
callbacks across all these contexts. A device reset can therefore
complete while a callback is still running.

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f003be006b1bf..6756f2fe4ec50 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2367,6 +2367,7 @@ static void rproc_type_release(struct device *dev)
>  
>  	dev_info(&rproc->dev, "releasing %s\n", rproc->name);
>  
> +	cleanup_srcu_struct(&rproc->vq_srcu);

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of a
use-after-free here if the crash handler work item is still pending?

rproc_alloc() initializes the rproc->crash_handler work item. If the remote
processor is deleted and freed, neither rproc_del() nor rproc_type_release()
calls cancel_work_sync(&rproc->crash_handler). If the work executes after this
structure is freed, the thread will dereference freed memory.

[Severity: High]
This is a pre-existing issue, but can the error path in rproc_add() cause a
use-after-free and a subsequent double-free?

If device_add(dev) fails, the error path erroneously calls put_device(dev):

drivers/remoteproc/remoteproc_core.c:rproc_add() {
    ...
    ret = device_add(dev);
    if (ret < 0) {
        put_device(dev);
        goto rproc_remove_cdev;
    }
    ...
rproc_remove_cdev:
    rproc_char_device_remove(rproc);
    return ret;
}

Since the initial refcount is 1, put_device() drops it to 0, immediately
freeing the rproc structure. Calling rproc_char_device_remove(rproc) then
accesses the cdev within the already freed memory. Returning the error to the
caller typically results in rproc_free() being called, causing a double-free.

>  	idr_destroy(&rproc->notifyids);
>  
>  	if (rproc->index >= 0)
[ ... ]
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index d5e9ff045a28a..7fefb4bd7adcb 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
[ ... ]
> @@ -88,15 +89,23 @@ static bool rproc_virtio_notify(struct virtqueue *vq)
>   */
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
>  {
> +	irqreturn_t ret = IRQ_NONE;
>  	struct rproc_vring *rvring;
> +	struct virtqueue *vq;
> +	int idx;
>  
>  	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
>  
> +	idx = srcu_read_lock(&rproc->vq_srcu);
> +
>  	rvring = idr_find(&rproc->notifyids, notifyid);

[Severity: High]
This is a pre-existing issue, but might calling idr_find() here without
standard RCU read-side protection risk a use-after-free of internal radix tree
nodes?

This function runs in a preemptible context (threaded IRQ handler). idr_find()
relies on rcu_read_lock() for safe lockless traversal. While this patch wraps
it in srcu_read_lock(), SRCU does not block standard RCU grace periods.

If a concurrent idr_remove() runs (e.g., during device teardown) and a standard
RCU grace period completes while the thread is preempted inside idr_find(), the
internal nodes could be freed and then dereferenced when the thread resumes.

> -	if (!rvring || !rvring->vq)
> -		return IRQ_NONE;
> +	vq = rvring ? READ_ONCE(rvring->vq) : NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does the added SRCU
synchronization leave the rvring structure vulnerable to a use-after-free
during device teardown?

While rproc_vq_interrupt() is now protected by an SRCU critical section, there
does not appear to be a corresponding synchronize_srcu() in the platform device
teardown path:

drivers/remoteproc/remoteproc_virtio.c:rproc_virtio_remove() {
    ...
    for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
        rvring = &rvdev->vring[id];
        rproc_free_vring(rvring);
    }
    ...
}

The IDR entries are removed, but the function returns without waiting for SRCU
readers. The core driver then cleans up devres, instantly freeing the rvdev
(which embeds rvring). If rproc_vq_interrupt() is preempted after locating
rvring but before accessing rvring->vq, it will dereference freed memory upon
resumption.

> +	if (vq)
> +		ret = vring_interrupt(0, vq);
>  
> -	return vring_interrupt(0, rvring->vq);
> +	srcu_read_unlock(&rproc->vq_srcu, idx);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(rproc_vq_interrupt);
>  
> @@ -153,7 +162,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
>  
>  	vq->num_max = num;
>  
> -	rvring->vq = vq;
> +	WRITE_ONCE(rvring->vq, vq);
>  	vq->priv = rvring;

[Severity: High]
This isn't a bug introduced by this patch, but would publishing the virtqueue
pointer here without a release barrier expose uninitialized memory to
concurrent readers?

In vring_new_virtqueue(), vq is allocated and initialized, but vq->priv is not
set. The pointer is then published to concurrent readers via WRITE_ONCE.

Since WRITE_ONCE does not provide release semantics on weakly ordered
architectures, a concurrent rproc_vq_interrupt() could observe a non-NULL vq
before vq->priv is set or before the internal state of vq is visible. The
handler would then call vring_interrupt(0, vq) on an improperly initialized
structure.

[Severity: High]
This is a pre-existing issue, but could the error path in rp_find_vq()
permanently break the vring and cause a double-free on teardown?

If vring_new_virtqueue() fails, the error path calls rproc_free_vring(rvring),
which executes idr_remove(), but it does not reset rvring->notifyid:

drivers/remoteproc/remoteproc_virtio.c:rp_find_vq() {
    ...
    vq = vring_new_virtqueue(...);
    if (!vq) {
        dev_err(dev, "vring_new_virtqueue %s failed\n", name);
        rproc_free_vring(rvring);
        return ERR_PTR(-ENOMEM);
    }
    ...
}

If the driver successfully binds later, rp_find_vq() might succeed, but the
notifyid is no longer in the IDR, silently breaking interrupts. Furthermore,
when the device is unbound, rproc_virtio_remove() loops over all vrings and
calls rproc_free_vring() again, executing a second idr_remove() for the same
notifyid. If the ID was reused by another vring, its entry will be erroneously
deleted.

>  
>  	/* Update vring in resource table */
> @@ -165,14 +174,20 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,

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

Re: [PATCH v3 0/6] virtio: fix and add callback synchronization hooks

[Michael S. Tsirkin]

On Tue, Sep 08, 2026 at 07:38:11AM +0200, Karl Mehltretter wrote:
> This is v3 of the callback synchronization series. It is based on
> Michael S. Tsirkin's linux-next branch at f49e6cf91942 ("virtio:
> synchronize callbacks after device reset"), which already contains the
> core change and the virtio_input teardown reorder from v2.


Pls note I dropped the core change for now.
That one needs more work:

- transport changes should be separate patches from core
  changes

- shutdown generally needs more work, it should disable
  config in particular just like remove does.
 
- drivers that queue work must have a shutdown callback to
  sync it - that part can be separate patchsets




> Patches 1 and 2 fix two bugs in virtio-ccw's existing
> synchronize_cbs() hook. After a fallback from adapter to classic
> interrupts it selects the wrong lock, and the classic interrupt handler
> only takes the matching lock when notification hardening is enabled.
> Patch 3 adds SRCU tracking for remoteproc callbacks, which can sleep.
> Patches 4 and 5 replace the RCU fallback with synchronization against
> the UML IRQ and the TmFIFO callback locks. The fallback already covers
> these IRQ handlers and spinlock sections. Patch 6 adds SRCU tracking
> in virtio_vdpa so callback synchronization does not depend on the
> context in which the vDPA driver invokes the callback.
> 
> The UML, TmFIFO and remoteproc reset paths do not themselves prevent
> new virtqueue callbacks. UML and TmFIFO only clear a status field, and
> remoteproc does not wait for the remote side to acknowledge the reset.
> Without notification hardening or driver-specific teardown protection,
> callbacks can still start after reset. The new synchronization hooks
> do not fix that. The TmFIFO hook also does not synchronize with the
> rest of the FIFO work item outside the callback locks.
> 
> The virtio_input loop change from v2 (continue instead of break, so
> events the device already completed are still delivered) and the
> event buffer leak are sent separately.
> 
> Changes in v3:
> - Rebased on Michael's linux-next branch. Dropped the core change and
>   the virtio_input patch, which are there already.
> - Split the virtio-ccw fixes out of the core patch, one per bug, and
>   the transport patch into one patch per transport. The CCW and TmFIFO
>   changes are functionally unchanged from v2.
> - Patch 2: added a Fixes tag and described the existing shutdown
>   case. Removed the dependency note on the core reset change.
> - remoteproc: read the queue pointer once in rproc_vq_interrupt(), and
>   synchronize with callbacks in __rproc_virtio_del_vqs() before
>   freeing the queues (Sashiko). Place vq_srcu next to rvdevs so the
>   hunk also applies to mainline, which added attach_work after index.
> - virtio_uml: compare against UM_IRQ_ALLOC instead of a bare negative
>   check.
> - virtio_vdpa: use SRCU instead of a per-device rwlock, so the
>   callback tracking uses per-CPU counters, and cover the config
>   callback (Sashiko).
> - Rewrote the commit messages. Corrected the RCU fallback description
>   for UML, TmFIFO and the vDPA simulators. Dropped the claimed simulator
>   reset race: the simulators disable bottom halves around virtqueue
>   callbacks and serialize reset with the worker's mutex.
> 
> Changes in v2:
> - Moved callback synchronization from virtio-pci into the core, as
>   Michael suggested, and added the missing synchronize_cbs() hooks.
> 
> Testing: the changed objects build with W=1 without warnings, with
> clang on arm64, x86-64 and s390 and with gcc on SMP UML, and the
> patches also apply to current mainline and linux-next. The runtime
> tests from v2 were not repeated on this version: the input, rebind and
> shutdown checks on arm64 MMIO and x86 PCI covered code that is
> unchanged here, and the remoteproc and virtio_vdpa hooks have only
> been build-tested. No remoteproc, TmFIFO or s390 hardware was
> available.
> 
> v2: https://lore.kernel.org/r/20260905152059.89560-1-kmehltretter@gmail.com
> v1: https://lore.kernel.org/r/20260818040433.66986-1-kmehltretter@gmail.com
> 
> Karl Mehltretter (6):
>   virtio_ccw: fix synchronize_cbs() after interrupt fallback
>   virtio_ccw: always take irq_lock in the classic interrupt handler
>   remoteproc: implement synchronize_cbs() for virtio devices
>   um: virtio_uml: implement synchronize_cbs()
>   platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()
>   virtio_vdpa: implement synchronize_cbs()
> 
>  arch/um/drivers/virtio_uml.c             | 10 +++++++
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 15 ++++++++++
>  drivers/remoteproc/remoteproc_core.c     | 12 ++++++++
>  drivers/remoteproc/remoteproc_virtio.c   | 37 +++++++++++++++++++-----
>  drivers/s390/virtio/virtio_ccw.c         |  6 +---
>  drivers/virtio/virtio_vdpa.c             | 34 ++++++++++++++++++++--
>  include/linux/remoteproc.h               |  3 ++
>  7 files changed, 102 insertions(+), 15 deletions(-)
> 
> 
> base-commit: f49e6cf919425cc55f10fd7cda7e0fc895df4cc0
> -- 
> 2.39.5 (Apple Git-154)

Re: [PATCH v3 2/6] virtio_ccw: always take irq_lock in the classic interrupt handler

[Michael S. Tsirkin]

On Tue, Sep 08, 2026 at 05:51:48AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The virtio interrupt handlers pass retrieved virtqueue pointers directly to `vring_interrupt()` without a NULL check. A malicious or buggy host can trigger a kernel panic by providing an invalid indicator or interrupt vector.

Buggy host crashing guest is not something we worry about.
Host denying guest service at will is exactly how cloud providers get paid.

> - [High] Lost configuration change notifications due to clearing the indicator bit after processing.
> --
> 
> commit 40545ed73d4381a07a7934fddf3ac60ecd1f90d7
> Author: Karl Mehltretter <kmehltretter@gmail.com>
> Subject: virtio_ccw: always take irq_lock in the classic interrupt handler
> 
> This commit unconditionally takes the read lock on vcdev->irq_lock in 
> virtio_ccw_int_handler() so the write side can properly synchronize with
> the callbacks even when notification hardening is disabled.
> 
> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> > index b90299205698b..552d779980129 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -1204,13 +1204,11 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> 
> [ ... ]
> 
> >  	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);
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for virtio_ccw_vq_by_ind()
> to return a NULL virtqueue pointer if a malicious or buggy host provides an
> invalid indicator?
> 
> Passing an unvalidated NULL pointer directly to vring_interrupt() can cause a
> kernel panic when it is dereferenced inside more_used():
> 
> drivers/virtio/virtio_ring.c:vring_interrupt() {
>     ...
>     if (!more_used(vq)) {
>         pr_debug("virtqueue interrupt with no work for %p\n", vq);
>     ...
> }
> 
> Similarly, in virtio_airq_handler(), airq_iv_get_ptr() can return NULL which
> is unconditionally passed to vring_interrupt().
> 
> Could we add a NULL check before calling the interrupt handler?
> 
> >  	}
> > -#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));
> >  	}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but could this sequence lead to
> lost configuration change notifications?
> 
> Because indicators2 is shared DMA memory updated by the host, if the host
> triggers another configuration change in the window between test_bit() and
> clear_bit(), it sets the bit in memory and raises a new interrupt.
> 
> However, the guest will then blindly execute clear_bit() and wipe out the
> newly set notification flag. When the newly queued interrupt handler runs,
> it will find the bit cleared and do nothing, permanently losing the event.
> 
> Would using test_and_clear_bit() before calling virtio_config_changed()
> close this race window?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260908053817.26065-1-kmehltretter@gmail.com?part=2

Re: [PATCH v3 0/6] virtio: fix and add callback synchronization hooks

[Michael S. Tsirkin]

On Tue, Sep 08, 2026 at 07:38:11AM +0200, Karl Mehltretter wrote:
> This is v3 of the callback synchronization series. It is based on
> Michael S. Tsirkin's linux-next branch at f49e6cf91942 ("virtio:
> synchronize callbacks after device reset"), which already contains the
> core change and the virtio_input teardown reorder from v2.


Thanks I applied since this looks like a net improvement.
Pls keep iterating on the core change.
If you want to take a stab at some of the pre-existing issues -
will be welcome.
Pls do note which patches were tested and which were not.

> Patches 1 and 2 fix two bugs in virtio-ccw's existing
> synchronize_cbs() hook. After a fallback from adapter to classic
> interrupts it selects the wrong lock, and the classic interrupt handler
> only takes the matching lock when notification hardening is enabled.
> Patch 3 adds SRCU tracking for remoteproc callbacks, which can sleep.
> Patches 4 and 5 replace the RCU fallback with synchronization against
> the UML IRQ and the TmFIFO callback locks. The fallback already covers
> these IRQ handlers and spinlock sections. Patch 6 adds SRCU tracking
> in virtio_vdpa so callback synchronization does not depend on the
> context in which the vDPA driver invokes the callback.
> 
> The UML, TmFIFO and remoteproc reset paths do not themselves prevent
> new virtqueue callbacks. UML and TmFIFO only clear a status field, and
> remoteproc does not wait for the remote side to acknowledge the reset.
> Without notification hardening or driver-specific teardown protection,
> callbacks can still start after reset. The new synchronization hooks
> do not fix that. The TmFIFO hook also does not synchronize with the
> rest of the FIFO work item outside the callback locks.
> 
> The virtio_input loop change from v2 (continue instead of break, so
> events the device already completed are still delivered) and the
> event buffer leak are sent separately.
> 
> Changes in v3:
> - Rebased on Michael's linux-next branch. Dropped the core change and
>   the virtio_input patch, which are there already.
> - Split the virtio-ccw fixes out of the core patch, one per bug, and
>   the transport patch into one patch per transport. The CCW and TmFIFO
>   changes are functionally unchanged from v2.
> - Patch 2: added a Fixes tag and described the existing shutdown
>   case. Removed the dependency note on the core reset change.
> - remoteproc: read the queue pointer once in rproc_vq_interrupt(), and
>   synchronize with callbacks in __rproc_virtio_del_vqs() before
>   freeing the queues (Sashiko). Place vq_srcu next to rvdevs so the
>   hunk also applies to mainline, which added attach_work after index.
> - virtio_uml: compare against UM_IRQ_ALLOC instead of a bare negative
>   check.
> - virtio_vdpa: use SRCU instead of a per-device rwlock, so the
>   callback tracking uses per-CPU counters, and cover the config
>   callback (Sashiko).
> - Rewrote the commit messages. Corrected the RCU fallback description
>   for UML, TmFIFO and the vDPA simulators. Dropped the claimed simulator
>   reset race: the simulators disable bottom halves around virtqueue
>   callbacks and serialize reset with the worker's mutex.
> 
> Changes in v2:
> - Moved callback synchronization from virtio-pci into the core, as
>   Michael suggested, and added the missing synchronize_cbs() hooks.
> 
> Testing: the changed objects build with W=1 without warnings, with
> clang on arm64, x86-64 and s390 and with gcc on SMP UML, and the
> patches also apply to current mainline and linux-next. The runtime
> tests from v2 were not repeated on this version: the input, rebind and
> shutdown checks on arm64 MMIO and x86 PCI covered code that is
> unchanged here, and the remoteproc and virtio_vdpa hooks have only
> been build-tested. No remoteproc, TmFIFO or s390 hardware was
> available.
> 
> v2: https://lore.kernel.org/r/20260905152059.89560-1-kmehltretter@gmail.com
> v1: https://lore.kernel.org/r/20260818040433.66986-1-kmehltretter@gmail.com
> 
> Karl Mehltretter (6):
>   virtio_ccw: fix synchronize_cbs() after interrupt fallback
>   virtio_ccw: always take irq_lock in the classic interrupt handler
>   remoteproc: implement synchronize_cbs() for virtio devices
>   um: virtio_uml: implement synchronize_cbs()
>   platform/mellanox: mlxbf-tmfifo: implement synchronize_cbs()
>   virtio_vdpa: implement synchronize_cbs()
> 
>  arch/um/drivers/virtio_uml.c             | 10 +++++++
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 15 ++++++++++
>  drivers/remoteproc/remoteproc_core.c     | 12 ++++++++
>  drivers/remoteproc/remoteproc_virtio.c   | 37 +++++++++++++++++++-----
>  drivers/s390/virtio/virtio_ccw.c         |  6 +---
>  drivers/virtio/virtio_vdpa.c             | 34 ++++++++++++++++++++--
>  include/linux/remoteproc.h               |  3 ++
>  7 files changed, 102 insertions(+), 15 deletions(-)
> 
> 
> base-commit: f49e6cf919425cc55f10fd7cda7e0fc895df4cc0
> -- 
> 2.39.5 (Apple Git-154)

Re: [PATCH v3 6/6] virtio_vdpa: implement synchronize_cbs()

[Michael S. Tsirkin]

On Tue, Sep 08, 2026 at 05:51:59AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Possible Use-After-Free of virtqueue memory in callback during find_vqs error handling
> --
> 
> commit 05a7e5947030bc4c942e05c96bf1d657ed426d56
> Author: Karl Mehltretter <kmehltretter@gmail.com>
> 
> virtio_vdpa: implement synchronize_cbs()
> 
> virtio_vdpa relies on the core's synchronize_rcu() fallback for callback
> synchronization. This commit adds an SRCU domain per device and implements
> synchronize_cbs() with synchronize_srcu() to cover the callbacks regardless
> of the calling context.
> 
> > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > index 6bcf4567a9292..f9559b6b2e0b6 100644
> > --- a/drivers/virtio/virtio_vdpa.c
> > +++ b/drivers/virtio/virtio_vdpa.c
> [ ... ]
> > @@ -123,8 +128,22 @@ 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 = to_virtio_vdpa_device(vq->vdev);
> 
> [Severity: High]
> This is a pre-existing issue, but could this callback be invoked
> concurrently on a freed vq, leading to a use-after-free when
> dereferencing vq->vdev?
> 
> During driver probe, if virtio_vdpa_find_vqs() successfully creates some
> virtqueues but fails on a subsequent one, it jumps to err_setup_vq and
> calls virtio_vdpa_del_vqs():
> 
> drivers/virtio/virtio_vdpa.c:virtio_vdpa_find_vqs() {
>     ...
> err_setup_vq:
> 	virtio_vdpa_del_vqs(vdev);
>     ...
> }
> 
> This tears down the previously created virtqueues. However, the callback is
> not cleared with set_vq_cb(..., NULL), nor does it synchronize with pending
> or concurrently running interrupt handlers (such as vp_vdpa_vq_handler).
> 
> Could a spurious or device-initiated interrupt occurring right before or
> during vring_del_virtqueue(vq) race and cause virtio_vdpa_virtqueue_cb()
> to dereference the freed vq structure when accessing vq->vdev?


IRQs are not yet allocated then are they?
Looks like a false positive to me.

> > +	irqreturn_t ret;
> > +	int idx;
> > +
> > +	idx = srcu_read_lock(&vd_dev->cb_srcu);
> > +	ret = vring_interrupt(0, vq);
> > +	srcu_read_unlock(&vd_dev->cb_srcu, idx);
> > +
> > +	return ret;
> > +}
> > +
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260908053817.26065-1-kmehltretter@gmail.com?part=6