Skip to content

virtio: retire a queue after a fatal fetch error - #4036

Open
jstarks wants to merge 1 commit into
microsoft:mainfrom
jstarks:fix_virtio_blk_hang
Open

virtio: retire a queue after a fatal fetch error#4036
jstarks wants to merge 1 commit into
microsoft:mainfrom
jstarks:fix_virtio_blk_hang

Conversation

@jstarks

@jstarks jstarks commented Jul 26, 2026

Copy link
Copy Markdown
Member

A guest can hand a device a descriptor chain the queue refuses to parse: one with a cycle, an indirect table entry that is itself indirect, or an indirect table outside guest memory. The queue rejects such a chain without consuming it, so the available index does not move and the next fetch fails identically.

virtio-blk and the virtio-vsock TX path logged the error and kept polling. Since the retry fails immediately, the worker's future never returns Pending, so it spins at full CPU and can no longer be stopped -- until_stopped only polls the cancel future once the work future returns Pending -- hanging teardown, reset, save/restore, and inspect. The trace is rate-limited, so this is nearly silent, and any guest can trigger it.

Fix this in the queue so a device cannot reintroduce it. A fetch error now latches, and the queue then reports itself permanently empty instead of repeating an error that can never clear. Completions still publish to the used ring, so in-flight work drains before shutdown. The two affected devices also stop their loops explicitly. This makes an oversubscribed ring fatal where it was previously recoverable; no device relied on that recovery, and it was itself a spin hazard.

Discovered via https://github.com/weltling/virtio-villain.

A guest can hand a device a descriptor chain the queue refuses to parse:
one with a cycle, an indirect table entry that is itself indirect, or an
indirect table outside guest memory. The queue rejects such a chain
without consuming it, so the available index does not move and the next
fetch fails identically.

virtio-blk and the virtio-vsock TX path logged the error and kept polling.
Since the retry fails immediately, the worker's future never returns
Pending, so it spins at full CPU and can no longer be stopped --
until_stopped only polls the cancel future once the work future returns
Pending -- hanging teardown, reset, save/restore, and inspect. The trace
is rate-limited, so this is nearly silent, and any guest can trigger it.

Fix this in the queue so a device cannot reintroduce it. A fetch error now
latches, and the queue then reports itself permanently empty instead of
repeating an error that can never clear. Completions still publish to the
used ring, so in-flight work drains before shutdown. The two affected
devices also stop their loops explicitly. This makes an oversubscribed
ring fatal where it was previously recoverable; no device relied on that
recovery, and it was itself a spin hazard.

Discovered via https://github.com/weltling/virtio-villain.
Copilot AI review requested due to automatic review settings July 26, 2026 17:19
@jstarks
jstarks requested a review from a team as a code owner July 26, 2026 17:19
@github-actions github-actions Bot added the unsafe Related to unsafe code label Jul 26, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the virtio queue fetch path against guest-triggered fatal descriptor-chain parsing errors that previously could cause device workers to spin at 100% CPU and become effectively non-cancellable (wedging teardown/reset/save/restore/inspect). It does this by latching the first fatal fetch error in the queue core and thereafter reporting the queue as permanently empty (and parking the Stream), while still allowing already-consumed descriptors to complete so in-flight work can drain cleanly.

Changes:

  • Add a “failed/retired” latch to the virtio queue fetch side: first fatal fetch error is returned once, then the queue reports no further work forever.
  • Adjust VirtioQueue’s poll_kick and Stream behavior so callers cannot busy-loop after a fatal queue error (stream parks, kick polling stays pending).
  • Update virtio-blk and virtio-vsock workers to stop their loops on queue read errors, and add regression tests to cover “no spin / remains stoppable” behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
vm/devices/virtio/virtio/src/tests.rs Adds unit tests asserting fatal fetch errors are reported once and then the queue behaves as empty/parked (including stream parking).
vm/devices/virtio/virtio/src/queue.rs Introduces failed latch in the queue core fetch path to retire fetching after the first fatal QueueError.
vm/devices/virtio/virtio/src/common.rs Makes poll_kick return Pending forever after failure and documents the “stream parks” behavior after a fatal error.
vm/devices/virtio/virtio_vsock/src/lib.rs Stops the vsock worker when the TX queue yields a fatal read error, avoiding a retry loop.
vm/devices/virtio/virtio_blk/src/lib.rs Stops accepting new work after a fatal queue error but continues draining in-flight I/O before exiting.
vm/devices/virtio/virtio_blk/src/integration_tests.rs Adds an integration regression test ensuring a cyclic descriptor chain cannot wedge the virtio-blk worker (uses a separate executor thread + timeout).

@github-actions

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants