Re: [PATCH v2 3/3] virtio: implement synchronize_cbs for remaining transports
From: sashiko-bot@kernel.org
Date: 2026-09-05 15:35:15
Also in:
kvm, linux-s390
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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? [ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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