Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured

[Stefano Stabellini]

On Wed, 28 Jun 2017, Greg Kurz wrote:
> On Tue, 27 Jun 2017 16:34:51 -0700 (PDT)
> Stefano Stabellini <address@hidden> wrote:
> 
> > On Tue, 27 Jun 2017, Greg Kurz wrote:
> > > On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
> > > Stefano Stabellini <address@hidden> wrote:
> > >   
> > > > On Fri, 23 Jun 2017, Greg Kurz wrote:  
> > > > > The 9P protocol is transport agnostic: if the guest misconfigured the
> > > > > buffers, the best we can do is to set the broken flag on the device.
> > > > > 
> > > > > Since virtio_pdu_vmarshal() may be called by several active PDUs, we
> > > > > check if the transport isn't broken already to avoid printing extra
> > > > > error messages.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > > ---
> > > > > v4: - update changelog and add comment to explain why we check 
> > > > > vdev->broken
> > > > >       in virtio_pdu_vmarshal()
> > > > >     - dropped uneeded vdev->broken check in virtio_pdu_vunmarshal()
> > > > > ---
> > > > >  hw/9pfs/9p.c               |    2 +-
> > > > >  hw/9pfs/9p.h               |    2 +-
> > > > >  hw/9pfs/virtio-9p-device.c |   48 
> > > > > +++++++++++++++++++++++++++++++++++++++-----
> > > > >  hw/9pfs/xen-9p-backend.c   |    3 ++-
> > > > >  4 files changed, 47 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > > index a0ae98f7ca6f..8e5cac71eb60 100644
> > > > > --- a/hw/9pfs/9p.c
> > > > > +++ b/hw/9pfs/9p.c
> > > > > @@ -1664,7 +1664,7 @@ static void 
> > > > > v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
> > > > >      unsigned int niov;
> > > > >  
> > > > >      if (is_write) {
> > > > > -        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov);
> > > > > +        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, 
> > > > > size + skip);
> > > > >      } else {
> > > > >          pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, 
> > > > > size + skip);
> > > > >      }
> > > > > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > > > > index aac1b0b2ce3d..d1cfeaf10e4f 100644
> > > > > --- a/hw/9pfs/9p.h
> > > > > +++ b/hw/9pfs/9p.h
> > > > > @@ -363,7 +363,7 @@ struct V9fsTransport {
> > > > >      void        (*init_in_iov_from_pdu)(V9fsPDU *pdu, struct iovec 
> > > > > **piov,
> > > > >                                          unsigned int *pniov, size_t 
> > > > > size);
> > > > >      void        (*init_out_iov_from_pdu)(V9fsPDU *pdu, struct iovec 
> > > > > **piov,
> > > > > -                                         unsigned int *pniov);
> > > > > +                                         unsigned int *pniov, size_t 
> > > > > size);
> > > > >      void        (*push_and_notify)(V9fsPDU *pdu);
> > > > >  };
> > > > >  
> > > > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > > > > index 1a68c1622d3a..ed9e4817a26c 100644
> > > > > --- a/hw/9pfs/virtio-9p-device.c
> > > > > +++ b/hw/9pfs/virtio-9p-device.c
> > > > > @@ -146,8 +146,22 @@ static ssize_t virtio_pdu_vmarshal(V9fsPDU *pdu, 
> > > > > size_t offset,
> > > > >      V9fsState *s = pdu->s;
> > > > >      V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> > > > >      VirtQueueElement *elem = v->elems[pdu->idx];
> > > > > -
> > > > > -    return v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, 
> > > > > fmt, ap);
> > > > > +    int ret;    
> > > > 
> > > > I think ret should be ssize_t
> > > >   
> > > 
> > > Yes, you're right. I'll change that.
> > >   
> > > >   
> > > > > +    ret = v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, 
> > > > > fmt, ap);
> > > > > +    if (ret < 0) {
> > > > > +        VirtIODevice *vdev = VIRTIO_DEVICE(v);
> > > > > +
> > > > > +        /* Any active PDU may try to send something back to the 
> > > > > client without
> > > > > +         * knowing if the transport is broken or not. This could 
> > > > > result in
> > > > > +         * MAX_REQ - 1 (ie, 127) extra error messages being printed.
> > > > > +         */
> > > > > +        if (!vdev->broken) {
> > > > > +            virtio_error(vdev, "Failed to encode VirtFS reply type 
> > > > > %d",
> > > > > +                         pdu->id + 1);
> > > > > +        }
> > > > > +    }
> > > > > +    return ret;
> > > > >  }
> > > > >  
> > > > >  static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
> > > > > @@ -156,28 +170,52 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU 
> > > > > *pdu, size_t offset,
> > > > >      V9fsState *s = pdu->s;
> > > > >      V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> > > > >      VirtQueueElement *elem = v->elems[pdu->idx];
> > > > > +    int ret;    
> > > > 
> > > > same here
> > > >   
> > > 
> > > Ditto.
> > >   
> > > >   
> > > > > -    return v9fs_iov_vunmarshal(elem->out_sg, elem->out_num, offset, 
> > > > > 1, fmt, ap);
> > > > > +    ret = v9fs_iov_vunmarshal(elem->out_sg, elem->out_num, offset, 
> > > > > 1, fmt, ap);
> > > > > +    if (ret < 0) {
> > > > > +        VirtIODevice *vdev = VIRTIO_DEVICE(v);
> > > > > +
> > > > > +        virtio_error(vdev, "Failed to decode VirtFS request type 
> > > > > %d", pdu->id);
> > > > > +    }
> > > > > +    return ret;
> > > > >  }
> > > > >  
> > > > > -/* The size parameter is used by other transports. Do not drop it. */
> > > > >  static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec 
> > > > > **piov,
> > > > >                                          unsigned int *pniov, size_t 
> > > > > size)
> > > > >  {
> > > > >      V9fsState *s = pdu->s;
> > > > >      V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> > > > >      VirtQueueElement *elem = v->elems[pdu->idx];
> > > > > +    size_t buf_size = iov_size(elem->in_sg, elem->in_num);
> > > > > +
> > > > > +    if (buf_size < size) {
> > > > > +        VirtIODevice *vdev = VIRTIO_DEVICE(v);
> > > > > +
> > > > > +        virtio_error(vdev,
> > > > > +                     "VirtFS reply type %d needs %zu bytes, buffer 
> > > > > has %zu",
> > > > > +                     pdu->id + 1, size, buf_size);
> > > > > +    }
> > > > >  
> > > > >      *piov = elem->in_sg;
> > > > >      *pniov = elem->in_num;
> > > > >  }
> > > > >  
> > > > >  static void virtio_init_out_iov_from_pdu(V9fsPDU *pdu, struct iovec 
> > > > > **piov,
> > > > > -                                         unsigned int *pniov)
> > > > > +                                         unsigned int *pniov, size_t 
> > > > > size)
> > > > >  {
> > > > >      V9fsState *s = pdu->s;
> > > > >      V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> > > > >      VirtQueueElement *elem = v->elems[pdu->idx];
> > > > > +    size_t buf_size = iov_size(elem->out_sg, elem->out_num);
> > > > > +
> > > > > +    if (buf_size < size) {
> > > > > +        VirtIODevice *vdev = VIRTIO_DEVICE(v);
> > > > > +
> > > > > +        virtio_error(vdev,
> > > > > +                     "VirtFS request type %d needs %zu bytes, buffer 
> > > > > has %zu",
> > > > > +                     pdu->id, size, buf_size);
> > > > > +    }
> > > > >  
> > > > >      *piov = elem->out_sg;
> > > > >      *pniov = elem->out_num;
> > > > > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > > > > index 922cc967be63..a82cf817fe45 100644
> > > > > --- a/hw/9pfs/xen-9p-backend.c
> > > > > +++ b/hw/9pfs/xen-9p-backend.c
> > > > > @@ -147,7 +147,8 @@ static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU 
> > > > > *pdu,
> > > > >  
> > > > >  static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu,
> > > > >                                             struct iovec **piov,
> > > > > -                                           unsigned int *pniov)
> > > > > +                                           unsigned int *pniov,
> > > > > +                                           size_t size)
> > > > >  {
> > > > >      Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state);
> > > > >      Xen9pfsRing *ring = &xen_9pfs->rings[pdu->tag % 
> > > > > xen_9pfs->num_rings];    
> > > > 
> > > > Maybe you could include the same changes you made for xen, see below
> > > >   
> > > 
> > > Sure, I'd be glad to do so. I just have one concern. In the virtio case, 
> > > we call
> > > virtio_error() which does two things: print an error message AND set the 
> > > device
> > > broken flag (no more I/O until device is reset). Is there something 
> > > similar with
> > > the xen transport ?  
> > 
> > No, there isn't anything like that yet. The backend is also missing the
> > implementation of "disconnect", I think it is the right time to
> > introduce it. I made enough changes that I think they deserve their own
> > patch, but I would be happy if you carried it in your series.
> > 
> 
> Ok, the patch looks ok. I'll add it to my series. Just a couple of 
> questions...

Thanks!


> > ---
> > xen-9pfs: disconnect if buffers are misconfigured 
> > 
> > Implement xen_9pfs_disconnect by unbinding the event channels. On
> > xen_9pfs_free, call disconnect if any event channels haven't been
> > disconnected.
> > 
> > If the frontend misconfigured the buffers set the backend to "Closing"
> > and disconnect it. Misconfigurations include requesting a read of more
> > bytes than available on the ring buffer, or claiming to be writing more
> > data than available on the ring buffer.
> > 
> > Signed-off-by: Stefano Stabellini <address@hidden>
> > 
> > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > index a82cf81..8db4703 100644
> > --- a/hw/9pfs/xen-9p-backend.c
> > +++ b/hw/9pfs/xen-9p-backend.c
> > @@ -54,6 +54,8 @@ typedef struct Xen9pfsDev {
> >      Xen9pfsRing *rings;
> >  } Xen9pfsDev;
> >  
> > +static void xen_9pfs_disconnect(struct XenDevice *xendev);
> > +
> >  static void xen_9pfs_in_sg(Xen9pfsRing *ring,
> >                             struct iovec *in_sg,
> >                             int *num,
> > @@ -125,10 +127,19 @@ static ssize_t xen_9pfs_pdu_vmarshal(V9fsPDU *pdu,
> >      Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state);
> >      struct iovec in_sg[2];
> >      int num;
> > +    ssize_t ret;
> >  
> >      xen_9pfs_in_sg(&xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings],
> >                     in_sg, &num, pdu->idx, ROUND_UP(offset + 128, 512));
> > -    return v9fs_iov_vmarshal(in_sg, num, offset, 0, fmt, ap);
> > +    
> > +    ret = v9fs_iov_vmarshal(in_sg, num, offset, 0, fmt, ap);
> > +    if (ret < 0) {
> > +        xen_pv_printf(&xen_9pfs->xendev, 0,
> > +                      "Failed to encode VirtFS request type %d\n", pdu->id 
> > + 1);
> > +        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> 
> Shouldn't the state be set in xen_9pfs_disconnect() ?

No, because xen_9pfs_disconnect is also used as a callback from
hw/xen/xen_backend.c, which also sets the state to XenbusStateClosing,
see xen_be_disconnect.


> > +        xen_9pfs_disconnect(&xen_9pfs->xendev);
> > +    }
> > +    return ret;
> >  }
> >  
> >  static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU *pdu,
> > @@ -139,10 +150,19 @@ static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU *pdu,
> >      Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state);
> >      struct iovec out_sg[2];
> >      int num;
> > +    ssize_t ret;
> >  
> >      xen_9pfs_out_sg(&xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings],
> >                      out_sg, &num, pdu->idx);
> > -    return v9fs_iov_vunmarshal(out_sg, num, offset, 0, fmt, ap);
> > +    
> > +    ret = v9fs_iov_vunmarshal(out_sg, num, offset, 0, fmt, ap);
> > +    if (ret < 0) {
> > +        xen_pv_printf(&xen_9pfs->xendev, 0,
> > +                      "Failed to decode VirtFS request type %d\n", 
> > pdu->id);
> > +        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> > +        xen_9pfs_disconnect(&xen_9pfs->xendev);
> 
> Same here
> 
> > +    }
> > +    return ret;
> >  }
> >  
> >  static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu,
> > @@ -170,11 +190,22 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU 
> > *pdu,
> >      Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state);
> >      Xen9pfsRing *ring = &xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings];
> >      int num;
> > +    size_t buf_size;
> >  
> >      g_free(ring->sg);
> >  
> >      ring->sg = g_malloc0(sizeof(*ring->sg) * 2);
> >      xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> > +
> > +    buf_size = iov_size(ring->sg, num);
> > +    if (buf_size  < size) {
> > +        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
> > +                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
> > +                buf_size);
> > +        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> > +        xen_9pfs_disconnect(&xen_9pfs->xendev);
> > +    }
> > +
> >      *piov = ring->sg;
> >      *pniov = num;
> >  }
> > @@ -218,7 +249,7 @@ static int xen_9pfs_init(struct XenDevice *xendev)
> >  static int xen_9pfs_receive(Xen9pfsRing *ring)
> >  {
> >      P9MsgHeader h;
> > -    RING_IDX cons, prod, masked_prod, masked_cons;
> > +    RING_IDX cons, prod, masked_prod, masked_cons, queued;
> >      V9fsPDU *pdu;
> >  
> >      if (ring->inprogress) {
> > @@ -229,8 +260,8 @@ static int xen_9pfs_receive(Xen9pfsRing *ring)
> >      prod = ring->intf->out_prod;
> >      xen_rmb();
> >  
> > -    if (xen_9pfs_queued(prod, cons, XEN_FLEX_RING_SIZE(ring->ring_order)) <
> > -        sizeof(h)) {
> > +    queued = xen_9pfs_queued(prod, cons, 
> > XEN_FLEX_RING_SIZE(ring->ring_order));
> > +    if (queued < sizeof(h)) {
> >          return 0;
> 
> Shouldn't this return an error as well ?

Well spotted! But I am actually thinking that I shouldn't be returning
errors at all now from xen_9pfs_receive. The function xen_9pfs_receive
is always called to check for updates. It can be called even when there
are no updates available. In those cases, there isn't enough data on the
ring to proceed. I think it is correct to return 0 here.

The more difficult case is below. What if the data is only partially
written to the ring? The check (queued < ring->out_size) would fail. I
think we should return 0 (not an error) there too, because the other end
could be in the middle of writing. But obviously we shouldn't proceed
either.


> >      }
> >      ring->inprogress = true;
> > @@ -247,6 +278,15 @@ static int xen_9pfs_receive(Xen9pfsRing *ring)
> >      ring->out_size = le32_to_cpu(h.size_le);
> >      ring->out_cons = cons + le32_to_cpu(h.size_le);
> >  
> > +    if (queued < ring->out_size) {
> > +        xen_pv_printf(&ring->priv->xendev, 0, "Xen 9pfs request type %d"
> > +                "needs %u bytes, buffer has %u\n", pdu->id, ring->out_size,
> > +                queued);
> > +        xen_be_set_state(&ring->priv->xendev, XenbusStateClosing);
> > +        xen_9pfs_disconnect(&ring->priv->xendev);
> > +        return -EINVAL;
> > +    }

So I think this should just be

    if (queued < ring->out_size) {
        return 0;
    }


> >      pdu_submit(pdu, &h);
> >  
> >      return 0;
> > @@ -269,15 +309,30 @@ static void xen_9pfs_evtchn_event(void *opaque)
> >      qemu_bh_schedule(ring->bh);
> >  }
> >  
> > -static int xen_9pfs_free(struct XenDevice *xendev)
> > +static void xen_9pfs_disconnect(struct XenDevice *xendev)
> >  {
> > +    Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
> >      int i;
> > +
> > +    for (i = 0; i < xen_9pdev->num_rings; i++) {
> > +        if (xen_9pdev->rings[i].evtchndev != NULL) {
> > +            
> > qemu_set_fd_handler(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
> > +                    NULL, NULL, NULL);
> > +            xenevtchn_unbind(xen_9pdev->rings[i].evtchndev,
> > +                             xen_9pdev->rings[i].local_port);
> > +            xen_9pdev->rings[i].evtchndev = NULL;
> > +        }
> > +    }
> > +}
> > +
> > +static int xen_9pfs_free(struct XenDevice *xendev)
> > +{
> >      Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
> > +    int i;
> >  
> > -    g_free(xen_9pdev->id);
> > -    g_free(xen_9pdev->tag);
> > -    g_free(xen_9pdev->path);
> > -    g_free(xen_9pdev->security_model);
> > +    if (xen_9pdev->rings[0].evtchndev != NULL) {
> > +        xen_9pfs_disconnect(xendev);
> > +    }
> >  
> >      for (i = 0; i < xen_9pdev->num_rings; i++) {
> >          if (xen_9pdev->rings[i].data != NULL) {
> > @@ -290,16 +345,15 @@ static int xen_9pfs_free(struct XenDevice *xendev)
> >                      xen_9pdev->rings[i].intf,
> >                      1);
> >          }
> > -        if (xen_9pdev->rings[i].evtchndev > 0) {
> > -            
> > qemu_set_fd_handler(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
> > -                    NULL, NULL, NULL);
> > -            xenevtchn_unbind(xen_9pdev->rings[i].evtchndev,
> > -                             xen_9pdev->rings[i].local_port);
> > -        }
> >          if (xen_9pdev->rings[i].bh != NULL) {
> >              qemu_bh_delete(xen_9pdev->rings[i].bh);
> >          }
> >      }
> > +
> > +    g_free(xen_9pdev->id);
> > +    g_free(xen_9pdev->tag);
> > +    g_free(xen_9pdev->path);
> > +    g_free(xen_9pdev->security_model);
> >      g_free(xen_9pdev->rings);
> >      return 0;
> >  }
> > @@ -423,11 +477,6 @@ static void xen_9pfs_alloc(struct XenDevice *xendev)
> >      xenstore_write_be_int(xendev, "max-ring-page-order", MAX_RING_ORDER);
> >  }
> >  
> > -static void xen_9pfs_disconnect(struct XenDevice *xendev)
> > -{
> > -    /* Dynamic hotplug of PV filesystems at runtime is not supported. */
> > -}
> > -
> >  struct XenDevOps xen_9pfs_ops = {
> >      .size       = sizeof(Xen9pfsDev),
> >      .flags      = DEVOPS_FLAG_NEED_GNTDEV,