From d7a0e8fc0484ef18c25e7396baea07dfd6520a30 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 22 Mar 2019 03:22:20 +0100 Subject: [PATCH 053/163] nbd/server: fix bitmap export RH-Author: John Snow Message-id: <20190322032241.8111-8-jsnow@redhat.com> Patchwork-id: 85094 O-Subject: [RHEL-7.7 qemu-kvm-rhev PATCH 07/28] nbd/server: fix bitmap export Bugzilla: 1691563 RH-Acked-by: Max Reitz RH-Acked-by: Stefan Hajnoczi RH-Acked-by: Miroslav Rezanina From: Vladimir Sementsov-Ogievskiy bitmap_to_extents function is broken: it switches dirty variable after every iteration, however it can process only part of dirty (or zero) area during one iteration in case when this area is too large for one extent. Fortunately, the bug doesn't produce wrong extent flags: it just inserts a zero-length extent between sequential extents representing large dirty (or zero) area. However, zero-length extents are forbidden by the NBD protocol. So, a careful client should consider such a reply as a server fault, while a less-careful will likely ignore zero-length extents. The bug can only be triggered by a client that requests block status for nearly 4G at once (a request of 4G and larger is impossible per the protocol, and requests smaller than 4G less the bitmap granularity cause the loop to quit iterating rather than revisit the tail of the large area); it also cannot trigger if the client used the NBD_CMD_FLAG_REQ_ONE flag. Since qemu 3.0 as client (using the x-dirty-bitmap extension) always passes the flag, it is immune; and we are not aware of other open-source clients that know how to request qemu:dirty-bitmap:FOO contexts. Clients that want to avoid the bug could cap block status requests to a smaller length, such as 2G or 3G. Fix this by more careful handling of dirty variable. Bug was introduced in 3d068aff16 "nbd/server: implement dirty bitmap export", with the whole function. and is present in v3.0.0 release. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20180914165116.23182-1-vsementsov@virtuozzo.com> CC: qemu-stable@nongnu.org Reviewed-by: Eric Blake [eblake: improved commit message] Signed-off-by: Eric Blake (cherry picked from commit 6545916d528de7a6b784f4d10e7b236b30bfaced) Signed-off-by: John Snow Signed-off-by: Miroslav Rezanina --- nbd/server.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nbd/server.c b/nbd/server.c index 0ab0dbd..a8ddc4a 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1951,6 +1951,8 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset, assert(begin < overall_end && nb_extents); while (begin < overall_end && i < nb_extents) { + bool next_dirty = !dirty; + if (dirty) { end = bdrv_dirty_bitmap_next_zero(bitmap, begin, UINT64_MAX); } else { @@ -1962,6 +1964,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset, end = MIN(bdrv_dirty_bitmap_size(bitmap), begin + UINT32_MAX + 1 - bdrv_dirty_bitmap_granularity(bitmap)); + next_dirty = dirty; } if (dont_fragment && end > overall_end) { end = overall_end; @@ -1971,7 +1974,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset, extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0); i++; begin = end; - dirty = !dirty; + dirty = next_dirty; } bdrv_dirty_iter_free(it); -- 1.8.3.1