Vladimir Sementsov-Ogievskiy
2023-Jun-27 13:23 UTC
[Libguestfs] [PATCH v4 16/24] nbd/server: Support 64-bit block status
On 08.06.23 16:56, Eric Blake wrote:> The NBD spec states that if the client negotiates extended headers, > the server must avoid NBD_REPLY_TYPE_BLOCK_STATUS and instead use > NBD_REPLY_TYPE_BLOCK_STATUS_EXT which supports 64-bit lengths, even if > the reply does not need more than 32 bits. As of this patch, > client->mode is still never NBD_MODE_EXTENDED, so the code added here > does not take effect until the next patch enables negotiation. > > For now, all metacontexts that we know how to export never populate > more than 32 bits of information, so we don't have to worry about > NBD_REP_ERR_EXT_HEADER_REQD or filtering during handshake, and we > always send all zeroes for the upper 32 bits of status during > NBD_CMD_BLOCK_STATUS. > > Note that we previously had some interesting size-juggling on call > chains, such as: > > nbd_co_send_block_status(uint32_t length) > -> blockstatus_to_extents(uint32_t bytes) > -> bdrv_block_status_above(bytes, &uint64_t num) > -> nbd_extent_array_add(uint64_t num) > -> store num in 32-bit length > > But we were lucky that it never overflowed: bdrv_block_status_above > never sets num larger than bytes, and we had previously been capping > 'bytes' at 32 bits (since the protocol does not allow sending a larger > request without extended headers). This patch adds some assertions > that ensure we continue to avoid overflowing 32 bits for a narrow[..]> @@ -2162,19 +2187,23 @@ static void nbd_extent_array_convert_to_be(NBDExtentArray *ea) > * would result in an incorrect range reported to the client) > */ > static int nbd_extent_array_add(NBDExtentArray *ea, > - uint32_t length, uint32_t flags) > + uint64_t length, uint32_t flags) > { > assert(ea->can_add); > > if (!length) { > return 0; > } > + if (!ea->extended) { > + assert(length <= UINT32_MAX); > + } > > /* Extend previous extent if flags are the same */ > if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) { > - uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length; > + uint64_t sum = length + ea->extents[ea->count - 1].length; > > - if (sum <= UINT32_MAX) { > + assert(sum >= length); > + if (sum <= UINT32_MAX || ea->extended) {that "if" and uint64_t sum was to avoid overflow. I think, we can't just assert, instead include the check into if: if (sum >= length && (sum <= UINT32_MAX || ea->extended) { ... } with this: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov at yandex-team.ru> -- Best regards, Vladimir
Eric Blake
2023-Aug-04 19:36 UTC
[Libguestfs] [PATCH v4 16/24] nbd/server: Support 64-bit block status
On Tue, Jun 27, 2023 at 04:23:49PM +0300, Vladimir Sementsov-Ogievskiy wrote:> On 08.06.23 16:56, Eric Blake wrote: > > The NBD spec states that if the client negotiates extended headers, > > the server must avoid NBD_REPLY_TYPE_BLOCK_STATUS and instead use > > NBD_REPLY_TYPE_BLOCK_STATUS_EXT which supports 64-bit lengths, even if > > the reply does not need more than 32 bits. As of this patch, > > client->mode is still never NBD_MODE_EXTENDED, so the code added here > > does not take effect until the next patch enables negotiation. > > > > For now, all metacontexts that we know how to export never populate > > more than 32 bits of information, so we don't have to worry about > > NBD_REP_ERR_EXT_HEADER_REQD or filtering during handshake, and we > > always send all zeroes for the upper 32 bits of status during > > NBD_CMD_BLOCK_STATUS. > > > > Note that we previously had some interesting size-juggling on call > > chains, such as: > > > > nbd_co_send_block_status(uint32_t length) > > -> blockstatus_to_extents(uint32_t bytes) > > -> bdrv_block_status_above(bytes, &uint64_t num) > > -> nbd_extent_array_add(uint64_t num) > > -> store num in 32-bit length > > > > But we were lucky that it never overflowed: bdrv_block_status_above > > never sets num larger than bytes, and we had previously been capping > > 'bytes' at 32 bits (since the protocol does not allow sending a larger > > request without extended headers). This patch adds some assertions > > that ensure we continue to avoid overflowing 32 bits for a narrow > > > [..] > > > @@ -2162,19 +2187,23 @@ static void nbd_extent_array_convert_to_be(NBDExtentArray *ea) > > * would result in an incorrect range reported to the client) > > */ > > static int nbd_extent_array_add(NBDExtentArray *ea, > > - uint32_t length, uint32_t flags) > > + uint64_t length, uint32_t flags) > > { > > assert(ea->can_add); > > > > if (!length) { > > return 0; > > } > > + if (!ea->extended) { > > + assert(length <= UINT32_MAX); > > + } > > > > /* Extend previous extent if flags are the same */ > > if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) { > > - uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length; > > + uint64_t sum = length + ea->extents[ea->count - 1].length; > > > > - if (sum <= UINT32_MAX) { > > + assert(sum >= length); > > + if (sum <= UINT32_MAX || ea->extended) { > > that "if" and uint64_t sum was to avoid overflow. I think, we can't just assert, instead include the check into if: > > if (sum >= length && (sum <= UINT32_MAX || ea->extended) {Why? The assertion is stating that there was no overflow, because we are in control of ea->extents[ea->count - 1].length (it came from local code performing block status, and our block layer guarantees that no block status returns more than 2^63 bytes because we don't support images larger than off_t). At best, all I need is a comment why the assertion is valid.> > ... > > } > > with this: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov at yandex-team.ru> > > -- > Best regards, > Vladimir >-- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org