Re: [PATCH 2/2] 9pfs: fix init_in_iov_from_pdu truncating size

[Stefano Stabellini]

On Sun, 10 May 2020, Christian Schoenebeck wrote:
> Commit SHA-1 16724a173049ac29c7b5ade741da93a0f46edff7 introduced
> truncating the response to the currently available transport buffer
> size, which was supposed to fix an 9pfs error on Xen boot where
> transport buffer might still be smaller than required for response.
> 
> Unfortunately this change broke small reads (with less than 12
> bytes).
> 
> To address both concerns, check the actual response type and only
> truncate reply for Rreaddir responses, 

I realize you mean "Rread" (not Rreaddir). Are we sure that truncation
can only happen with Rread? I checked the spec it looks like Directories
are pretty much like files from the spec point of view. So it seems to
me that truncation might be possible there too.


> and only if truncated reply would at least return one payload byte to
> client. Use Rreaddir's precise header size (11) for this instead of
> P9_IOHDRSZ.

Ah! That's the underlying error isn't it? That P9_IOHDRSZ is not really
the size of the reply header, it is bigger. Hence the check:

  if (buf_size < P9_IOHDRSZ) {

can be wrong for very small sizes.



> Fixes: 16724a173049ac29c7b5ade741da93a0f46edff7
> Fixes: https://bugs.launchpad.net/bugs/1877688
> Signed-off-by: Christian Schoenebeck <address@hidden>
> ---
>  hw/9pfs/virtio-9p-device.c | 35 +++++++++++++++++++++++++++--------
>  hw/9pfs/xen-9p-backend.c   | 38 +++++++++++++++++++++++++++++---------
>  2 files changed, 56 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 536447a355..57e4d92ecb 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -154,15 +154,34 @@ static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, 
> struct iovec **piov,
>      VirtQueueElement *elem = v->elems[pdu->idx];
>      size_t buf_size = iov_size(elem->in_sg, elem->in_num);
>  
> -    if (buf_size < P9_IOHDRSZ) {
> -        VirtIODevice *vdev = VIRTIO_DEVICE(v);
> +    if (pdu->id + 1 == P9_RREAD) {
> +        /* size[4] Rread tag[2] count[4] data[count] */

4+2+4 = 10


> +        const size_t hdr_size = 11;

Are you adding 1 to account for "count"?


> +        /*
> +         * If current transport buffer size is smaller than actually required
> +         * for this Rreaddir response, then truncate the response to the
> +         * currently available transport buffer size, however only if it 
> would
> +         * at least allow to return 1 payload byte to client.
> +         */
> +        if (buf_size < hdr_size + 1) {

If you have already added 1 before, why do we need to add 1 again here?


> +            VirtIODevice *vdev = VIRTIO_DEVICE(v);
>  
> -        virtio_error(vdev,
> -                     "VirtFS reply type %d needs %zu bytes, buffer has %zu, 
> less than minimum",
> -                     pdu->id + 1, *size, buf_size);
> -    }
> -    if (buf_size < *size) {
> -        *size = buf_size;
> +            virtio_error(vdev,
> +                         "VirtFS reply type %d needs %zu bytes, buffer has "
> +                         "%zu, less than minimum (%zu)",
> +                         pdu->id + 1, *size, buf_size, hdr_size + 1);
> +        }

I think we want to return here


> +        if (buf_size < *size) {
> +            *size = buf_size;
> +        }
> +    } else {
> +        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;
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index f04caabfe5..98f340d24b 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -201,15 +201,35 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
>      xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, *size);
>  
>      buf_size = iov_size(ring->sg, num);
> -    if (buf_size  < P9_IOHDRSZ) {
> -        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs reply type %d needs "
> -                      "%zu bytes, buffer has %zu, less than minimum\n",
> -                      pdu->id + 1, *size, buf_size);
> -        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> -    }
> -    if (buf_size  < *size) {
> -        *size = buf_size;
> +    if (pdu->id + 1 == P9_RREAD) {
> +        /* size[4] Rread tag[2] count[4] data[count] */
> +        const size_t hdr_size = 11;
> +        /*
> +         * If current transport buffer size is smaller than actually required
> +         * for this Rreaddir response, then truncate the response to the
> +         * currently available transport buffer size, however only if it 
> would
> +         * at least allow to return 1 payload byte to client.
> +         */
> +        if (buf_size < hdr_size + 1) {
> +            xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs reply type %d "
> +                          "needs %zu bytes, buffer has %zu, less than "
> +                          "minimum (%zu)\n",
> +                          pdu->id + 1, *size, buf_size, hdr_size + 1);
> +            xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> +            xen_9pfs_disconnect(&xen_9pfs->xendev);

I htink we want to return here.


> +        }
> +        if (buf_size < *size) {
> +            *size = buf_size;
> +        }
> +    } else {
> +        if (buf_size < *size) {
> +            xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs reply type %d "
> +                         "needs %zu bytes, buffer has %zu\n", pdu->id + 1,
> +                          *size, buf_size);
> +
> +            xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> +            xen_9pfs_disconnect(&xen_9pfs->xendev);
> +        }
>      }
>  
>      *piov = ring->sg;
> -- 
> 2.20.1
>