[RFC PATCH] scsi: virtio_scsi: bound EH timer resets to avoid unkillable hang

[RFC PATCH] scsi: virtio_scsi: bound EH timer resets to avoid unkillable hang

[Nguyen Ngoc Thang]

Reported-by: syzbot+53706c567afab5131044@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=53706c567afab5131044
("INFO: task hung in __rq_qos_throttle (2)", still open upstream, no
fix, bisection failed, 97 crashes since 2026-08-08)

virtscsi_eh_timed_out() always returns SCSI_EH_RESET_TIMER, trusting
the host to eventually answer every command. A privileged raw write
to /sys/bus/pci/devices/*/config that clears PCI_COMMAND_MASTER on
the disk breaks that assumption: the device can no longer DMA, so no
completion -- real or error -- ever arrives, and the reset loop runs
forever. Any submitter that then hits wbt's inflight limit blocks
uninterruptibly in wbt_wait() with no way to recover.

The syzbot C reproducer does exactly this: it opens the disk's PCI
config sysfs file and pwrite()s the two bytes 02 00 at offset 4
(PCI_COMMAND), clearing PCI_COMMAND_MASTER while keeping
PCI_COMMAND_MEMORY set. The hung task is a writeback worker stuck in
wbt_wait() <- __rq_qos_throttle() <- blk_mq_get_new_requests(), on a
disk named sdaN -- i.e. a SCSI-model disk, consistent with
virtio_scsi rather than virtio-blk.

Bound the resets: after VIRTSCSI_EH_RESET_LIMIT tries (~150s), let
real SCSI EH run (abort -> device reset -> offline), which fails the
command and unblocks rq_qos waiters. A host that is merely slow for
longer than that now gets its commands aborted/offlined instead of
waited on indefinitely -- a deliberate tradeoff.

virtscsi_tmf(), used by both abort and device-reset handlers, waited
on the same broken ctrl virtqueue unboundedly, which would just move
the hang into the EH thread. Bound it too. On timeout the command is
left queued on the ctrl vq (a device that answers late must still be
able to find it), so it is not freed. Clearing cmd->comp so a late
completion doesn't write through the now-invalid on-stack completion
has to be serialized against virtscsi_complete_free(), which reads
cmd->comp and calls complete() on it under ctrl_vq.vq_lock -- taking
that same lock around the check-and-clear (and re-checking
completion_done() inside it) closes the race instead of just
shrinking it. The leak is one virtio_scsi_cmd per timed-out TMF,
bounded by queue depth, on a path where the device is being offlined
anyway.

Verified with a QEMU virtio-scsi repro that reproduces the syzbot
mechanism above (pwrite of 02 00 at PCI config offset 4 on the sda
backing device, mid-writeback, via /sys/bus/pci/devices/*/config):
unpatched, a request sits stuck past its own 30s timeout with zero EH
activity even after 100+s; patched, eh_resets increments
deterministically, SCSI EH aborts/resets/offlines the device at the
expected ~150-190s mark, and both previously-stuck writers come back
with -EIO instead of hanging. Full dmesg from the patched run was
checked for WARN/BUG/lockdep output around the new ctrl_vq.vq_lock
critical section in virtscsi_tmf(); none appeared.

Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
 drivers/scsi/virtio_scsi.c | 51 ++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 35731b18c519..b4f20c487718 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -37,6 +37,11 @@
 #define VIRTIO_SCSI_EVENT_LEN 8
 #define VIRTIO_SCSI_VQ_BASE 2
 
+/* Max timer resets in virtscsi_eh_timed_out() before letting real EH run. */
+#define VIRTSCSI_EH_RESET_LIMIT 5
+/* How long to let the host answer an abort/reset TMF before giving up. */
+#define VIRTSCSI_TMF_TIMEOUT (10 * HZ)
+
 static unsigned int virtscsi_poll_queues;
 module_param(virtscsi_poll_queues, uint, 0644);
 MODULE_PARM_DESC(virtscsi_poll_queues,
@@ -46,6 +51,7 @@ MODULE_PARM_DESC(virtscsi_poll_queues,
 struct virtio_scsi_cmd {
 	struct scsi_cmnd *sc;
 	struct completion *comp;
+	unsigned int eh_resets;
 	union {
 		struct virtio_scsi_cmd_req       cmd;
 		struct virtio_scsi_cmd_req_pi    cmd_pi;
@@ -586,6 +592,7 @@ static enum scsi_qc_status virtscsi_queuecommand(struct Scsi_Host *shost,
 		"cmd %p CDB: %#02x\n", sc, sc->cmnd[0]);
 
 	cmd->sc = sc;
+	cmd->eh_resets = 0;
 
 	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
 
@@ -625,7 +632,35 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
 			      sizeof cmd->req.tmf, sizeof cmd->resp.tmf, true) < 0)
 		goto out;
 
-	wait_for_completion(&comp);
+	if (!wait_for_completion_timeout(&comp, VIRTSCSI_TMF_TIMEOUT)) {
+		unsigned long flags;
+		bool completed;
+
+		/*
+		 * No answer within the timeout. virtscsi_complete_free()
+		 * reads cmd->comp and calls complete() on it under
+		 * ctrl_vq.vq_lock, so take the same lock to decide, atomically
+		 * with that path, whether the completion already happened.
+		 *
+		 * If it hasn't: clear cmd->comp so a completion that arrives
+		 * after we drop the lock finds NULL and leaves this
+		 * soon-to-be-invalid stack frame alone. cmd stays queued on
+		 * the ctrl vq (a device that answers late must still be able
+		 * to find it), so it is not freed here.
+		 *
+		 * If it has: the response landed (and complete() already ran)
+		 * right as we timed out, so fall through and read it as if
+		 * wait_for_completion_timeout() had succeeded.
+		 */
+		spin_lock_irqsave(&vscsi->ctrl_vq.vq_lock, flags);
+		completed = completion_done(&comp);
+		if (!completed)
+			cmd->comp = NULL;
+		spin_unlock_irqrestore(&vscsi->ctrl_vq.vq_lock, flags);
+
+		if (!completed)
+			return FAILED;
+	}
 	if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
 	    cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
 		ret = SUCCESS;
@@ -783,13 +818,19 @@ static void virtscsi_commit_rqs(struct Scsi_Host *shost, u16 hwq)
 }
 
 /*
- * The host guarantees to respond to each command, although I/O
- * latencies might be higher than on bare metal.  Reset the timer
- * unconditionally to give the host a chance to perform EH.
+ * The host normally answers every command, so reset the timer and keep
+ * waiting. But if the transport is broken (e.g. bus mastering was turned
+ * off), no completion can ever arrive: give up after a few resets so SCSI
+ * EH fails the command instead of blocking its submitter forever.
  */
 static enum scsi_timeout_action virtscsi_eh_timed_out(struct scsi_cmnd *scmnd)
 {
-	return SCSI_EH_RESET_TIMER;
+	struct virtio_scsi_cmd *cmd = scsi_cmd_priv(scmnd);
+
+	if (++cmd->eh_resets < VIRTSCSI_EH_RESET_LIMIT)
+		return SCSI_EH_RESET_TIMER;
+
+	return SCSI_EH_NOT_HANDLED;
 }
 
 static const struct scsi_host_template virtscsi_host_template = {
-- 
2.43.0

Re: [RFC PATCH] scsi: virtio_scsi: bound EH timer resets to avoid unkillable hang

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing eh_host_reset_handler causes SCSI EH to free commands while the host is active, resulting in DMA memory corruption and Use-After-Free.
--

commit c1b61825f071ba8c8020eead463039fdb142c7b0
Author: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>

scsi: virtio_scsi: bound EH timer resets to avoid unkillable hang

This commit limits the number of times virtscsi_eh_timed_out() can reset
the command timer to 5, returning SCSI_EH_NOT_HANDLED afterward. It also
bounds the wait time in virtscsi_tmf() when handling aborts and resets to
prevent infinite hangs if the transport breaks.

Closes: https://syzkaller.appspot.com/bug?extid=53706c567afab5131044

> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 35731b18c519e..b4f20c4877182 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c

[ ... ]

> @@ -783,13 +818,19 @@ static void virtscsi_commit_rqs(struct Scsi_Host *shost, u16 hwq)
>  }
>  
>  /*
> - * The host guarantees to respond to each command, although I/O
> - * latencies might be higher than on bare metal.  Reset the timer
> - * unconditionally to give the host a chance to perform EH.
> + * The host normally answers every command, so reset the timer and keep
> + * waiting. But if the transport is broken (e.g. bus mastering was turned
> + * off), no completion can ever arrive: give up after a few resets so SCSI
> + * EH fails the command instead of blocking its submitter forever.
>   */
>  static enum scsi_timeout_action virtscsi_eh_timed_out(struct scsi_cmnd *scmnd)
>  {
> -	return SCSI_EH_RESET_TIMER;
> +	struct virtio_scsi_cmd *cmd = scsi_cmd_priv(scmnd);
> +
> +	if (++cmd->eh_resets < VIRTSCSI_EH_RESET_LIMIT)
> +		return SCSI_EH_RESET_TIMER;
> +
> +	return SCSI_EH_NOT_HANDLED;

[Severity: Critical]
Does returning SCSI_EH_NOT_HANDLED here allow SCSI EH to run and eventually
free the command without ensuring the host has dropped its DMA references?

If the virtio host experiences a severe delay and eventually completes the
timed-out commands, could it DMA the response directly into memory that has
already been freed by SCSI EH and potentially reallocated to new requests?

If this happens, wouldn't it result in a use-after-free and DMA memory
corruption when virtscsi_complete_cmd() runs?

For example:

virtscsi_complete_cmd() {
	struct virtio_scsi_cmd *cmd = buf;
	struct scsi_cmnd *sc = cmd->sc;
	struct virtio_scsi_cmd_resp *resp = &cmd->resp.cmd;
	...
	scsi_done(sc);
}

Here the dangling pointer from the virtqueue would be dereferenced after
the command has been freed by SCSI EH, potentially followed by a kernel
panic when scsi_done() is called on the corrupted scsi_cmnd pointer.

>  }
>  
>  static const struct scsi_host_template virtscsi_host_template = {

[Severity: Critical]
Does the host template lack an eh_host_reset_handler?

Without this, SCSI EH cannot safely stop the virtio device before freeing
the timed-out commands. If SCSI EH attempts aborts and device resets which
also time out, it will give up, offline the device, and flush (free) the
commands. When the host later resumes and processes the virtqueue, could it
overwrite the freed memory?

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

Re: [RFC PATCH] scsi: virtio_scsi: bound EH timer resets to avoid unkillable hang

[Nguyen Ngoc Thang]

Thank you for the review -- the eh_host_reset_handler finding is correct,
and the underlying mechanism is worse than a stale read.

Once virtscsi_eh_timed_out() lets SCSI EH run to completion on an
unresponsive host, scsi_eh_bus_device_reset() leaves the command in
work_q (device reset fails the same way abort does, via the same bounded
virtscsi_tmf()), and since virtio_scsi implements neither
eh_target_reset_handler, eh_bus_reset_handler nor eh_host_reset_handler,
scsi_eh_target_reset()/scsi_eh_bus_reset()/scsi_eh_host_reset() all fail
immediately (scsi_try_*_reset() return FAILED when the handler pointer is
NULL) and the command falls through to scsi_eh_offline_sdevs(), which
calls scsi_eh_finish_cmd() and frees the tag back to the block layer.

The struct virtio_scsi_cmd for that command lives in scsi_cmd_priv(),
i.e. inside the now-freed request. If a new command is queued on the
same tag, it gets the same address. If the host was only very slow --
not actually dead -- and eventually completes the *original* descriptor,
virtscsi_complete_cmd() will read cmd->sc from that shared address and
call scsi_done() on whatever command currently occupies it, which by
then is the live, unrelated command still in flight on that tag. That's
a double completion on a request the driver itself has not finished,
not merely a read of stale memory.

The original virtscsi_eh_timed_out() comment ("The host guarantees to
respond to each command... Reset the timer unconditionally") reads, in
this light, less like an optimistic assumption and more like the
invariant that kept this reachable at all: as long as EH could never
progress past abort/device-reset into offline+free, the tag-reuse race
had no opening. My patch removes that invariant to fix the wbt hang, but
doesn't supply a replacement, which is the hole you're pointing at.

What I looked at as a replacement and why I'm not sending it as a v2 yet:

Adding eh_host_reset_handler that mirrors virtscsi_freeze()/
virtscsi_restore() (virtscsi_remove_vqs() + virtscsi_init()) was my first
instinct. virtio_reset_device() does call virtio_break_device() +
virtio_synchronize_cbs() before dev->config->reset(), which is meant to
guarantee no vq callback is still in flight when del_vqs() runs -- but
that pairing is compiled in only under CONFIG_VIRTIO_HARDEN_NOTIFICATION
(drivers/virtio/virtio.c:255-264), so the safety property isn't universal.
Separately, a host reset triggered by one wedged command on one LUN would
tear down and reinitialize every virtqueue on the host, silently
abandoning any genuinely in-flight, unrelated I/O on other LUNs/targets
that scsi_error_handler() never queued for recovery in the first place --
freeze/restore can rely on PM having fully quiesced the block queues
first; EH's SHOST_RECOVERY only blocks new submissions, it doesn't drain
what's already dispatched. I don't want to trade the tag-reuse race for
a config-dependent fix plus silent collateral I/O loss without your
input on whether that tradeoff is acceptable or whether there's a
narrower primitive intended for this.

Is virtio_break_device() (independent of host reset, and independent of
CONFIG_VIRTIO_HARDEN_NOTIFICATION) the intended way to make a single
wedged command's descriptor permanently safe to let go of, without
resetting the whole device? It looks close -- virtqueue_get_buf() checks
vq->broken before touching the used ring at all -- but it's one-way (no
unbreak short of recreating the vq), so I'm not certain it's meant to be
used outside device teardown. Has this class of problem (bound a SCSI EH
timeout on a transport that can't cheaply prove a command is gone) come
up for virtio_scsi or virtio_blk before, and is there a pattern I should
be following instead of inventing one?

Happy to do the legwork on whichever direction you point at -- I have a
QEMU virtio-scsi repro harness already wired up for the original hang
(clearing PCI_COMMAND_MASTER mid-write) that I can extend to exercise
the recovery path too.