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 <[email protected]> > Signed-off-by: Eric Blake <[email protected]> > --- > > 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
