On Mon, 26 Sep 2016 17:35:38 +0100 Stefan Hajnoczi <stefa...@redhat.com> wrote:
> On Mon, Sep 26, 2016 at 10:34:56AM +0200, Greg Kurz wrote:
> > The virtio_scsi_bad_req() function is called when a guest sends a
> > request with missing or ill-sized headers. This generally happens
> > when the virtio_scsi_parse_req() function returns an error.
> >
> > With this patch, virtio_scsi_bad_req() will mark the device as broken,
> > detach the request from the virtqueue and free it, instead of forcing
> > QEMU to exit.
> >
> > In nearly all locations where virtio_scsi_bad_req() is called, the only
> > thing to do next is to return to the caller.
> >
> > The virtio_scsi_handle_cmd_req_prepare() function is an exception though.
> >
> > It is called in a loop by virtio_scsi_handle_cmd_vq() and passed requests
> > freshly popped from a cmd virtqueue; virtio_scsi_handle_cmd_req_prepare()
> > does some sanity checks on the request and returns a boolean flag to
> > indicate whether the request should be queued or not. In the latter case,
> > virtio_scsi_handle_cmd_req_prepare() has detected a non-fatal error and
> > sent a response back to the guest.
> >
> > We have now a new condition to take into account: the device is broken
> > and should stop all processing.
> >
> > The return value of virtio_scsi_handle_cmd_req_prepare() is hence changed
> > to an int. A return value of zero means that the request should be queued.
> > Other non-fatal error cases where the reqyest shoudn't be queued return
>
> s/reqyest/request/
>
oops...
> > @@ -574,11 +578,24 @@ static void
> > virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
> > void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
> > {
> > VirtIOSCSIReq *req, *next;
> > + int ret;
> > +
> > QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
> >
> > while ((req = virtio_scsi_pop_req(s, vq))) {
> > - if (virtio_scsi_handle_cmd_req_prepare(s, req)) {
> > + ret = virtio_scsi_handle_cmd_req_prepare(s, req);
> > + if (!ret) {
> > QTAILQ_INSERT_TAIL(&reqs, req, next);
> > + } else if (ret == -EINVAL) {
> > + /* The device is broken and shouldn't process any request */
> > + while (!QTAILQ_EMPTY(&reqs)) {
> > + req = QTAILQ_FIRST(&reqs);
> > + QTAILQ_REMOVE(&reqs, req, next);
> > + blk_io_unplug(req->sreq->dev->conf.blk);
>
> Are you sure blk_io_plug() was called for this request? If we returned
> early in virtio_scsi_handle_cmd_req_prepare() then it wasn't called.
>
Early return in virtio_scsi_handle_cmd_req_prepare() means an error was
detected, in which case the request didn't get queued; we are sure that
blk_io_plug() was called for all requests in this queue.
> > + scsi_req_unref(req->sreq);
>
> Which scsi_req_ref() is this paired with? If it's the call in
> scsi_req_enqueue() then that function was never called and we shouldn't
> unref.
It is paired with the one in virtio_scsi_handle_cmd_req_prepare(), which
is called just before blk_io_plug().
But looking at the patch again, I realize I missed this:
@@ -562,7 +562,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s,
}
scsi_req_ref(req->sreq);
blk_io_plug(d->conf.blk);
- return true;
+ return 0;
}
static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req
I'll send a v4.
Cheers.
--
Greg
pgpMaSx7miLML.pgp
Description: OpenPGP digital signature
