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 <vsement...@yandex-team.ru>
>
> --
> Best regards,
> Vladimir
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
