Re: [PATCH v4 16/24] nbd/server: Support 64-bit block status

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

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@yandex-team.ru>

-- 
Best regards,
Vladimir