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

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

[Fam Zheng]

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.

v5: Rebase on to the block/io.c split. (Kevin)

v4: "if (!qiov)" -> "if (!qiov && bytes >= align)". (Paolo)

v3: Fix the case where the unaligned request is contained within the first
    block. (Paolo)
    Also update iotests 033 to cover the code path with qemu-io.

v2: Split to three aligned pwritev.


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                 | 123 +++++++++++++++++++++++++--------------------
 tests/qemu-iotests/033     |  13 +++++
 tests/qemu-iotests/033.out |  30 +++++++++++
 3 files changed, 111 insertions(+), 55 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v5 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>
---
 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)
-- 
1.9.3

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

[Stefan Hajnoczi]

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

On Tue, May 05, 2015 at 10:51:13AM +0800, Fam Zheng wrote:
> 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>
> ---
>  block/io.c | 45 ++++++---------------------------------------
>  1 file changed, 6 insertions(+), 39 deletions(-)

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

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

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

[Fam Zheng]

For zero write, qiov passed by callers (qemu-io "write -z" and scsi-disk
"write same") is NULL.

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 | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 63 insertions(+), 17 deletions(-)

diff --git a/block/io.c b/block/io.c
index 4e5a92e..b4b587d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1214,6 +1214,8 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
      */
     tracked_request_begin(&req, bs, offset, bytes, true);
 
+    assert(qiov || flags & BDRV_REQ_ZERO_WRITE);
+
     if (offset & (align - 1)) {
         QEMUIOVector head_qiov;
         struct iovec head_iov;
@@ -1236,13 +1238,39 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
         }
         BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
 
-        qemu_iovec_init(&local_qiov, qiov->niov + 2);
-        qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
-        qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
-        use_local_qiov = true;
+        if (qiov) {
+            qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 2 : 1);
+            qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
+            qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
+            use_local_qiov = true;
+            bytes += offset & (align - 1);
+            offset = offset & ~(align - 1);
+        } else {
+            uint64_t local_off = offset & (align - 1);
+            uint64_t local_bytes = MIN(bytes, align - local_off);
 
-        bytes += offset & (align - 1);
-        offset = offset & ~(align - 1);
+            memset(head_buf + local_off, 0, local_bytes);
+            ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
+                                       &head_qiov, 0);
+            if (ret < 0) {
+                goto fail;
+            }
+            bytes -= local_bytes;
+            offset = ROUND_UP(offset, align);
+        }
+    }
+
+    if (!qiov && bytes >= align) {
+        uint64_t aligned_bytes = bytes & ~(align - 1);
+
+        assert((offset & (align - 1)) == 0);
+        ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes,
+                                   NULL, flags);
+        if (ret < 0) {
+            goto fail;
+        }
+        bytes -= aligned_bytes;
+        offset += aligned_bytes;
     }
 
     if ((offset + bytes) & (align - 1)) {
@@ -1270,21 +1298,39 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
         }
         BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
 
-        if (!use_local_qiov) {
-            qemu_iovec_init(&local_qiov, qiov->niov + 1);
-            qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
-            use_local_qiov = true;
+        if (qiov) {
+            if (!use_local_qiov) {
+                qemu_iovec_init(&local_qiov, qiov->niov + 1);
+                qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
+                use_local_qiov = true;
+            }
+
+            tail_bytes = (offset + bytes) & (align - 1);
+            qemu_iovec_add(&local_qiov, tail_buf + tail_bytes,
+                           align - tail_bytes);
+
+            bytes = ROUND_UP(bytes, align);
+        } else {
+            assert((offset & (align - 1)) == 0);
+            assert(bytes < align);
+
+            memset(tail_buf, 0, bytes & (align - 1));
+            ret = bdrv_aligned_pwritev(bs, &req, offset, align,
+                                       &tail_qiov, 0);
+            if (ret < 0) {
+                goto fail;
+            }
+            offset += align;
+            bytes = 0;
         }
 
-        tail_bytes = (offset + bytes) & (align - 1);
-        qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - tail_bytes);
-
-        bytes = ROUND_UP(bytes, align);
     }
 
-    ret = bdrv_aligned_pwritev(bs, &req, offset, bytes,
-                               use_local_qiov ? &local_qiov : qiov,
-                               flags);
+    if (bytes) {
+        ret = bdrv_aligned_pwritev(bs, &req, offset, bytes,
+                                   use_local_qiov ? &local_qiov : qiov,
+                                   flags);
+    }
 
 fail:
     tracked_request_end(&req);
-- 
1.9.3

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

[Stefan Hajnoczi]

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

On Tue, May 05, 2015 at 10:51:14AM +0800, Fam Zheng wrote:

This function is complex.  I had to draw a diagram to remember the
relationships between the variables.  It would be nice to split it if
that can be done in a way that makes the code nicer.

> @@ -1236,13 +1238,39 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
>          }
>          BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
>  
> -        qemu_iovec_init(&local_qiov, qiov->niov + 2);
> -        qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
> -        qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> -        use_local_qiov = true;
> +        if (qiov) {
> +            qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 2 : 1);

Should be:
qemu_iovec_init(&local_qiov, qiov->niov + 2);

since we know qiov != NULL here.

> @@ -1270,21 +1298,39 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
>          }
>          BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
>  
> -        if (!use_local_qiov) {
> -            qemu_iovec_init(&local_qiov, qiov->niov + 1);
> -            qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> -            use_local_qiov = true;
> +        if (qiov) {
> +            if (!use_local_qiov) {
> +                qemu_iovec_init(&local_qiov, qiov->niov + 1);
> +                qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> +                use_local_qiov = true;
> +            }
> +
> +            tail_bytes = (offset + bytes) & (align - 1);
> +            qemu_iovec_add(&local_qiov, tail_buf + tail_bytes,
> +                           align - tail_bytes);
> +
> +            bytes = ROUND_UP(bytes, align);
> +        } else {
> +            assert((offset & (align - 1)) == 0);
> +            assert(bytes < align);
> +
> +            memset(tail_buf, 0, bytes & (align - 1));

Since assert(bytes < align) is above, the following is clearer:

memset(tail_buf, 0, bytes)

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

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

[Fam Zheng]

On Mon, 05/11 15:43, Stefan Hajnoczi wrote:
> On Tue, May 05, 2015 at 10:51:14AM +0800, Fam Zheng wrote:
> 
> This function is complex.  I had to draw a diagram to remember the
> relationships between the variables.  It would be nice to split it if
> that can be done in a way that makes the code nicer.

OK. I can split zero write to a separate function.

[Qemu-devel] [PATCH v5 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>
---
 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
-- 
1.9.3

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

[Stefan Hajnoczi]

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

On Tue, May 05, 2015 at 10:51:15AM +0800, Fam Zheng wrote:
> Test zero write in byte range 512~1024 for 4k alignment.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/033     | 13 +++++++++++++
>  tests/qemu-iotests/033.out | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)

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

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