[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are m
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured |
Date: |
Tue, 27 Jun 2017 08:54:18 +0200 |
On Mon, 26 Jun 2017 16:22:23 -0700 (PDT)
Stefano Stabellini <address@hidden> 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 <address@hidden>
> > ---
> > 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;
> }
pgp7C4LyRNgKk.pgp
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v4 0/4] 9pfs: handle transport errors, Greg Kurz, 2017/06/23
- [Qemu-devel] [PATCH v4 1/4] virtio-9p: record element after sanity checks, Greg Kurz, 2017/06/23
- [Qemu-devel] [PATCH v4 2/4] virtio-9p: message header is 7-byte long, Greg Kurz, 2017/06/23
- [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured, Greg Kurz, 2017/06/23
- Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured, Stefano Stabellini, 2017/06/26
- Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured,
Greg Kurz <=
- Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured, Stefano Stabellini, 2017/06/27
- Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured, Greg Kurz, 2017/06/28
- Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured, Stefano Stabellini, 2017/06/28
- Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured, Greg Kurz, 2017/06/28
- Re: [Qemu-devel] [PATCH v4 3/4] virtio-9p: break device if buffers are misconfigured, Stefano Stabellini, 2017/06/28
[Qemu-devel] [PATCH v4 4/4] 9pfs: handle transport errors in pdu_complete(), Greg Kurz, 2017/06/23