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 <gr...@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. */