[PATCH 0/1] virtio-9pfs: don't truncate response

[PATCH 0/1] virtio-9pfs: don't truncate response

[Christian Schoenebeck]

The following patch reverts
SHA-1 16724a173049ac29c7b5ade741da93a0f46edff for the virtio backend.

Greg, it is intended as a quick fix for
https://bugs.launchpad.net/bugs/1877688 at least for virtio, for the
case the appropriate fix on Xen side might still take a while. Because
this bug is too serious to let it rest for too long.

In case Stefano comes up with a fix for Xen soon, you might just ignore
this patch and just revert SHA-1 16724a173049ac29c7b5ade741da93a0f46edff
entirely instead of course.

Christian Schoenebeck (1):
  virtio-9pfs: don't truncate response

 hw/9pfs/virtio-9p-device.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH 1/1] virtio-9pfs: don't truncate response

[Christian Schoenebeck]

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 fix this introduced bug for virtio at least, let's revert this change
for the virtio transport. Unlike with Xen, we should never come into
this situation with virtio that the available transport buffer would be
too small for delivering any response to client. So truncating the buffer
is not necessary with virtio in the first place.

This bug still needs to be addressed for Xen appropriately though.

Fixes: 16724a173049ac29c7b5ade741da93a0f46edff7 (for virtio only)
Fixes: https://bugs.launchpad.net/bugs/1877688 (for virtio only)
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/virtio-9p-device.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 536447a355..bb6154945a 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -154,16 +154,13 @@ 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) {
+    if (buf_size < *size) {
         VirtIODevice *vdev = VIRTIO_DEVICE(v);
 
         virtio_error(vdev,
-                     "VirtFS reply type %d needs %zu bytes, buffer has %zu, less than minimum",
+                     "VirtFS reply type %d needs %zu bytes, buffer has %zu",
                      pdu->id + 1, *size, buf_size);
     }
-    if (buf_size < *size) {
-        *size = buf_size;
-    }
 
     *piov = elem->in_sg;
     *pniov = elem->in_num;
-- 
2.20.1

Re: [PATCH 1/1] virtio-9pfs: don't truncate response

[Stefano Stabellini]

On Thu, 14 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 fix this introduced bug for virtio at least, let's revert this change
> for the virtio transport. Unlike with Xen, we should never come into
> this situation with virtio that the available transport buffer would be
> too small for delivering any response to client. So truncating the buffer
> is not necessary with virtio in the first place.
> 
> This bug still needs to be addressed for Xen appropriately though.
> 
> Fixes: 16724a173049ac29c7b5ade741da93a0f46edff7 (for virtio only)
> Fixes: https://bugs.launchpad.net/bugs/1877688 (for virtio only)
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  hw/9pfs/virtio-9p-device.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 536447a355..bb6154945a 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -154,16 +154,13 @@ 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) {
> +    if (buf_size < *size) {
>          VirtIODevice *vdev = VIRTIO_DEVICE(v);
>  
>          virtio_error(vdev,
> -                     "VirtFS reply type %d needs %zu bytes, buffer has %zu, less than minimum",
> +                     "VirtFS reply type %d needs %zu bytes, buffer has %zu",
>                       pdu->id + 1, *size, buf_size);
>      }
> -    if (buf_size < *size) {
> -        *size = buf_size;
> -    }
>  
>      *piov = elem->in_sg;
>      *pniov = elem->in_num;
> -- 
> 2.20.1
> 

Re: [PATCH 0/1] virtio-9pfs: don't truncate response

[Stefano Stabellini]

On Thu, 14 May 2020, Christian Schoenebeck wrote:
> The following patch reverts
> SHA-1 16724a173049ac29c7b5ade741da93a0f46edff for the virtio backend.
> 
> Greg, it is intended as a quick fix for
> https://bugs.launchpad.net/bugs/1877688 at least for virtio, for the
> case the appropriate fix on Xen side might still take a while. Because
> this bug is too serious to let it rest for too long.
> 
> In case Stefano comes up with a fix for Xen soon, you might just ignore
> this patch and just revert SHA-1 16724a173049ac29c7b5ade741da93a0f46edff
> entirely instead of course.

I am aiming at getting this fixed within a few days.  Up to you as you
want to proceed with the patches. I am happy either way.

Re: [PATCH 0/1] virtio-9pfs: don't truncate response

[Greg Kurz]

On Thu, 14 May 2020 20:38:02 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> The following patch reverts
> SHA-1 16724a173049ac29c7b5ade741da93a0f46edff for the virtio backend.
> 
> Greg, it is intended as a quick fix for
> https://bugs.launchpad.net/bugs/1877688 at least for virtio, for the
> case the appropriate fix on Xen side might still take a while. Because
> this bug is too serious to let it rest for too long.
> 
> In case Stefano comes up with a fix for Xen soon, you might just ignore
> this patch and just revert SHA-1 16724a173049ac29c7b5ade741da93a0f46edff
> entirely instead of course.
> 

Stefano said he could come up with a fix in a few days, so I'll just
wait for now. If nothing happens in, say, two weeks, please ping me
and I'll merge this patch.

> Christian Schoenebeck (1):
>   virtio-9pfs: don't truncate response
> 
>  hw/9pfs/virtio-9p-device.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>