[Qemu-devel] [PATCH v4 0/4] 9pfs: handle transport errors

[Qemu-devel] [PATCH v4 0/4] 9pfs: handle transport errors

[Greg Kurz]

The 9p protocol relies on a reliable transport, but the current code
treats transport errors (ie, failure to marshal or unmarshal) as if
they were coming from the backend. This doesn't make sense: if the
transport failed, we should notify the guest that the transport is
broken and needs to be reset, using transport specific means.

This series only modifies the virtio transport so that it can notify
the guest about transport failures.

Changes since v3:
- addressed comment from MST (patch 3)

--
Greg

---

Greg Kurz (4):
      virtio-9p: record element after sanity checks
      virtio-9p: message header is 7-byte long
      virtio-9p: break device if buffers are misconfigured
      9pfs: handle transport errors in pdu_complete()


 hw/9pfs/9p.c               |   25 ++++++++++++-------
 hw/9pfs/9p.h               |    7 ++++-
 hw/9pfs/virtio-9p-device.c |   59 ++++++++++++++++++++++++++++++++++++--------
 hw/9pfs/xen-9p-backend.c   |    3 +-
 4 files changed, 72 insertions(+), 22 deletions(-)

[Qemu-devel] [PATCH v4 1/4] virtio-9p: record element after sanity checks

[Greg Kurz]

If the guest sends a malformed request, we end up with a dangling pointer
in V9fsVirtioState. This doesn't seem to cause any bug, but let's remove
this side effect anyway.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/9pfs/virtio-9p-device.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 245abd8aaef1..3380bfc0c551 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -61,7 +61,6 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
         }
         QEMU_BUILD_BUG_ON(sizeof(out) != 7);
 
-        v->elems[pdu->idx] = elem;
         len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                          &out, sizeof(out));
         if (len != sizeof(out)) {
@@ -70,6 +69,8 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
             goto out_free_req;
         }
 
+        v->elems[pdu->idx] = elem;
+
         pdu_submit(pdu, &out);
     }
 

[Qemu-devel] [PATCH v4 2/4] virtio-9p: message header is 7-byte long

[Greg Kurz]

The 9p spec at http://man.cat-v.org/plan_9/5/intro reads:

 "Each 9P message begins with a four-byte size field specify-
  ing the length in bytes of the complete message including
  the four bytes of the size field itself.  The next byte is
  the message type, one of the constants in the enumeration in
  the include file <fcall.h>.  The next two bytes are an iden-
  tifying tag, described below."

ie, each message starts with a 7-byte long header.

The core 9P code already assumes this pretty much everywhere. This patch
does the following:
- makes the assumption explicit in the common 9p.h header, since it isn't
  related to the transport
- open codes the header size in handle_9p_output() and hardens the sanity
  check on the space needed for the reply message

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.h               |    5 +++++
 hw/9pfs/virtio-9p-device.c |    8 +++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index c886ba78d2ee..aac1b0b2ce3d 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -124,6 +124,11 @@ typedef struct {
     uint8_t id;
     uint16_t tag_le;
 } QEMU_PACKED P9MsgHeader;
+/* According to the specification, 9p messages start with a 7-byte header.
+ * Since most of the code uses this header size in literal form, we must be
+ * sure this is indeed the case.
+ */
+QEMU_BUILD_BUG_ON(sizeof(P9MsgHeader) != 7);
 
 struct V9fsPDU
 {
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 3380bfc0c551..1a68c1622d3a 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -53,17 +53,15 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
             goto out_free_pdu;
         }
 
-        if (elem->in_num == 0) {
+        if (iov_size(elem->in_sg, elem->in_num) < 7) {
             virtio_error(vdev,
                          "The guest sent a VirtFS request without space for "
                          "the reply");
             goto out_free_req;
         }
-        QEMU_BUILD_BUG_ON(sizeof(out) != 7);
 
-        len = iov_to_buf(elem->out_sg, elem->out_num, 0,
-                         &out, sizeof(out));
-        if (len != sizeof(out)) {
+        len = iov_to_buf(elem->out_sg, elem->out_num, 0, &out, 7);
+        if (len != 7) {
             virtio_error(vdev, "The guest sent a malformed VirtFS request: "
                          "header size is %zd, should be 7", len);
             goto out_free_req;

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

[Greg Kurz]

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 <groug@kaod.org>
---
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;
+
+    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;
 
-    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];

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 <groug@kaod.org>
> ---
> 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


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


diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index a82cf81..83a2bfe 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -125,10 +125,17 @@ 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", pdu->id + 1);
+    }
+    return ret;
 }
 
 static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU *pdu,
@@ -139,10 +146,17 @@ 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", pdu->id);
+    }
+    return ret;
 }
 
 static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu,
@@ -153,11 +167,20 @@ static void xen_9pfs_init_out_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_out_sg(ring, ring->sg, &num, pdu->idx);
+
+    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", pdu->id, size,
+                buf_size);
+    }
+
     *piov = ring->sg;
     *pniov = num;
 }
@@ -170,11 +193,20 @@ 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", pdu->id, size,
+                buf_size);
+    }
+
     *piov = ring->sg;
     *pniov = num;
 }

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 10260 bytes --]

On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
Stefano Stabellini <sstabellini@kernel.org> 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 <groug@kaod.org>
> > ---
> > 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 ?

> 
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index a82cf81..83a2bfe 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -125,10 +125,17 @@ 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", pdu->id + 1);
> +    }
> +    return ret;
>  }
>  
>  static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU *pdu,
> @@ -139,10 +146,17 @@ 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", pdu->id);
> +    }
> +    return ret;
>  }
>  
>  static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu,
> @@ -153,11 +167,20 @@ static void xen_9pfs_init_out_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_out_sg(ring, ring->sg, &num, pdu->idx);
> +
> +    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", pdu->id, size,
> +                buf_size);
> +    }
> +
>      *piov = ring->sg;
>      *pniov = num;
>  }
> @@ -170,11 +193,20 @@ 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", pdu->id, size,
> +                buf_size);
> +    }
> +
>      *piov = ring->sg;
>      *pniov = num;
>  }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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 <sstabellini@kernel.org> 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 <groug@kaod.org>
> > > ---
> > > 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.

---
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 <stefano@aporeto.com>

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);
+        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);
+    }
+    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;
     }
     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;
+    }
+
     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,

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 16019 bytes --]

On Tue, 27 Jun 2017 16:34:51 -0700 (PDT)
Stefano Stabellini <sstabellini@kernel.org> wrote:

> On Tue, 27 Jun 2017, Greg Kurz wrote:
> > On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
> > Stefano Stabellini <sstabellini@kernel.org> 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 <groug@kaod.org>
> > > > ---
> > > > 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...

> ---
> 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 <stefano@aporeto.com>
> 
> 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() ?

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

>      }
>      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;
> +    }
> +
>      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,


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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 <sstabellini@kernel.org> wrote:
> 
> > On Tue, 27 Jun 2017, Greg Kurz wrote:
> > > On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
> > > Stefano Stabellini <sstabellini@kernel.org> 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 <groug@kaod.org>
> > > > > ---
> > > > > 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 <stefano@aporeto.com>
> > 
> > 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,

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 8564 bytes --]

On Wed, 28 Jun 2017 11:54:28 -0700 (PDT)
Stefano Stabellini <sstabellini@kernel.org> 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(&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.
> 

Oh ok, I see.

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

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

Ok, I'll change it in your patch.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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 <sstabellini@kernel.org> 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(&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.
> > 
> 
> Oh ok, I see.
> 
> > 
> > > > +        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.
> > 
> 
> 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) {
> > > > +        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;
> >     }
> 
> Ok, I'll change it in your patch.

Thank you!


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

[Qemu-devel] [PATCH v4 4/4] 9pfs: handle transport errors in pdu_complete()

[Greg Kurz]

Contrary to what is written in the comment, a buggy guest can misconfigure
the transport buffers and pdu_marshal() may return an error.  If this ever
happens, it is up to the transport layer to handle the situation (9P is
transport agnostic).

This fixes Coverity issue CID1348518.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 8e5cac71eb60..6c92bad5b3b4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -624,15 +624,11 @@ void pdu_free(V9fsPDU *pdu)
     QLIST_INSERT_HEAD(&s->free_list, pdu, next);
 }
 
-/*
- * We don't do error checking for pdu_marshal/unmarshal here
- * because we always expect to have enough space to encode
- * error details
- */
 static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
 {
     int8_t id = pdu->id + 1; /* Response */
     V9fsState *s = pdu->s;
+    int ret;
 
     if (len < 0) {
         int err = -len;
@@ -644,11 +640,19 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
             str.data = strerror(err);
             str.size = strlen(str.data);
 
-            len += pdu_marshal(pdu, len, "s", &str);
+            ret = pdu_marshal(pdu, len, "s", &str);
+            if (ret < 0) {
+                goto out_notify;
+            }
+            len += ret;
             id = P9_RERROR;
         }
 
-        len += pdu_marshal(pdu, len, "d", err);
+        ret = pdu_marshal(pdu, len, "d", err);
+        if (ret < 0) {
+            goto out_notify;
+        }
+        len += ret;
 
         if (s->proto_version == V9FS_PROTO_2000L) {
             id = P9_RLERROR;
@@ -657,12 +661,15 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
     }
 
     /* fill out the header */
-    pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag);
+    if (pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag) < 0) {
+        goto out_notify;
+    }
 
     /* keep these in sync */
     pdu->size = len;
     pdu->id = id;
 
+out_notify:
     pdu->s->transport->push_and_notify(pdu);
 
     /* Now wakeup anybody waiting in flush for this request */