| Message ID | 149328636874.30266.8813988060953128881.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
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. */
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. */
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. */
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(-)