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 Wed, 28 Jun 2017 11:54:28 -0700 (PDT)
> Stefano Stabellini  wrote:
> [...]
> > > > @@ -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(_9pfs->rings[pdu->tag % xen_9pfs->num_rings],
> > > > in_sg, , 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(_9pfs->xendev, 0,
> > > > +  "Failed to encode VirtFS request type %d\n", 
> > > > pdu->id + 1);
> > > > +xen_be_set_state(_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.
> > 
> 
> Oh ok, I see.
> 
> > 
> > > > +xen_9pfs_disconnect(_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(_9pfs->rings[pdu->tag % xen_9pfs->num_rings],
> > > >  out_sg, , 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(_9pfs->xendev, 0,
> > > > +  "Failed to decode VirtFS request type %d\n", 
> > > > pdu->id);
> > > > +xen_be_set_state(_9pfs->xendev, XenbusStateClosing);
> > > > +xen_9pfs_disconnect(_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 = _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, , pdu->idx, size);
> > > > +
> > > > +buf_size = iov_size(ring->sg, num);
> > > > +if (buf_size  < size) {
> > > > +xen_pv_printf(_9pfs->xendev, 0, "Xen 9pfs request type %d"
> > > > +"needs %zu bytes, buffer has %zu\n", pdu->id, size,
> > > > +buf_size);
> > > > +xen_be_set_state(_9pfs->xendev, XenbusStateClosing);
> > > > +xen_9pfs_disconnect(_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.
> > 
> 
> Makes sense.
> 
> > 
> > > >  }
> 

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

[Greg Kurz]

On Wed, 28 Jun 2017 11:54:28 -0700 (PDT)
Stefano Stabellini  wrote:
[...]
> > > @@ -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(_9pfs->rings[pdu->tag % xen_9pfs->num_rings],
> > > in_sg, , 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(_9pfs->xendev, 0,
> > > +  "Failed to encode VirtFS request type %d\n", 
> > > pdu->id + 1);
> > > +xen_be_set_state(_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.
> 

Oh ok, I see.

> 
> > > +xen_9pfs_disconnect(_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(_9pfs->rings[pdu->tag % xen_9pfs->num_rings],
> > >  out_sg, , 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(_9pfs->xendev, 0,
> > > +  "Failed to decode VirtFS request type %d\n", 
> > > pdu->id);
> > > +xen_be_set_state(_9pfs->xendev, XenbusStateClosing);
> > > +xen_9pfs_disconnect(_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 = _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, , pdu->idx, size);
> > > +
> > > +buf_size = iov_size(ring->sg, num);
> > > +if (buf_size  < size) {
> > > +xen_pv_printf(_9pfs->xendev, 0, "Xen 9pfs request type %d"
> > > +"needs %zu bytes, buffer has %zu\n", pdu->id, size,
> > > +buf_size);
> > > +xen_be_set_state(_9pfs->xendev, XenbusStateClosing);
> > > +xen_9pfs_disconnect(_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.
> 

Makes sense.

> 
> > >  }
> > >  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) {
> > > +

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  wrote:
> 
> > On Tue, 27 Jun 2017, Greg Kurz wrote:
> > > On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
> > > Stefano Stabellini  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 
> > > > > ---
> > > > > 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, , );
> > > > > +pdu->s->transport->init_out_iov_from_pdu(pdu, , , 
> > > > > size + skip);
> > > > >  } else {
> > > > >  pdu->s->transport->init_in_iov_from_pdu(pdu, , , 
> > > > > 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 

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

[Greg Kurz]

On Tue, 27 Jun 2017 16:34:51 -0700 (PDT)
Stefano Stabellini  wrote:

> On Tue, 27 Jun 2017, Greg Kurz wrote:
> > On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
> > Stefano Stabellini  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 
> > > > ---
> > > > 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, , );
> > > > +pdu->s->transport->init_out_iov_from_pdu(pdu, , , 
> > > > size + skip);
> > > >  } else {
> > > >  pdu->s->transport->init_in_iov_from_pdu(pdu, , , 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 

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

[Stefano Stabellini]

On Tue, 27 Jun 2017, Greg Kurz wrote:
> On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
> Stefano Stabellini  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 
> > > ---
> > > 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, , );
> > > +pdu->s->transport->init_out_iov_from_pdu(pdu, , , size 
> > > + skip);
> > >  } else {
> > >  pdu->s->transport->init_in_iov_from_pdu(pdu, , , 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 = 

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

[Greg Kurz]

On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
Stefano Stabellini  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 
> > ---
> > 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, , );
> > +pdu->s->transport->init_out_iov_from_pdu(pdu, , , size + 
> > skip);
> >  } else {
> >  pdu->s->transport->init_in_iov_from_pdu(pdu, , , 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;

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

[Stefano Stabellini]

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 
> ---
> 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, , );
> +pdu->s->transport->init_out_iov_from_pdu(pdu, , , size + 
> skip);
>  } else {
>  pdu->s->transport->init_in_iov_from_pdu(pdu, , , 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


> +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


> -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 =