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

[Qemu-devel] [PATCH v2 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 modifies the existing virtio-9p transport so that it can
notify the guest about transport failures. The core 9p code is modified
as well so that it stops handling requests when the transport fails.

Changes since v1:
- dropped the "virtio: Error object based virtio_error()" patch
- see patches for detailed changes

--
Greg

---

Greg Kurz (4):
      fsdev: don't allow unknown format in marshal/unmarshal
      9pfs: drop pdu_push_and_notify()
      virtio-9p: factor out virtio_9p_error_err()
      9pfs: handle broken transport


 fsdev/9p-iov-marshal.c     |    4 ++-
 hw/9pfs/9p.c               |   52 ++++++++++++++++++++++++++++++++------------
 hw/9pfs/9p.h               |    1 +
 hw/9pfs/virtio-9p-device.c |   51 +++++++++++++++++++++++++++++--------------
 4 files changed, 75 insertions(+), 33 deletions(-)

[Qemu-devel] [PATCH v2 1/4] fsdev: don't allow unknown format in marshal/unmarshal

[Greg Kurz]

The code only uses well known format strings. An unknown format token is a
bug.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/9p-iov-marshal.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index 1d16f8df4bd4..a1c9beddd2e7 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -168,7 +168,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
             break;
         }
         default:
-            break;
+            g_assert_not_reached();
         }
         if (copied < 0) {
             return copied;
@@ -281,7 +281,7 @@ ssize_t v9fs_iov_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
             break;
         }
         default:
-            break;
+            g_assert_not_reached();
         }
         if (copied < 0) {
             return copied;

[Qemu-devel] [PATCH v2 2/4] 9pfs: drop pdu_push_and_notify()

[Greg Kurz]

Only pdu_complete() needs to notify the client that a request has completed.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c80ba67389ce..01deffa0c3b5 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -65,11 +65,6 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
     return ret;
 }
 
-static void pdu_push_and_notify(V9fsPDU *pdu)
-{
-    pdu->s->transport->push_and_notify(pdu);
-}
-
 static int omode_to_uflags(int8_t mode)
 {
     int ret = 0;
@@ -668,7 +663,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
     pdu->size = len;
     pdu->id = id;
 
-    pdu_push_and_notify(pdu);
+    pdu->s->transport->push_and_notify(pdu);
 
     /* Now wakeup anybody waiting in flush for this request */
     if (!qemu_co_queue_next(&pdu->complete)) {

[Qemu-devel] [PATCH v2 3/4] virtio-9p: factor out virtio_9p_error_err()

[Greg Kurz]

When an unrecoverable is hit, we need to set the broken flag of the virtio
device, detach the queue element and free it. This is currently open coded
in handle_9p_output(). It is fine since this is the only function that can
set the broken flag. But if we want to be able to do this from other places,
we must consolidate the logic in a helper.

Signed-off-by: Greg Kurz <groug@kaod.org>
--
v2: - rely on the existing virtio_error() API
---
 hw/9pfs/virtio-9p-device.c |   35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 3782f437029b..c71659823fdc 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -22,21 +22,32 @@
 
 static const struct V9fsTransport virtio_9p_transport;
 
+static void virtio_9p_free_element(V9fsVirtioState *v, unsigned int idx)
+{
+    VirtQueueElement **pelem = &v->elems[idx];
+    g_free(*pelem);
+    *pelem = NULL;
+}
+
 static void virtio_9p_push_and_notify(V9fsPDU *pdu)
 {
     V9fsState *s = pdu->s;
     V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
-    VirtQueueElement *elem = v->elems[pdu->idx];
 
     /* push onto queue and notify */
-    virtqueue_push(v->vq, elem, pdu->size);
-    g_free(elem);
-    v->elems[pdu->idx] = NULL;
+    virtqueue_push(v->vq, v->elems[pdu->idx], pdu->size);
+    virtio_9p_free_element(v, pdu->idx);
 
     /* FIXME: we should batch these completions */
     virtio_notify(VIRTIO_DEVICE(v), v->vq);
 }
 
+#define virtio_9p_error(v, idx, ...) {                 \
+    virtio_error(VIRTIO_DEVICE(v), ## __VA_ARGS__);    \
+    virtqueue_detach_element(v->vq, v->elems[idx], 0); \
+    virtio_9p_free_element(v, idx);                    \
+}
+
 static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     V9fsVirtioState *v = (V9fsVirtioState *)vdev;
@@ -52,22 +63,19 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
         if (!elem) {
             goto out_free_pdu;
         }
+        v->elems[pdu->idx] = elem;
 
         if (elem->in_num == 0) {
-            virtio_error(vdev,
-                         "The guest sent a VirtFS request without space for "
-                         "the reply");
-            goto out_free_req;
+            virtio_9p_error(v, pdu->idx, "The guest sent a VirtFS request without space for the reply");
+            goto out_free_pdu;
         }
         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)) {
-            virtio_error(vdev, "The guest sent a malformed VirtFS request: "
-                         "header size is %zd, should be 7", len);
-            goto out_free_req;
+            virtio_9p_error(v, pdu->idx, "The guest sent a malformed VirtFS request: header size is %zd, should be 7", len);
+            goto out_free_pdu;
         }
 
         pdu->size = le32_to_cpu(out.size_le);
@@ -81,9 +89,6 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
 
     return;
 
-out_free_req:
-    virtqueue_detach_element(vq, elem, 0);
-    g_free(elem);
 out_free_pdu:
     pdu_free(pdu);
 }

[Qemu-devel] [PATCH v2 4/4] 9pfs: handle broken transport

[Greg Kurz]

The 9p protocol is transport agnostic: if an error occurs when copying data
to/from the client, this should be handled by the transport layer [1] and
the 9p server should simply stop processing requests [2].

[1] can be implemented in the transport marshal/unmarshal handlers. In the
case of virtio, this means calling virtio_error() to inform the guest that
the device isn't functional anymore.

[2] means that the pdu_complete() function shouldn't send a reply back to
the client if the transport had a failure. This cannot be decided using the
current error path though, since we cannot discriminate if the error comes
from the transport or the backend. This patch hence introduces a flag in
the 9pfs state to record that the transport is broken. The device needs to
be reset for the flag to be unset.

This fixes Coverity issue CID 1348518.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - use unlikely() when checking if the transport is broken
    - fail marshal/unmarshal if transport is broken
    - v9fs_xattr_read() mark transport as broken if v9fs_pack() fails
---
 hw/9pfs/9p.c               |   45 ++++++++++++++++++++++++++++++++++++--------
 hw/9pfs/9p.h               |    1 +
 hw/9pfs/virtio-9p-device.c |   16 ++++++++++++++--
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 01deffa0c3b5..406c1937ed21 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -46,10 +46,17 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
     ssize_t ret;
     va_list ap;
 
+    if (unlikely(pdu->s->transport_broken)) {
+        return -EIO;
+    }
+
     va_start(ap, fmt);
     ret = pdu->s->transport->pdu_vmarshal(pdu, offset, fmt, ap);
     va_end(ap);
 
+    if (ret < 0) {
+        pdu->s->transport_broken = true;
+    }
     return ret;
 }
 
@@ -58,10 +65,17 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
     ssize_t ret;
     va_list ap;
 
+    if (unlikely(pdu->s->transport_broken)) {
+        return -EIO;
+    }
+
     va_start(ap, fmt);
     ret = pdu->s->transport->pdu_vunmarshal(pdu, offset, fmt, ap);
     va_end(ap);
 
+    if (ret < 0) {
+        pdu->s->transport_broken = true;
+    }
     return ret;
 }
 
@@ -624,15 +638,15 @@ 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 (unlikely(s->transport_broken)) {
+        goto out_complete;
+    }
 
     if (len < 0) {
         int err = -len;
@@ -644,11 +658,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_complete;
+            }
+            len += ret;
             id = P9_RERROR;
         }
 
-        len += pdu_marshal(pdu, len, "d", err);
+        ret = pdu_marshal(pdu, len, "d", err);
+        if (ret < 0) {
+            goto out_complete;
+        }
+        len += ret;
 
         if (s->proto_version == V9FS_PROTO_2000L) {
             id = P9_RLERROR;
@@ -657,7 +679,10 @@ 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);
+    ret = pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag);
+    if (ret < 0) {
+        goto out_complete;
+    }
 
     /* keep these in sync */
     pdu->size = len;
@@ -665,6 +690,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
 
     pdu->s->transport->push_and_notify(pdu);
 
+out_complete:
     /* Now wakeup anybody waiting in flush for this request */
     if (!qemu_co_queue_next(&pdu->complete)) {
         pdu_free(pdu);
@@ -1702,6 +1728,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
                     read_count);
     qemu_iovec_destroy(&qiov_full);
     if (err < 0) {
+        s->transport_broken = true;
         return err;
     }
     offset += err;
@@ -3596,6 +3623,8 @@ void v9fs_reset(V9fsState *s)
     while (!data.done) {
         aio_poll(qemu_get_aio_context(), true);
     }
+
+    s->transport_broken = false;
 }
 
 static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 5312d8a42405..145d0c87dd6a 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -246,6 +246,7 @@ typedef struct V9fsState
     Error *migration_blocker;
     V9fsConf fsconf;
     V9fsQID root_qid;
+    bool transport_broken;
 } V9fsState;
 
 /* 9p2000.L open flags */
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index c71659823fdc..9e61fbf7c63e 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -158,8 +158,14 @@ 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];
+    int ret;
 
-    return v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
+    ret = v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
+    if (ret < 0) {
+        virtio_9p_error(v, pdu->idx,
+                        "Failed to marshal VirtFS reply type %d", pdu->id);
+    }
+    return ret;
 }
 
 static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
@@ -168,8 +174,14 @@ 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) {
+        virtio_9p_error(v, pdu->idx,
+                        "Failed to unmarshal VirtFS request type %d", pdu->id);
+    }
+    return ret;
 }
 
 /* The size parameter is used by other transports. Do not drop it. */

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v2 0/4] 9pfs: handle transport errors
Message-id: 149328633283.30266.4224847428546759127.stgit@bahia
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5248ba7 9pfs: handle broken transport
5a209be virtio-9p: factor out virtio_9p_error_err()
7949eeb 9pfs: drop pdu_push_and_notify()
10a5f41 fsdev: don't allow unknown format in marshal/unmarshal

=== OUTPUT BEGIN ===
Checking PATCH 1/4: fsdev: don't allow unknown format in marshal/unmarshal...
Checking PATCH 2/4: 9pfs: drop pdu_push_and_notify()...
Checking PATCH 3/4: virtio-9p: factor out virtio_9p_error_err()...
ERROR: line over 90 characters
#69: FILE: hw/9pfs/virtio-9p-device.c:69:
+            virtio_9p_error(v, pdu->idx, "The guest sent a VirtFS request without space for the reply");

ERROR: line over 90 characters
#81: FILE: hw/9pfs/virtio-9p-device.c:77:
+            virtio_9p_error(v, pdu->idx, "The guest sent a malformed VirtFS request: header size is %zd, should be 7", len);

total: 2 errors, 0 warnings, 72 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: 9pfs: handle broken transport...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 1/4] fsdev: don't allow unknown format in marshal/unmarshal

[Stefano Stabellini]

On Thu, 27 Apr 2017, Greg Kurz wrote:
> The code only uses well known format strings. An unknown format token is a
> bug.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  fsdev/9p-iov-marshal.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index 1d16f8df4bd4..a1c9beddd2e7 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -168,7 +168,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
>              break;
>          }
>          default:
> -            break;
> +            g_assert_not_reached();
>          }
>          if (copied < 0) {
>              return copied;
> @@ -281,7 +281,7 @@ ssize_t v9fs_iov_vmarshal(struct iovec *in_sg, int in_num, size_t offset,
>              break;
>          }
>          default:
> -            break;
> +            g_assert_not_reached();
>          }
>          if (copied < 0) {
>              return copied;
> 
> 

Re: [Qemu-devel] [PATCH v2 2/4] 9pfs: drop pdu_push_and_notify()

[Stefano Stabellini]

On Thu, 27 Apr 2017, Greg Kurz wrote:
> Only pdu_complete() needs to notify the client that a request has completed.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p.c |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index c80ba67389ce..01deffa0c3b5 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -65,11 +65,6 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
>      return ret;
>  }
>  
> -static void pdu_push_and_notify(V9fsPDU *pdu)

I would probably turn this into a static inline and keep it around as
syntactic sugar. Regardless:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> -{
> -    pdu->s->transport->push_and_notify(pdu);
> -}
> -
>  static int omode_to_uflags(int8_t mode)
>  {
>      int ret = 0;
> @@ -668,7 +663,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
>      pdu->size = len;
>      pdu->id = id;
>  
> -    pdu_push_and_notify(pdu);
> +    pdu->s->transport->push_and_notify(pdu);
>  
>      /* Now wakeup anybody waiting in flush for this request */
>      if (!qemu_co_queue_next(&pdu->complete)) {
> 
> 

Re: [Qemu-devel] [PATCH v2 4/4] 9pfs: handle broken transport

[Stefano Stabellini]

On Thu, 27 Apr 2017, Greg Kurz wrote:
> The 9p protocol is transport agnostic: if an error occurs when copying data
> to/from the client, this should be handled by the transport layer [1] and
> the 9p server should simply stop processing requests [2].
> 
> [1] can be implemented in the transport marshal/unmarshal handlers. In the
> case of virtio, this means calling virtio_error() to inform the guest that
> the device isn't functional anymore.
> 
> [2] means that the pdu_complete() function shouldn't send a reply back to
> the client if the transport had a failure. This cannot be decided using the
> current error path though, since we cannot discriminate if the error comes
> from the transport or the backend. This patch hence introduces a flag in
> the 9pfs state to record that the transport is broken. The device needs to
> be reset for the flag to be unset.
> 
> This fixes Coverity issue CID 1348518.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - use unlikely() when checking if the transport is broken
>     - fail marshal/unmarshal if transport is broken
>     - v9fs_xattr_read() mark transport as broken if v9fs_pack() fails
> ---
>  hw/9pfs/9p.c               |   45 ++++++++++++++++++++++++++++++++++++--------
>  hw/9pfs/9p.h               |    1 +
>  hw/9pfs/virtio-9p-device.c |   16 ++++++++++++++--
>  3 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 01deffa0c3b5..406c1937ed21 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -46,10 +46,17 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
>      ssize_t ret;
>      va_list ap;
>  
> +    if (unlikely(pdu->s->transport_broken)) {
> +        return -EIO;
> +    }
> +
>      va_start(ap, fmt);
>      ret = pdu->s->transport->pdu_vmarshal(pdu, offset, fmt, ap);
>      va_end(ap);
>  
> +    if (ret < 0) {
> +        pdu->s->transport_broken = true;
> +    }
>      return ret;
>  }
>  
> @@ -58,10 +65,17 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
>      ssize_t ret;
>      va_list ap;
>  
> +    if (unlikely(pdu->s->transport_broken)) {
> +        return -EIO;
> +    }
> +
>      va_start(ap, fmt);
>      ret = pdu->s->transport->pdu_vunmarshal(pdu, offset, fmt, ap);
>      va_end(ap);
>  
> +    if (ret < 0) {
> +        pdu->s->transport_broken = true;
> +    }
>      return ret;
>  }
>  
> @@ -624,15 +638,15 @@ 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 (unlikely(s->transport_broken)) {
> +        goto out_complete;
> +    }
>  
>      if (len < 0) {
>          int err = -len;
> @@ -644,11 +658,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_complete;
> +            }
> +            len += ret;
>              id = P9_RERROR;
>          }
>  
> -        len += pdu_marshal(pdu, len, "d", err);
> +        ret = pdu_marshal(pdu, len, "d", err);
> +        if (ret < 0) {
> +            goto out_complete;
> +        }
> +        len += ret;
>  
>          if (s->proto_version == V9FS_PROTO_2000L) {
>              id = P9_RLERROR;
> @@ -657,7 +679,10 @@ 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);
> +    ret = pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag);
> +    if (ret < 0) {
> +        goto out_complete;
> +    }

If I am not mistaken, none of these "if (ret < 0)" are necessary in
pdu_complete. Just like any of the other call sites of
pdu_marshal/pdu_unmarshal, we could just let it go through the calls,
which would actually do nothing once transport_broken is set.

We could move the transport_broken check right before the call to
push_and_notify.


>      /* keep these in sync */
>      pdu->size = len;
> @@ -665,6 +690,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
>  
>      pdu->s->transport->push_and_notify(pdu);
>  
> +out_complete:
>      /* Now wakeup anybody waiting in flush for this request */
>      if (!qemu_co_queue_next(&pdu->complete)) {
>          pdu_free(pdu);
> @@ -1702,6 +1728,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
>                      read_count);
>      qemu_iovec_destroy(&qiov_full);
>      if (err < 0) {
> +        s->transport_broken = true;
>          return err;
>      }
>      offset += err;
> @@ -3596,6 +3623,8 @@ void v9fs_reset(V9fsState *s)
>      while (!data.done) {
>          aio_poll(qemu_get_aio_context(), true);
>      }
> +
> +    s->transport_broken = false;
>  }
>  
>  static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 5312d8a42405..145d0c87dd6a 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -246,6 +246,7 @@ typedef struct V9fsState
>      Error *migration_blocker;
>      V9fsConf fsconf;
>      V9fsQID root_qid;
> +    bool transport_broken;
>  } V9fsState;
>  
>  /* 9p2000.L open flags */
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index c71659823fdc..9e61fbf7c63e 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -158,8 +158,14 @@ 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];
> +    int ret;
>  
> -    return v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
> +    ret = v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
> +    if (ret < 0) {
> +        virtio_9p_error(v, pdu->idx,
> +                        "Failed to marshal VirtFS reply type %d", pdu->id);
> +    }
> +    return ret;
>  }
>  
>  static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
> @@ -168,8 +174,14 @@ 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) {
> +        virtio_9p_error(v, pdu->idx,
> +                        "Failed to unmarshal VirtFS request type %d", pdu->id);
> +    }
> +    return ret;
>  }
>  
>  /* The size parameter is used by other transports. Do not drop it. */

Re: [Qemu-devel] [PATCH v2 4/4] 9pfs: handle broken transport

[Greg Kurz]

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

On Thu, 27 Apr 2017 11:51:24 -0700 (PDT)
Stefano Stabellini <sstabellini@kernel.org> wrote:

> On Thu, 27 Apr 2017, Greg Kurz wrote:
> > The 9p protocol is transport agnostic: if an error occurs when copying data
> > to/from the client, this should be handled by the transport layer [1] and
> > the 9p server should simply stop processing requests [2].
> > 
> > [1] can be implemented in the transport marshal/unmarshal handlers. In the
> > case of virtio, this means calling virtio_error() to inform the guest that
> > the device isn't functional anymore.
> > 
> > [2] means that the pdu_complete() function shouldn't send a reply back to
> > the client if the transport had a failure. This cannot be decided using the
> > current error path though, since we cannot discriminate if the error comes
> > from the transport or the backend. This patch hence introduces a flag in
> > the 9pfs state to record that the transport is broken. The device needs to
> > be reset for the flag to be unset.
> > 
> > This fixes Coverity issue CID 1348518.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > v2: - use unlikely() when checking if the transport is broken
> >     - fail marshal/unmarshal if transport is broken
> >     - v9fs_xattr_read() mark transport as broken if v9fs_pack() fails
> > ---
> >  hw/9pfs/9p.c               |   45 ++++++++++++++++++++++++++++++++++++--------
> >  hw/9pfs/9p.h               |    1 +
> >  hw/9pfs/virtio-9p-device.c |   16 ++++++++++++++--
> >  3 files changed, 52 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 01deffa0c3b5..406c1937ed21 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -46,10 +46,17 @@ ssize_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
> >      ssize_t ret;
> >      va_list ap;
> >  
> > +    if (unlikely(pdu->s->transport_broken)) {
> > +        return -EIO;
> > +    }
> > +
> >      va_start(ap, fmt);
> >      ret = pdu->s->transport->pdu_vmarshal(pdu, offset, fmt, ap);
> >      va_end(ap);
> >  
> > +    if (ret < 0) {
> > +        pdu->s->transport_broken = true;
> > +    }
> >      return ret;
> >  }
> >  
> > @@ -58,10 +65,17 @@ ssize_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
> >      ssize_t ret;
> >      va_list ap;
> >  
> > +    if (unlikely(pdu->s->transport_broken)) {
> > +        return -EIO;
> > +    }
> > +
> >      va_start(ap, fmt);
> >      ret = pdu->s->transport->pdu_vunmarshal(pdu, offset, fmt, ap);
> >      va_end(ap);
> >  
> > +    if (ret < 0) {
> > +        pdu->s->transport_broken = true;
> > +    }
> >      return ret;
> >  }
> >  
> > @@ -624,15 +638,15 @@ 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 (unlikely(s->transport_broken)) {
> > +        goto out_complete;
> > +    }
> >  
> >      if (len < 0) {
> >          int err = -len;
> > @@ -644,11 +658,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_complete;
> > +            }
> > +            len += ret;
> >              id = P9_RERROR;
> >          }
> >  
> > -        len += pdu_marshal(pdu, len, "d", err);
> > +        ret = pdu_marshal(pdu, len, "d", err);
> > +        if (ret < 0) {
> > +            goto out_complete;
> > +        }
> > +        len += ret;
> >  
> >          if (s->proto_version == V9FS_PROTO_2000L) {
> >              id = P9_RLERROR;
> > @@ -657,7 +679,10 @@ 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);
> > +    ret = pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag);
> > +    if (ret < 0) {
> > +        goto out_complete;
> > +    }  
> 
> If I am not mistaken, none of these "if (ret < 0)" are necessary in
> pdu_complete. Just like any of the other call sites of
> pdu_marshal/pdu_unmarshal, we could just let it go through the calls,
> which would actually do nothing once transport_broken is set.
> 
> We could move the transport_broken check right before the call to
> push_and_notify.
> 

Ugh, I now realize that any request that didn't hit the first marshal/unmarshal
error and that don't go through push_and_notify will leak a ring slot... I need
to rethink this series :-\

> 
> >      /* keep these in sync */
> >      pdu->size = len;
> > @@ -665,6 +690,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
> >  
> >      pdu->s->transport->push_and_notify(pdu);
> >  
> > +out_complete:
> >      /* Now wakeup anybody waiting in flush for this request */
> >      if (!qemu_co_queue_next(&pdu->complete)) {
> >          pdu_free(pdu);
> > @@ -1702,6 +1728,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> >                      read_count);
> >      qemu_iovec_destroy(&qiov_full);
> >      if (err < 0) {
> > +        s->transport_broken = true;
> >          return err;
> >      }
> >      offset += err;
> > @@ -3596,6 +3623,8 @@ void v9fs_reset(V9fsState *s)
> >      while (!data.done) {
> >          aio_poll(qemu_get_aio_context(), true);
> >      }
> > +
> > +    s->transport_broken = false;
> >  }
> >  
> >  static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index 5312d8a42405..145d0c87dd6a 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -246,6 +246,7 @@ typedef struct V9fsState
> >      Error *migration_blocker;
> >      V9fsConf fsconf;
> >      V9fsQID root_qid;
> > +    bool transport_broken;
> >  } V9fsState;
> >  
> >  /* 9p2000.L open flags */
> > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > index c71659823fdc..9e61fbf7c63e 100644
> > --- a/hw/9pfs/virtio-9p-device.c
> > +++ b/hw/9pfs/virtio-9p-device.c
> > @@ -158,8 +158,14 @@ 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];
> > +    int ret;
> >  
> > -    return v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
> > +    ret = v9fs_iov_vmarshal(elem->in_sg, elem->in_num, offset, 1, fmt, ap);
> > +    if (ret < 0) {
> > +        virtio_9p_error(v, pdu->idx,
> > +                        "Failed to marshal VirtFS reply type %d", pdu->id);
> > +    }
> > +    return ret;
> >  }
> >  
> >  static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
> > @@ -168,8 +174,14 @@ 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) {
> > +        virtio_9p_error(v, pdu->idx,
> > +                        "Failed to unmarshal VirtFS request type %d", pdu->id);
> > +    }
> > +    return ret;
> >  }
> >  
> >  /* The size parameter is used by other transports. Do not drop it. */  


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