On Wed, May 21, 2025 at 10:57 PM Parav Pandit [off-list ref] wrote:

quoted

From: Stefan Hajnoczi <stefanha@redhat.com>
Sent: Wednesday, May 21, 2025 8:27 PM

On Wed, May 21, 2025 at 06:37:41AM +0000, Parav Pandit wrote:

quoted

When the PCI device is surprise removed, requests may not complete the
device as the VQ is marked as broken. Due to this, the disk deletion
hangs.

Fix it by aborting the requests when the VQ is broken.

With this fix now fio completes swiftly.
An alternative of IO timeout has been considered, however when the
driver knows about unresponsive block device, swiftly clearing them
enables users and upper layers to react quickly.

Verified with multiple device unplug iterations with pending requests
in virtio used ring and some pending with the device.

Fixes: 43bb40c5b926 ("virtio_pci: Support surprise removal of virtio
pci device")
Cc: stable@vger.kernel.org
Reported-by: lirongqing@baidu.com
Closes:
https://lore.kernel.org/virtualization/c45dd68698cd47238c55fb73ca9b474
1@baidu.com/
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Reviewed-by: Israel Rukshin <redacted>
Signed-off-by: Parav Pandit <redacted>
---
changelog:
v0->v1:
- Fixed comments from Stefan to rename a cleanup function
- Improved logic for handling any outstanding requests
  in bio layer
- improved cancel callback to sync with ongoing done()

---
 drivers/block/virtio_blk.c | 95
++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7cffea01d868..5212afdbd3c7 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -435,6 +435,13 @@ static blk_status_t virtio_queue_rq(struct

blk_mq_hw_ctx *hctx,

quoted

    blk_status_t status;
    int err;

+   /* Immediately fail all incoming requests if the vq is broken.
+    * Once the queue is unquiesced, upper block layer flushes any

pending

quoted

+    * queued requests; fail them right away.
+    */
+   if (unlikely(virtqueue_is_broken(vblk->vqs[qid].vq)))
+           return BLK_STS_IOERR;
+
    status = virtblk_prep_rq(hctx, vblk, req, vbr);
    if (unlikely(status))
            return status;

@@ -508,6 +515,11 @@ static void virtio_queue_rqs(struct rq_list *rqlist)
    while ((req = rq_list_pop(rqlist))) {
            struct virtio_blk_vq *this_vq = get_virtio_blk_vq(req-

mq_hctx);

+           if (unlikely(virtqueue_is_broken(this_vq->vq))) {
+                   rq_list_add_tail(&requeue_list, req);
+                   continue;
+           }
+
            if (vq && vq != this_vq)
                    virtblk_add_req_batch(vq, &submit_list);
            vq = this_vq;

@@ -1554,6 +1566,87 @@ static int virtblk_probe(struct virtio_device

*vdev)

quoted

    return err;
 }

+static bool virtblk_request_cancel(struct request *rq, void *data) {
+   struct virtblk_req *vbr = blk_mq_rq_to_pdu(rq);
+   struct virtio_blk *vblk = data;
+   struct virtio_blk_vq *vq;
+   unsigned long flags;
+
+   vq = &vblk->vqs[rq->mq_hctx->queue_num];
+
+   spin_lock_irqsave(&vq->lock, flags);
+
+   vbr->in_hdr.status = VIRTIO_BLK_S_IOERR;
+   if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq))
+           blk_mq_complete_request(rq);
+
+   spin_unlock_irqrestore(&vq->lock, flags);
+   return true;
+}
+
+static void virtblk_broken_device_cleanup(struct virtio_blk *vblk) {
+   struct request_queue *q = vblk->disk->queue;
+
+   if (!virtqueue_is_broken(vblk->vqs[0].vq))
+           return;

Can a subset of virtqueues be broken? If so, then this code doesn't handle it.

On device removal all the VQs are broken. This check only uses a VQ to decide on.
In future may be more elaborate API to have virtio_dev_broken() can be added.
Prefer to keep this patch without extending many APIs given it has Fixes tag.

virtblk_remove() is called not just when a PCI device is hot
unplugged. For example, removing the virtio_blk kernel module or
unbinding a specific virtio device instance also calls it.

My concern is that virtblk_broken_device_cleanup() is only intended
for the cases where all virtqueues are broken or none are broken. If
just the first virtqueue is broken then it completes requests on
operational virtqueues and they may still raise an interrupt.

The use-after-free I'm thinking about is when virtblk_request_cancel()
-> ... -> blk_mq_end_request() has been called on a virtqueue that is
not broken, followed by virtblk_done() using the struct request
obtained from blk_mq_rq_from_pdu().

Maybe just adding a virtqueue_is_broken() check in
virtblk_request_cancel() is enough to skip requests that are still
in-flight on operational virtqueues.

quoted

quoted

+
+   /* Start freezing the queue, so that new requests keeps waitng at
+the

s/waitng/waiting/

Ack.

quoted

quoted

+    * door of bio_queue_enter(). We cannot fully freeze the queue

because

quoted

+    * freezed queue is an empty queue and there are pending requests,

so

quoted

+    * only start freezing it.
+    */
+   blk_freeze_queue_start(q);
+
+   /* When quiescing completes, all ongoing dispatches have completed
+    * and no new dispatch will happen towards the driver.
+    * This ensures that later when cancel is attempted, then are not
+    * getting processed by the queue_rq() or queue_rqs() handlers.
+    */
+   blk_mq_quiesce_queue(q);
+
+   /*
+    * Synchronize with any ongoing VQ callbacks, effectively quiescing
+    * the device and preventing it from completing further requests
+    * to the block layer. Any outstanding, incomplete requests will be
+    * completed by virtblk_request_cancel().
+    */
+   virtio_synchronize_cbs(vblk->vdev);
+
+   /* At this point, no new requests can enter the queue_rq() and
+    * completion routine will not complete any new requests either for

the

quoted

+    * broken vq. Hence, it is safe to cancel all requests which are
+    * started.
+    */
+   blk_mq_tagset_busy_iter(&vblk->tag_set, virtblk_request_cancel,
+vblk);

Although virtio_synchronize_cbs() was called, a broken/malicious device can
still raise IRQs. Would that lead to use-after-free or similar undefined
behavior for requests that have been submitted to the device?

It shouldn't because vring_interrupt() also checks for the broken VQ before invoking the _done().
Once the VQ is broken and even if _done() is invoked, it wont progress further on get_buf().
And VQs are freed later in del_vq() after the device is reset as you suggested.

See above about a scenario where a race can happen.

quoted

It seems safer to reset the device before marking the requests as failed.

Such addition should be avoided because when the device is surprise removed, even reset will not complete.

The virtblk_remove() function modified by this patch calls
virtio_reset_device(). Is the expected behavior after this patch that
virtblk_remove() spins forever?

quoted

quoted

+   blk_mq_tagset_wait_completed_request(&vblk->tag_set);
+
+   /* All pending requests are cleaned up. Time to resume so that disk
+    * deletion can be smooth. Start the HW queues so that when queue

is

quoted

+    * unquiesced requests can again enter the driver.
+    */
+   blk_mq_start_stopped_hw_queues(q, true);
+
+   /* Unquiescing will trigger dispatching any pending requests to the
+    * driver which has crossed bio_queue_enter() to the driver.
+    */
+   blk_mq_unquiesce_queue(q);
+
+   /* Wait for all pending dispatches to terminate which may have been
+    * initiated after unquiescing.
+    */
+   blk_mq_freeze_queue_wait(q);
+
+   /* Mark the disk dead so that once queue unfreeze, the requests
+    * waiting at the door of bio_queue_enter() can be aborted right

away.

quoted

+    */
+   blk_mark_disk_dead(vblk->disk);
+
+   /* Unfreeze the queue so that any waiting requests will be aborted.

*/

quoted

+   blk_mq_unfreeze_queue_nomemrestore(q);
+}
+
 static void virtblk_remove(struct virtio_device *vdev)  {
    struct virtio_blk *vblk = vdev->priv; @@ -1561,6 +1654,8 @@ static
void virtblk_remove(struct virtio_device *vdev)
    /* Make sure no work handler is accessing the device. */
    flush_work(&vblk->config_work);

+   virtblk_broken_device_cleanup(vblk);
+
    del_gendisk(vblk->disk);
    blk_mq_free_tag_set(&vblk->tag_set);

--
2.34.1