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 <qemu_...@crudebyte.com>
> ---
> 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
>
