| Message ID | 20180802144834.520904-5-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD fixes for unaligned images | expand |
02.08.2018 17:48, 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()), 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. > > 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). > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 08/17/2018 08:57 AM, Vladimir Sementsov-Ogievskiy wrote: > 02.08.2018 17:48, 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()), 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. >> >> 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). >> >> Reported-by: Richard W.M. Jones <rjones@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > This patch is not a full solution. It fixes things so that a client accessing the first half of the final sector no longer rounds things up and chokes the server, but does not prevent a client from attempting to access the second half of the final sector (where that access still reaches the server). I probably need yet another patch, similar to Rich's 'nbdkit --filter truncate', where reads of the trailing hole created by qemu's rounding are padded to NUL without asking the server, and where writes are ignored if all zero or cause ENOSPACE if nonzero. Or, a much bigger patch series to make qemu quit rounding size up :) (I'd like to get there someday, but it's faster to kick out the quick patch for just NBD than to audit the entire block stack)
diff --git a/block/nbd.c b/block/nbd.c index e87699fb73b..a3e6889c57f 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -473,7 +473,16 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp) uint32_t min = s->info.min_block; uint32_t max = MIN_NON_ZERO(NBD_MAX_BUFFER_SIZE, s->info.max_block); - bs->bl.request_alignment = min ? min : BDRV_SECTOR_SIZE; + /* + * If the server did not advertise an alignment, then pick the + * largest power of 2 that evenly divides the advertised size, but + * does not exceed a sector. + */ + if (!min) { + min = 1 << ctz32(BDRV_SECTOR_SIZE | s->info.size); + } + + bs->bl.request_alignment = min; bs->bl.max_pdiscard = max; bs->bl.max_pwrite_zeroes = max; bs->bl.max_transfer = max;
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()), 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. 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). Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- block/nbd.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)