On Thu, May 22, 2025 at 10:56 AM Parav Pandit <pa...@nvidia.com> wrote:
>
>
> > From: Stefan Hajnoczi <stefa...@gmail.com>
> > Sent: Thursday, May 22, 2025 8:06 PM
> >
> > On Wed, May 21, 2025 at 10:57 PM Parav Pandit <pa...@nvidia.com> wrote:
> > > > From: Stefan Hajnoczi <stefa...@redhat.com>
> > > > Sent: Wednesday, May 21, 2025 8:27 PM
> > > >
> > > > On Wed, May 21, 2025 at 06:37:41AM +0000, Parav Pandit wrote:
> > > > > 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: sta...@vger.kernel.org
> > > > > Reported-by: lirongq...@baidu.com
> > > > > Closes:
> > > > > https://lore.kernel.org/virtualization/c45dd68698cd47238c55fb73ca9
> > > > > b474
> > > > > 1...@baidu.com/
> > > > > Reviewed-by: Max Gurtovoy <mgurto...@nvidia.com>
> > > > > Reviewed-by: Israel Rukshin <isra...@nvidia.com>
> > > > > Signed-off-by: Parav Pandit <pa...@nvidia.com>
> > > > > ---
> > > > > 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,
> > > > > 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
> > > > > + * 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)
> > > > > 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.
> >
> This is ok.
>
> > 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.
> >
> I see that vq broken is extended for each reset scenario too lately in
> vp_modern_enable_vq_after_reset().
> So yes, this patch which was intended for original surprise removal bug where
> vq broken was not done for reset cases.
>
> I believe for fixing the cited patch, device->broken flag should be used.
> Max indicated this in an internal review, but I was inclined to avoid adding
> many changes.
> And hence reuse vq broken.
>
> So one option is to extend,
>
> virtio_break_device() to have a flag like below and check during remove().
> dev->broken = true;
>
> or to revert the patch, 43bb40c5b926, which Michael was not linking.
>
> > 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().
> >
> This can happen for case when nonsurprise removal is done possibly.
>
> > 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.
> Well, the idea of calling request_cancel() iterator only if the VQ is broken.
> So in regular remove() this should not be called. Existing flow is better.
>
> >
> > >
> > > >
> > > > > +
> > > > > + /* Start freezing the queue, so that new requests keeps waitng
> > > > > +at the
> > > >
> > > > s/waitng/waiting/
> > > >
> > > Ack.
> > >
> > > > > + * door of bio_queue_enter(). We cannot fully freeze the queue
> > > > because
> > > > > + * freezed queue is an empty queue and there are pending
> > > > > + requests,
> > > > so
> > > > > + * 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
> > > > > + * 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.
> >
> > >
> > > > 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?
> If the PCI device is truly removed physically, then yes.
> This patch is not addressing such problem that existed even before the patch
> in fixes tag.
>
> I have experienced this already. Adding that support is relatively bigger
> change (than this fix).
Perhaps a full solution rather than a partial solution would end up
being simpler and cleaner. Instead of cutting out a special code path
for the virtio-blk PCI surprise unplug case, tackling how the core
virtio subsystem should handle PCI surprise unplug may give
virtio_blk.c more helpful virtio APIs that make it less complex. It's
up to you.
Stefan
>
> >
> > >
> > > > > + 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
> > > > > + * 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.
> > > > > + */
> > > > > + blk_mark_disk_dead(vblk->disk);
> > > > > +
> > > > > + /* Unfreeze the queue so that any waiting requests will be
> > > > > aborted.
> > > > */
> > > > > + 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
> > > > >
> > >
