[Qemu-devel] [PATCH v7 0/3] block: Fix unaligned bdrv_aio_write_zeroes

[Qemu-devel] [PATCH v7 0/3] block: Fix unaligned bdrv_aio_write_zeroes

[Fam Zheng]

v7: Add Kevin's rev-by in patch 1 and 3.
    Address Stefan's and Kevin's comments on patch 2:
    - Don't duplicate tracked_request_begin and tracked_request_end;
    - Don't forget to remove debug printf;
    - Call qemu_vfree unconditionally;
    - Don't serialize aligned part of the zero write req;

An unaligned zero write causes NULL deferencing in bdrv_co_do_pwritev. That
path is reachable from bdrv_co_write_zeroes and bdrv_aio_write_zeroes.

You can easily trigger through the former with qemu-io, as the test case added
by 61815d6e0aa. For bdrv_aio_write_zeroes, in common cases there's always a
format driver (which uses 512 alignment), so it would be much rarer to have
unaligned requests (only concerning top level here, when the request goes down
to bs->file, where for example the alignment is 4k, it would then be calling
bdrv_co_write_zeroes because it's in a coroutine).

fc3959e4669a1c fixed bdrv_co_write_zeroes but not bdrv_aio_write_zeroes.  The
lattern is the actually used one by device model. Revert the previous fix, do
it in bdrv_co_do_pwritev, to cover both paths.



Fam Zheng (3):
  Revert "block: Fix unaligned zero write"
  block: Fix NULL deference for unaligned write if qiov is NULL
  qemu-iotests: Test unaligned sub-block zero write

 block/io.c                 | 142 ++++++++++++++++++++++++++++++++-------------
 tests/qemu-iotests/033     |  13 +++++
 tests/qemu-iotests/033.out |  30 ++++++++++
 3 files changed, 144 insertions(+), 41 deletions(-)

-- 
2.4.0

[Qemu-devel] [PATCH v7 1/3] Revert "block: Fix unaligned zero write"

[Fam Zheng]

This reverts commit fc3959e4669a1c2149b91ccb05101cfc7ae1fc05.

The core write code already handles the case, so remove this
duplication.

Because commit 61007b316 moved the touched code from block.c to
block/io.c, the change is manually reverted.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 45 ++++++---------------------------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/block/io.c b/block/io.c
index 1ce62c4..4e5a92e 100644
--- a/block/io.c
+++ b/block/io.c
@@ -929,19 +929,6 @@ out:
     return ret;
 }
 
-static inline uint64_t bdrv_get_align(BlockDriverState *bs)
-{
-    /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
-    return MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
-}
-
-static inline bool bdrv_req_is_aligned(BlockDriverState *bs,
-                                       int64_t offset, size_t bytes)
-{
-    int64_t align = bdrv_get_align(bs);
-    return !(offset & (align - 1) || (bytes & (align - 1)));
-}
-
 /*
  * Handle a read request in coroutine context
  */
@@ -952,7 +939,8 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
     BlockDriver *drv = bs->drv;
     BdrvTrackedRequest req;
 
-    uint64_t align = bdrv_get_align(bs);
+    /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
     uint8_t *head_buf = NULL;
     uint8_t *tail_buf = NULL;
     QEMUIOVector local_qiov;
@@ -1194,7 +1182,8 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
     BdrvRequestFlags flags)
 {
     BdrvTrackedRequest req;
-    uint64_t align = bdrv_get_align(bs);
+    /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
     uint8_t *head_buf = NULL;
     uint8_t *tail_buf = NULL;
     QEMUIOVector local_qiov;
@@ -1293,10 +1282,6 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
         bytes = ROUND_UP(bytes, align);
     }
 
-    if (use_local_qiov) {
-        /* Local buffer may have non-zero data. */
-        flags &= ~BDRV_REQ_ZERO_WRITE;
-    }
     ret = bdrv_aligned_pwritev(bs, &req, offset, bytes,
                                use_local_qiov ? &local_qiov : qiov,
                                flags);
@@ -1337,32 +1322,14 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
                                       int64_t sector_num, int nb_sectors,
                                       BdrvRequestFlags flags)
 {
-    int ret;
-
     trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
 
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
         flags &= ~BDRV_REQ_MAY_UNMAP;
     }
-    if (bdrv_req_is_aligned(bs, sector_num << BDRV_SECTOR_BITS,
-                            nb_sectors << BDRV_SECTOR_BITS)) {
-        ret = bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
-                                BDRV_REQ_ZERO_WRITE | flags);
-    } else {
-        uint8_t *buf;
-        QEMUIOVector local_qiov;
-        size_t bytes = nb_sectors << BDRV_SECTOR_BITS;
 
-        buf = qemu_memalign(bdrv_opt_mem_align(bs), bytes);
-        memset(buf, 0, bytes);
-        qemu_iovec_init(&local_qiov, 1);
-        qemu_iovec_add(&local_qiov, buf, bytes);
-
-        ret = bdrv_co_do_writev(bs, sector_num, nb_sectors, &local_qiov,
-                                BDRV_REQ_ZERO_WRITE | flags);
-        qemu_vfree(buf);
-    }
-    return ret;
+    return bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
+                             BDRV_REQ_ZERO_WRITE | flags);
 }
 
 int bdrv_flush_all(void)
-- 
2.4.0

[Qemu-devel] [PATCH v7 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Fam Zheng]

For zero write, callers pass in NULL qiov (qemu-io "write -z" or
scsi-disk "write same").

Commit fc3959e466 fixed bdrv_co_write_zeroes which is the common case
for this bug, but it still exists in bdrv_aio_write_zeroes. A simpler
fix would be in bdrv_co_do_pwritev which is the NULL dereference point
and covers both cases.

So don't access it in bdrv_co_do_pwritev in this case, use three aligned
writes.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 4e5a92e..d8d2989 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1174,6 +1174,94 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
     return ret;
 }
 
+static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs,
+                                                int64_t offset,
+                                                unsigned int bytes,
+                                                BdrvRequestFlags flags,
+                                                BdrvTrackedRequest *req)
+{
+    uint8_t *buf = NULL;
+    QEMUIOVector local_qiov;
+    struct iovec iov;
+    uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+    unsigned int head_padding_bytes, tail_padding_bytes;
+    int ret;
+
+    head_padding_bytes = offset & (align - 1);
+    tail_padding_bytes = align - ((offset + bytes) & (align - 1));
+
+
+    assert(flags & BDRV_REQ_ZERO_WRITE);
+    if (head_padding_bytes || tail_padding_bytes) {
+        buf = qemu_blockalign(bs, align);
+        iov = (struct iovec) {
+            .iov_base   = buf,
+            .iov_len    = align,
+        };
+        qemu_iovec_init_external(&local_qiov, &iov, 1);
+    }
+    if (head_padding_bytes) {
+        uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes);
+
+        /* RMW the unaligned part before head. */
+        mark_request_serialising(req, align);
+        wait_serialising_requests(req);
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_HEAD);
+        ret = bdrv_aligned_preadv(bs, req, offset & ~(align - 1), align,
+                                  align, &local_qiov, 0);
+        if (ret < 0) {
+            goto fail;
+        }
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
+
+        memset(buf + head_padding_bytes, 0, zero_bytes);
+        ret = bdrv_aligned_pwritev(bs, req, offset & ~(align - 1), align,
+                                   &local_qiov,
+                                   flags & ~BDRV_REQ_ZERO_WRITE);
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += zero_bytes;
+        bytes -= zero_bytes;
+    }
+
+    assert(!bytes || (offset & (align - 1)) == 0);
+    if (bytes >= align) {
+        /* Write the aligned part in the middle. */
+        uint64_t aligned_bytes = bytes & ~(align - 1);
+        ret = bdrv_aligned_pwritev(bs, req, offset, aligned_bytes,
+                                   NULL, flags);
+        if (ret < 0) {
+            goto fail;
+        }
+        bytes -= aligned_bytes;
+        offset += aligned_bytes;
+    }
+
+    assert(!bytes || (offset & (align - 1)) == 0);
+    if (bytes) {
+        assert(align == tail_padding_bytes + bytes);
+        /* RMW the unaligned part after tail. */
+        mark_request_serialising(req, align);
+        wait_serialising_requests(req);
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_TAIL);
+        ret = bdrv_aligned_preadv(bs, req, offset, align,
+                                  align, &local_qiov, 0);
+        if (ret < 0) {
+            goto fail;
+        }
+        BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
+
+        memset(buf, 0, bytes);
+        ret = bdrv_aligned_pwritev(bs, req, offset, align,
+                                   &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE);
+    }
+fail:
+    qemu_vfree(buf);
+    return ret;
+
+}
+
 /*
  * Handle a write request in coroutine context
  */
@@ -1214,6 +1302,11 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
      */
     tracked_request_begin(&req, bs, offset, bytes, true);
 
+    if (!qiov) {
+        ret = bdrv_co_do_zero_pwritev(bs, offset, bytes, flags, &req);
+        goto out;
+    }
+
     if (offset & (align - 1)) {
         QEMUIOVector head_qiov;
         struct iovec head_iov;
@@ -1287,14 +1380,14 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
                                flags);
 
 fail:
-    tracked_request_end(&req);
 
     if (use_local_qiov) {
         qemu_iovec_destroy(&local_qiov);
     }
     qemu_vfree(head_buf);
     qemu_vfree(tail_buf);
-
+out:
+    tracked_request_end(&req);
     return ret;
 }
 
-- 
2.4.0

Re: [Qemu-devel] [PATCH v7 2/3] block: Fix NULL deference for unaligned write if qiov is NULL

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

On Wed, May 13, 2015 at 01:12:00PM +0000, Fam Zheng wrote:
> For zero write, callers pass in NULL qiov (qemu-io "write -z" or
> scsi-disk "write same").
> 
> Commit fc3959e466 fixed bdrv_co_write_zeroes which is the common case
> for this bug, but it still exists in bdrv_aio_write_zeroes. A simpler
> fix would be in bdrv_co_do_pwritev which is the NULL dereference point
> and covers both cases.
> 
> So don't access it in bdrv_co_do_pwritev in this case, use three aligned
> writes.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/io.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 95 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v7 3/3] qemu-iotests: Test unaligned sub-block zero write

[Fam Zheng]

Test zero write in byte range 512~1024 for 4k alignment.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/033     | 13 +++++++++++++
 tests/qemu-iotests/033.out | 30 ++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033
index 4008f10..a61d8ce 100755
--- a/tests/qemu-iotests/033
+++ b/tests/qemu-iotests/033
@@ -78,6 +78,19 @@ for align in 512 4k; do
 	echo
 	echo "== verifying patterns (2) =="
 	do_test $align "read -P 0x0 0x400 0x20000" "$TEST_IMG" | _filter_qemu_io
+
+	echo
+	echo "== rewriting unaligned zeroes =="
+	do_test $align "write -P 0xb 0x0 0x1000" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "write -z 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
+
+	echo
+	echo "== verifying patterns (3) =="
+	do_test $align "read -P 0xb 0x0 0x200" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "read -P 0x0 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
+	do_test $align "read -P 0xb 0x400 0xc00" "$TEST_IMG" | _filter_qemu_io
+
+	echo
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/033.out b/tests/qemu-iotests/033.out
index 305949f..c3d18aa 100644
--- a/tests/qemu-iotests/033.out
+++ b/tests/qemu-iotests/033.out
@@ -27,6 +27,21 @@ wrote 65536/65536 bytes at offset 65536
 read 131072/131072 bytes at offset 1024
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+== rewriting unaligned zeroes ==
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (3) ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 3072/3072 bytes at offset 1024
+3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
 == preparing image ==
 wrote 1024/1024 bytes at offset 512
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -52,4 +67,19 @@ wrote 65536/65536 bytes at offset 65536
 == verifying patterns (2) ==
 read 131072/131072 bytes at offset 1024
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting unaligned zeroes ==
+wrote 4096/4096 bytes at offset 0
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (3) ==
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 3072/3072 bytes at offset 1024
+3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 *** done
-- 
2.4.0

Re: [Qemu-devel] [PATCH v7 0/3] block: Fix unaligned bdrv_aio_write_zeroes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Wed, May 13, 2015 at 01:11:58PM +0000, Fam Zheng wrote:
> v7: Add Kevin's rev-by in patch 1 and 3.
>     Address Stefan's and Kevin's comments on patch 2:
>     - Don't duplicate tracked_request_begin and tracked_request_end;
>     - Don't forget to remove debug printf;
>     - Call qemu_vfree unconditionally;
>     - Don't serialize aligned part of the zero write req;
> 
> An unaligned zero write causes NULL deferencing in bdrv_co_do_pwritev. That
> path is reachable from bdrv_co_write_zeroes and bdrv_aio_write_zeroes.
> 
> You can easily trigger through the former with qemu-io, as the test case added
> by 61815d6e0aa. For bdrv_aio_write_zeroes, in common cases there's always a
> format driver (which uses 512 alignment), so it would be much rarer to have
> unaligned requests (only concerning top level here, when the request goes down
> to bs->file, where for example the alignment is 4k, it would then be calling
> bdrv_co_write_zeroes because it's in a coroutine).
> 
> fc3959e4669a1c fixed bdrv_co_write_zeroes but not bdrv_aio_write_zeroes.  The
> lattern is the actually used one by device model. Revert the previous fix, do
> it in bdrv_co_do_pwritev, to cover both paths.
> 
> 
> 
> Fam Zheng (3):
>   Revert "block: Fix unaligned zero write"
>   block: Fix NULL deference for unaligned write if qiov is NULL
>   qemu-iotests: Test unaligned sub-block zero write
> 
>  block/io.c                 | 142 ++++++++++++++++++++++++++++++++-------------
>  tests/qemu-iotests/033     |  13 +++++
>  tests/qemu-iotests/033.out |  30 ++++++++++
>  3 files changed, 144 insertions(+), 41 deletions(-)
> 
> -- 
> 2.4.0
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v7 0/3] block: Fix unaligned bdrv_aio_write_zeroes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On Wed, May 13, 2015 at 01:11:58PM +0000, Fam Zheng wrote:
> v7: Add Kevin's rev-by in patch 1 and 3.
>     Address Stefan's and Kevin's comments on patch 2:
>     - Don't duplicate tracked_request_begin and tracked_request_end;
>     - Don't forget to remove debug printf;
>     - Call qemu_vfree unconditionally;
>     - Don't serialize aligned part of the zero write req;
> 
> An unaligned zero write causes NULL deferencing in bdrv_co_do_pwritev. That
> path is reachable from bdrv_co_write_zeroes and bdrv_aio_write_zeroes.
> 
> You can easily trigger through the former with qemu-io, as the test case added
> by 61815d6e0aa. For bdrv_aio_write_zeroes, in common cases there's always a
> format driver (which uses 512 alignment), so it would be much rarer to have
> unaligned requests (only concerning top level here, when the request goes down
> to bs->file, where for example the alignment is 4k, it would then be calling
> bdrv_co_write_zeroes because it's in a coroutine).
> 
> fc3959e4669a1c fixed bdrv_co_write_zeroes but not bdrv_aio_write_zeroes.  The
> lattern is the actually used one by device model. Revert the previous fix, do
> it in bdrv_co_do_pwritev, to cover both paths.
> 
> 
> 
> Fam Zheng (3):
>   Revert "block: Fix unaligned zero write"
>   block: Fix NULL deference for unaligned write if qiov is NULL
>   qemu-iotests: Test unaligned sub-block zero write
> 
>  block/io.c                 | 142 ++++++++++++++++++++++++++++++++-------------
>  tests/qemu-iotests/033     |  13 +++++
>  tests/qemu-iotests/033.out |  30 ++++++++++
>  3 files changed, 144 insertions(+), 41 deletions(-)

By the way, gcc 4.9.2 says:

block/io.c: In function ‘bdrv_co_do_pwritev’:
block/io.c:1193:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret;
              ^

I have squashed a fix.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]