Re: [Qemu-devel] [PATCH 2/3] virtio-blk: Don't exit on invalid VQ data

[Fam Zheng]

On Tue, 04/22 18:00, Jason Wang wrote:
> On 04/22/2014 04:55 PM, Fam Zheng wrote:
> > Set vdev's broken flag, instead of exit, if the VQ has invalid data.
> >
> > Check VirtIODevice.broken in VQ output handler, and don't pop any more
> > request if set, until the device is reset.
> >
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  hw/block/virtio-blk.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 8a568e5..f69aca6 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -339,20 +339,23 @@ static void virtio_blk_handle_read(VirtIOBlockReq 
> > *req)
> >                     virtio_blk_rw_complete, req);
> >  }
> >  
> > -static void virtio_blk_handle_request(VirtIOBlockReq *req,
> > -    MultiReqBuffer *mrb)
> > +static int virtio_blk_handle_request(VirtIOBlockReq *req,
> > +                                     MultiReqBuffer *mrb)
> >  {
> >      uint32_t type;
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(req->dev);
> >  
> >      if (req->elem.out_num < 1 || req->elem.in_num < 1) {
> >          error_report("virtio-blk missing headers");
> > -        exit(1);
> > +        virtio_set_broken(vdev);
> > +        return -EINVAL;
> >      }
> >  
> >      if (req->elem.out_sg[0].iov_len < sizeof(*req->out) ||
> >          req->elem.in_sg[req->elem.in_num - 1].iov_len < sizeof(*req->in)) {
> >          error_report("virtio-blk header not in correct element");
> > -        exit(1);
> > +        virtio_set_broken(vdev);
> > +        return -EINVAL;
> >      }
> >  
> >      req->out = (void *)req->elem.out_sg[0].iov_base;
> > @@ -389,6 +392,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq 
> > *req,
> >          virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> >          g_free(req);
> >      }
> > +    return 0;
> >  }
> >  
> >  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > @@ -399,6 +403,10 @@ static void virtio_blk_handle_output(VirtIODevice 
> > *vdev, VirtQueue *vq)
> >          .num_writes = 0,
> >      };
> >  
> > +    if (virtio_broken(vdev)) {
> > +        return;
> > +    }
> > +
> >  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> >      /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
> >       * dataplane here instead of waiting for .set_status().
> > @@ -410,7 +418,9 @@ static void virtio_blk_handle_output(VirtIODevice 
> > *vdev, VirtQueue *vq)
> >  #endif
> >  
> >      while ((req = virtio_blk_get_request(s))) {
> > -        virtio_blk_handle_request(req, &mrb);
> > +        if (virtio_blk_handle_request(req, &mrb) < 0) {
> > +            return;
> > +        }
> >      }
> >  
> >      virtio_submit_multiwrite(s->bs, &mrb);
> 
> For virtio-blk and virtio-scsi, can we just check this in
> virtio_queue_notify_vq()?

That way we are more explict and also stricter with the convention of setting
"broken": no queue will be passed to the device code, for any virtio-*.

But yes that should work and may be even easier, because in virtio.c there are
error paths in VQ itself, in which case guest/host communicating with VQ should
be avoid completely.

So it sounds like a good idea.

> 
> For virtio-net, just ignore the request form guest may be not enough. We
> may also stop processing incoming packets.

Yes, and similar to virtio-serial.

Thanks,
Fam