qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v4 16/24] nbd/server: Support 64-bit block status
Date: Tue, 27 Jun 2023 16:23:49 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]