> From: Stefan Hajnoczi <stefa...@gmail.com>
> Sent: Thursday, May 22, 2025 11:43 PM
>
> 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/c45dd68698cd47238c55fb7
> > > > > > 3ca9
> > > > > > 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.
>
Each upper layer is different in how to quiesce it. so not sure how that code
can be generalized in lower layers.
But I agree that a at minimum common PCI layer signaling layer indicating
device surprise removal is useful.
For the full solution it appeared lot more changes needed likely on top of
this. So present objective is restore the regression caused by the commit in
fixes tag.
