From: Eric Blake <eblake@redhat.com>
To: qemu-devel@nongnu.org
Cc: qemu-block@nongnu.org, jsnow@redhat.com, mreitz@redhat.com,
qemu-stable@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>,
Fam Zheng <famz@redhat.com>, Kevin Wolf <kwolf@redhat.com>
Subject: [Qemu-devel] [PATCH 2/4] block: Guarantee that *file is set on bdrv_get_block_status()
Date: Thu, 18 May 2017 21:32:31 -0500 [thread overview]
Message-ID: <20170519023233.24461-3-eblake@redhat.com> (raw)
In-Reply-To: <20170519023233.24461-1-eblake@redhat.com>
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 only when setting 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 caller is no longer dereferencing
uninitialized memory, so that we are more likely to get a reliable
SEGV instead of randomly acting on garbage. Adding an assertion
doesn't hurt either.
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
block/io.c | 4 +++-
tests/qemu-iotests/177 | 3 +++
tests/qemu-iotests/177.out | 2 ++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/block/io.c b/block/io.c
index fdd7485..164a82b 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,6 +1770,7 @@ 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;
}
@@ -1783,7 +1785,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
next prev parent reply other threads:[~2017-05-19 2:32 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-19 2:32 [Qemu-devel] [PATCH 0/4] more blkdebug tweaks Eric Blake
2017-05-19 2:32 ` [Qemu-devel] [PATCH 1/4] qemu-io: Don't die on second open Eric Blake
2017-05-24 6:28 ` Fam Zheng
2017-05-24 13:43 ` Eric Blake
2017-05-19 2:32 ` Eric Blake [this message]
2017-05-24 15:03 ` [Qemu-devel] [PATCH 2/4] block: Guarantee that *file is set on bdrv_get_block_status() Eric Blake
2017-05-19 2:32 ` [Qemu-devel] [PATCH 3/4] block: Simplify use of BDRV_BLOCK_RAW Eric Blake
2017-05-22 20:23 ` Eric Blake
2017-05-19 2:32 ` [Qemu-devel] [PATCH 4/4] blkdebug: Support .bdrv_co_get_block_status Eric Blake
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170519023233.24461-3-eblake@redhat.com \
--to=eblake@redhat.com \
--cc=famz@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-stable@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.