Re: [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop

[Fam Zheng]

On Fri, 11/13 14:17, Paolo Bonzini wrote:
> 
> 
> On 13/11/2015 10:32, Fam Zheng wrote:
> > When a request R is absorbed by request M, it is appended to the
> > "mr_next" queue led by M, and is completed together with the completion
> > of M, in virtio_blk_rw_complete.
> > 
> > With error policy equals stop, if M has an I/O error, now R also gets
> > prepended to the per device DMA restart queue, which will be retried
> > when VM resumes.  It leads to a double completion (in symptoms of memory
> > corruption or use after free).
> > 
> > Adding R to the queue is superfluous, only M needs to be in the queue.
> > 
> > Fix this by marking request R as "merged" and skipping it in
> > virtio_blk_handle_rw_error.
> > 
> > Cc: address@hidden
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  hw/block/virtio-blk.c          | 9 +++++++--
> >  include/hw/virtio/virtio-blk.h | 1 +
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index e70fccf..4871f2a 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -36,6 +36,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
> >      req->in_len = 0;
> >      req->next = NULL;
> >      req->mr_next = NULL;
> > +    req->merged = false;
> >      return req;
> >  }
> >  
> > @@ -72,8 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq 
> > *req, int error,
> >      VirtIOBlock *s = req->dev;
> >  
> >      if (action == BLOCK_ERROR_ACTION_STOP) {
> > -        req->next = s->rq;
> > -        s->rq = req;
> > +        /* Merged requests don't need to be restarted. */
> > +        if (!req->merged) {
> > +            req->next = s->rq;
> > +            s->rq = req;
> > +        }
> 
> I think it still needs to be queued, so that the request is migrated
> correctly.  Should req->merged be checked in virtio_blk_dma_restart_bh
> instead?

Yes you're right.

Thanks,
Fam