Re: [Qemu-devel] [PATCH for-4.0?] nbd/client: Deal with unaligned size from server

[Vladimir Sementsov-Ogievskiy]

28.03.2019 5:02, Eric Blake wrote:
> When a server advertises an unaligned size but no block sizes, the
> code was rounding up to a sector-aligned size (a known limitation of
> bdrv_getlength(); maybe 4.1 will fix it to be byte-accurate), then
> assuming a request_alignment of 512 (the recommendation of the NBD
> spec for maximum portability).  However, this means that qemu will
> actually attempt to access the padding bytes of the trailing partial
> sector, without realizing that it is risky.
> 
> An easy demonstration, using nbdkit as the server:
> $ nbdkit -fv random size=1023
> $ qemu-io -r -f raw -c 'r -v 0 1023' nbd://localhost:10809
> read failed: Invalid argument
> 
> because the client rounded the request up to 1024 bytes, which nbdkit
> then rejected as beyond the advertised size of 1023.
> 
> Note that qemu as the server refuses to send an unaligned size, as it
> has already rounded the unaligned image up to sector size, and then
> happily resizes the image on access (at least when serving a POSIX
> file over NBD). But since third-party servers may decide to kill the
> connection when we access out of bounds, it's better to just ignore
> the unaligned tail than it is to risk problems. We can undo this hack
> later once the block layer quits rounding sizes inappropriately.
> 
> Reported-by: Richard W.M. Jones <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
> 
> This is the minimal patch to avoid our client behaving out-of-spec,
> but I don't like that it makes the tail of the server's file
> unreadable. A better solution of auditing the block layer to use a
> proper byte length everywhere instead of rounding up may be 4.1
> material, but is certainly not appropriate for 4.0-rc2. I could also
> teach the NBD code to compare every request from the block length
> against client.info.size, and manually handle the tail itself, but
> that feels like a lot of pain (for something the block layer should be
> able to do generically, and where any NBD-specific tail-handling code
> just slows down the common case and would be ripped out again once the
> block layer quits rounding up). I'm willing to include this with my
> other NBD patches slated for -rc2 as part of my suite of improved
> third-party interoperability, but only if we agree that the truncation
> is appropriate.
> 
> Note that nbdkit includes '--filter=truncate round-up=512' as a way to
> expose the unaligned tail without making qemu trigger out-of-bounds
> operations: reads of the tail now succeed with contents of 0; writes
> fail with ENOSPC unless the contents requested to be written into the
> tail are all 0s.  So not fixing the bug for 4.0, and telling people to
> use nbdkit's truncate filter instead, is also a viable option.
> 
>   block/nbd.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 2e72df528ac..a2cd365f646 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -463,7 +463,17 @@ static int64_t nbd_getlength(BlockDriverState *bs)
>   {
>       BDRVNBDState *s = bs->opaque;
> 
> -    return s->client.info.size;
> +    /*
> +     * HACK: Round down to sector alignment. qemu as server always
> +     * advertises a sector-aligned image, so we only ever see
> +     * unaligned sizes with third-party servers. The block layer
> +     * defaults to rounding up, but that can result in us attempting
> +     * to read beyond EOF from the remote server; better is to
> +     * forcefully round down here and just treat the last few bytes
> +     * from the server as unreadable.  This can all go away once the
> +     * block layer uses actual byte lengths instead of rounding up.
> +     */
> +    return QEMU_ALIGN_DOWN(s->client.info.size, BDRV_SECTOR_SIZE);
>   }
> 
>   static void nbd_detach_aio_context(BlockDriverState *bs)
> 


What is wrong with correct EIO from server? I'm not sure that noisily ignoring
file tail is better.

Assume qemu-img convert from such nbd export: with your patch it will just 
ignore
last bytes, and user will think that all is OK. I'd prefer not doing it.


-- 
Best regards,
Vladimir