On 3/19/2026 10:19 PM, Martin K. Petersen wrote:
Hi Joshua!
This patch avoids a kernel warning that may occur if a virtio_scsi
controller is detached immediately following a disk detach. See the
commit message for details. The following are instructions to
produce the warning (without the proposed patch).
A few issues were flagged. Please review:
https://sashiko.dev/#/patchset/20260316153341.2062278-1-jdaley%40linux.ibm.com
Thanks!
Hello,
I've reviewed Sashiko's comments and concluded that this series requires further changes. I will
prepare a new version. Below are my responses to Sashiko's comments.
Thanks!
---
From Sashiko:
commit a1db3f127c8402cd51cacfe63d01f15805153a29
Author: Joshua Daley <[email protected]>
scsi: virtio_scsi: kick event_list unconditionally
The event_list processes non-hotplug events (such as LUN capacity
changes), so remove the conditions that guard the initial kicks in
_probe() and _restore(), as well as the work cancellation in _remove().
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 0ed8558dad72..982f49bc6c69 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -983,9 +983,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
goto scsi_add_host_failed;
virtio_device_ready(vdev);
-
- if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
- virtscsi_kick_event_all(vscsi);
+ virtscsi_kick_event_all(vscsi);
Does this code violate the virtio-scsi specification?
The specification mandates that a driver must not place buffers into the
event virtqueue if neither VIRTIO_SCSI_F_HOTPLUG nor VIRTIO_SCSI_F_CHANGE
has been negotiated.
By completely removing the VIRTIO_SCSI_F_HOTPLUG check without expanding it
to check for VIRTIO_SCSI_F_CHANGE, could this unconditionally populate the
event queue and cause strict implementations to reject the buffers or
transition the device into a broken state?
Stefan Hajnoczi <[email protected]> addressed this already. "No, this is a
hallucination."
scsi_scan_host(shost);
return 0;
[ ... ]
@@ -1002,8 +1000,7 @@ static void virtscsi_remove(struct virtio_device *vdev)
struct Scsi_Host *shost = virtio_scsi_host(vdev);
struct virtio_scsi *vscsi = shost_priv(shost);
- if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
- virtscsi_cancel_event_work(vscsi);
+ virtscsi_cancel_event_work(vscsi);
Can this introduce a use-after-free race during device removal that breaks
bisection?
If a concurrent worker thread executing virtscsi_handle_event() reaches
virtscsi_kick_event(), it invokes INIT_WORK(&event_node->work, ...).
INIT_WORK() clears work->data bits to 0.
If cancel_work_sync() runs concurrently in virtscsi_cancel_event_work(), it
might read work->data as 0, falsely conclude the work item is idle, and
return immediately without waiting for the work to complete.
virtscsi_remove() would then proceed to free the vscsi structure via
scsi_host_put(), while the worker thread continues and dereferences the now
freed vscsi->event_vq.vq_lock.
While this specific race appears to be fixed later in the patch series by
commit b602884db439 ("scsi: virtio_scsi: move INIT_WORK calls to
virtscsi_init"), removing the feature guard here exposes all virtio-scsi
instances to this crash, which might break bisection.
see below
scsi_remove_host(shost);
virtscsi_remove_vqs(vdev);
[ ... ]
@@ -1028,9 +1025,7 @@ static int virtscsi_restore(struct virtio_device *vdev)
return err;
virtio_device_ready(vdev);
-
- if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
- virtscsi_kick_event_all(vscsi);
+ virtscsi_kick_event_all(vscsi);
Could this cause workqueue linked list corruption during suspend and resume?
When PM suspend begins, freezable workqueues are frozen. Any unhandled event
from virtscsi_complete_event() leaves its work item queued. virtscsi_freeze()
tears down virtqueues but does not cancel pending work or set stop_events =
true.
Upon resume, virtscsi_restore() unconditionally calls virtscsi_kick_event_all()
here, which executes INIT_WORK() on all event nodes. Executing INIT_WORK() on
an actively queued work_struct blindly resets its list pointers, which silently
corrupts the frozen workqueue's doubly-linked list. This can lead to a kernel
panic when the workqueue is subsequently thawed.
Prior to this patch, this issue only affected devices that negotiated
VIRTIO_SCSI_F_HOTPLUG, but removing the guard exposes every virtio-scsi
device to this suspend/resume crash.
Both this and the previous review comment are based on the current issue with the INIT_WORK call
being located in virtscsi_kick_event().
As the Sashiko pointed out, this is addressed later in the series.
Performing this patch first does extend the INIT_WORK issue to devices that do not negotiate
hotplug, having a somewhat negative effect on bisection. So in that regard it makes sense to reorder
the patches such that the INIT_WORK call is moved first.
return err;
}
commit b602884db439878b4fde9b7b100a2b2263958fdf
Author: Joshua Daley <[email protected]>
scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init
The last step of virtscsi_handle_event is to call virtscsi_kick_event,
which calls INIT_WORK on it's own work item. INIT_WORK resets the
work item's data bits to 0.
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 982f49bc6c69..15ca2a2d7aa4 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -242,7 +242,6 @@ static int virtscsi_kick_event(struct virtio_scsi *vscsi,
struct scatterlist sg;
unsigned long flags;
- INIT_WORK(&event_node->work, virtscsi_handle_event);
sg_init_one(&sg, event_node->event, sizeof(struct virtio_scsi_event));
spin_lock_irqsave(&vscsi->event_vq.vq_lock, flags);
[ ... ]
@@ -898,6 +897,9 @@ static int virtscsi_init(struct virtio_device *vdev,
virtscsi_config_set(vdev, cdb_size, VIRTIO_SCSI_CDB_SIZE);
virtscsi_config_set(vdev, sense_size, VIRTIO_SCSI_SENSE_SIZE);
+ for (i = 0; i < VIRTIO_SCSI_EVENT_LEN; i++)
+ INIT_WORK(&vscsi->event_list[i].work, virtscsi_handle_event);
Can this code corrupt the workqueue's linked list during system resume?
Event works are queued on system_freezable_wq in virtscsi_complete_event():
virtscsi_complete_event() {
...
if (!vscsi->stop_events)
queue_work(system_freezable_wq, &event_node->work);
}
During system suspend, virtscsi_freeze() resets the virtio device but does
not cancel pending event works, as it lacks a call to
virtscsi_cancel_event_work(). Because the workqueue is freezable, pending
works remain actively linked in the workqueue's internal pending list.
Upon system resume, virtscsi_restore() runs and calls virtscsi_init(),
which unconditionally executes this INIT_WORK() during resume:
virtscsi_restore() -> virtscsi_init() {
...
for (i = 0; i < VIRTIO_SCSI_EVENT_LEN; i++)
INIT_WORK(&vscsi->event_list[i].work, virtscsi_handle_event);
}
Calling INIT_WORK() on a work item that is actively linked in a workqueue
executes INIT_LIST_HEAD(&work->entry), overwriting its list pointers while
adjacent workqueue list elements still point to it. This can result in a
corrupted workqueue linked list when the workqueue is thawed.
This appears to be a valid concern.
INIT_WORK need only be called on the event nodes ONCE in the lifetime of the device, it should not
be recalled on resume, especially since work may remain frozen in the workqueue and have its data
corrupted by this call.
Lets move the calls to virtscsi_probe() instead of virtscsi_init().
