1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v2 0/5] more blkdebug tweaks
  4. View patch

[Qemu-devel] [PATCH v2 2/5] block: Guarantee that *file is set on bdrv_get_block_status()

Eric Blake posted 5 patches 8 years, 8 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 2/5] block: Guarantee that *file is set on bdrv_get_block_status()
Posted by Eric Blake 8 years, 8 months ago 
We document that *file is valid if the return is not an error and
includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
when a driver (such as blkdebug) lacks a callback.  Broken in
commit 67a0fd2 (v2.6), when we added the file parameter.

Enhance qemu-iotest 177 to cover this, using a sequence that would
print garbage or even SEGV, because it was dererefencing through
uninitialized memory.  [The resulting test output shows that we
have less-than-ideal block status from the blkdebug driver, but
that's a separate fix coming up soon.]

Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
enough to fix the crash, but we can go one step further: always
setting *file, even on error, means that a broken caller that
blindly dereferences file without checking for error is now more
likely to get a reliable SEGV instead of randomly acting on garbage,
making it easier to diagnose such buggy callers.  Adding an
assertion that file is set where expected doesn't hurt either.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>

---
v2: drop redundant assignment
---
 block/io.c                 | 5 +++--
 tests/qemu-iotests/177     | 3 +++
 tests/qemu-iotests/177.out | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index fdd7485..8e6c3fe 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1749,6 +1749,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     int64_t n;
     int64_t ret, ret2;

+    *file = NULL;
     total_sectors = bdrv_nb_sectors(bs);
     if (total_sectors < 0) {
         return total_sectors;
@@ -1769,11 +1770,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
         if (bs->drv->protocol_name) {
             ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+            *file = bs;
         }
         return ret;
     }

-    *file = NULL;
     bdrv_inc_in_flight(bs);
     ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
                                             file);
@@ -1783,7 +1784,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     }

     if (ret & BDRV_BLOCK_RAW) {
-        assert(ret & BDRV_BLOCK_OFFSET_VALID);
+        assert(ret & BDRV_BLOCK_OFFSET_VALID && *file);
         ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
                                        *pnum, pnum, file);
         goto out;
diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177
index 2005c17..f8ed8fb 100755
--- a/tests/qemu-iotests/177
+++ b/tests/qemu-iotests/177
@@ -43,6 +43,7 @@ _supported_proto file
 CLUSTER_SIZE=1M
 size=128M
 options=driver=blkdebug,image.driver=qcow2
+nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG

 echo
 echo "== setting up files =="
@@ -106,6 +107,8 @@ function verify_io()
 }

 verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \
+    | _filter_qemu_img_map

 _check_test_img

diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index e887542..b754ed4 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352
 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 23068672/23068672 bytes at offset 111149056
 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          File
+0               0x8000000       blkdebug::TEST_DIR/t.IMGFMT
 No errors were found on the image.
 *** done
-- 
2.9.4
Re: [Qemu-devel] [PATCH v2 2/5] block: Guarantee that *file is set on bdrv_get_block_status()
Posted by Fam Zheng 8 years, 8 months ago 
On Wed, 05/24 15:28, Eric Blake wrote:
> We document that *file is valid if the return is not an error and
> includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
> when a driver (such as blkdebug) lacks a callback.  Broken in
> commit 67a0fd2 (v2.6), when we added the file parameter.
> 
> Enhance qemu-iotest 177 to cover this, using a sequence that would
> print garbage or even SEGV, because it was dererefencing through
> uninitialized memory.  [The resulting test output shows that we
> have less-than-ideal block status from the blkdebug driver, but
> that's a separate fix coming up soon.]
> 
> Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
> enough to fix the crash, but we can go one step further: always
> setting *file, even on error, means that a broken caller that
> blindly dereferences file without checking for error is now more
> likely to get a reliable SEGV instead of randomly acting on garbage,
> making it easier to diagnose such buggy callers.  Adding an
> assertion that file is set where expected doesn't hurt either.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: drop redundant assignment
> ---
>  block/io.c                 | 5 +++--
>  tests/qemu-iotests/177     | 3 +++
>  tests/qemu-iotests/177.out | 2 ++
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index fdd7485..8e6c3fe 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1749,6 +1749,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>      int64_t n;
>      int64_t ret, ret2;
> 
> +    *file = NULL;
>      total_sectors = bdrv_nb_sectors(bs);
>      if (total_sectors < 0) {
>          return total_sectors;
> @@ -1769,11 +1770,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
>          if (bs->drv->protocol_name) {
>              ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
> +            *file = bs;
>          }
>          return ret;
>      }
> 
> -    *file = NULL;
>      bdrv_inc_in_flight(bs);
>      ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
>                                              file);
> @@ -1783,7 +1784,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>      }
> 
>      if (ret & BDRV_BLOCK_RAW) {
> -        assert(ret & BDRV_BLOCK_OFFSET_VALID);
> +        assert(ret & BDRV_BLOCK_OFFSET_VALID && *file);
>          ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
>                                         *pnum, pnum, file);
>          goto out;
> diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177
> index 2005c17..f8ed8fb 100755
> --- a/tests/qemu-iotests/177
> +++ b/tests/qemu-iotests/177
> @@ -43,6 +43,7 @@ _supported_proto file
>  CLUSTER_SIZE=1M
>  size=128M
>  options=driver=blkdebug,image.driver=qcow2
> +nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG
> 
>  echo
>  echo "== setting up files =="
> @@ -106,6 +107,8 @@ function verify_io()
>  }
> 
>  verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \
> +    | _filter_qemu_img_map
> 
>  _check_test_img
> 
> diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
> index e887542..b754ed4 100644
> --- a/tests/qemu-iotests/177.out
> +++ b/tests/qemu-iotests/177.out
> @@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352
>  29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 23068672/23068672 bytes at offset 111149056
>  22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Offset          Length          File
> +0               0x8000000       blkdebug::TEST_DIR/t.IMGFMT
>  No errors were found on the image.
>  *** done
> -- 
> 2.9.4
> 

Reviewed-by: Fam Zheng <famz@redhat.com>
Re: [Qemu-devel] [PATCH v2 2/5] block: Guarantee that *file is set on bdrv_get_block_status()
Posted by Max Reitz 8 years, 8 months ago 
On 2017-05-24 22:28, Eric Blake wrote:
> We document that *file is valid if the return is not an error and
> includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
> when a driver (such as blkdebug) lacks a callback.  Broken in
> commit 67a0fd2 (v2.6), when we added the file parameter.

Not sure if I'd call that "broken", as in the part participle of "break"
because there was never any breaking. It just didn't abide by the
contract from the start.

> Enhance qemu-iotest 177 to cover this, using a sequence that would
> print garbage or even SEGV, because it was dererefencing through
> uninitialized memory.  [The resulting test output shows that we
> have less-than-ideal block status from the blkdebug driver, but
> that's a separate fix coming up soon.]
> 
> Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
> enough to fix the crash, but we can go one step further: always
> setting *file, even on error, means that a broken caller that
> blindly dereferences file without checking for error is now more
> likely to get a reliable SEGV instead of randomly acting on garbage,
> making it easier to diagnose such buggy callers.  Adding an
> assertion that file is set where expected doesn't hurt either.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: drop redundant assignment
> ---
>  block/io.c                 | 5 +++--
>  tests/qemu-iotests/177     | 3 +++
>  tests/qemu-iotests/177.out | 2 ++
>  3 files changed, 8 insertions(+), 2 deletions(-)

Anyway,

Reviewed-by: Max Reitz <mreitz@redhat.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.