[Libguestfs] [libnbd PATCH 3/8] pread: Reject server SR read response with no data chunks

[Richard W.M. Jones]

On Mon, Jun 17, 2019 at 07:07:53PM -0500, Eric Blake wrote:
> The NBD spec requires that a server doing structured reads must not
> succeed unless it covers the entire buffer with reply chunks. In the
> general case, this requires a lot of bookkeeping to check whether
> offsets were non-overlapping and sufficient, and we'd rather defer
> such checking to an optional callback function. But a callback
> function will only be reached per chunk, while we still want to fail
> the overall read if the callback function was never called because the
> server erroneously replied with NBD_REPLY_TYPE_NONE with no other
> chunks instead of an expected NBD_REPLY_TYPE_ERROR*. For this specific
> error case, the bookkeeping is much simpler - we merely track if we've
> seen at least one data chunk.

I guess the implicit assumption here is that count > 0, which IIRC the
NBD protocol demands.  It seems like we don't actually check this in
nbd_internal_command_common so would it be worth adding a check there?

In any case this patch on its own looks good, so ACK.

Rich.

>  generator/states-reply-simple.c     | 1 +
>  generator/states-reply-structured.c | 2 ++
>  lib/aio.c                           | 2 ++
>  lib/internal.h                      | 1 +
>  4 files changed, 6 insertions(+)
> 
> diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
> index 12536e0..935f6d2 100644
> --- a/generator/states-reply-simple.c
> +++ b/generator/states-reply-simple.c
> @@ -40,6 +40,7 @@
>    if (cmd->error == 0 && cmd->type == NBD_CMD_READ) {
>      h->rbuf = cmd->data;
>      h->rlen = cmd->count;
> +    cmd->data_seen = true;
>      SET_NEXT_STATE (%RECV_READ_PAYLOAD);
>    }
>    else {
> diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
> index 9bb165b..6740400 100644
> --- a/generator/states-reply-structured.c
> +++ b/generator/states-reply-structured.c
> @@ -269,6 +269,7 @@
>        return -1;
>      }
>      assert (cmd->data);
> +    cmd->data_seen = true;
> 
>      /* Length of the data following. */
>      length -= 8;
> @@ -331,6 +332,7 @@
>        return -1;
>      }
>      assert (cmd->data);
> +    cmd->data_seen = true;
> 
>      /* Is the data within bounds? */
>      if (offset < cmd->offset) {
> diff --git a/lib/aio.c b/lib/aio.c
> index 52e8892..7fb0fdf 100644
> --- a/lib/aio.c
> +++ b/lib/aio.c
> @@ -73,6 +73,8 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
> 
>    type = cmd->type;
>    error = cmd->error;
> +  if (type == NBD_CMD_READ && !cmd->data_seen && !error)
> +    error = EIO;
> 
>    /* Retire it from the list and free it. */
>    if (prev_cmd != NULL)
> diff --git a/lib/internal.h b/lib/internal.h
> index 6fde06c..1f8f789 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -242,6 +242,7 @@ struct command_in_flight {
>    uint32_t count;
>    void *data; /* Buffer for read/write, opaque for block status */
>    extent_fn extent_fn;
> +  bool data_seen; /* For read, true if at least one data chunk seen */
>    uint32_t error; /* Local errno value */
>  };
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v