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

[Qemu-devel] [PATCH v4 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.

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.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 v4 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.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 45 ++++++---------------------------------------
 1 file changed, 6 insertions(+), 39 deletions(-)

diff --git a/block.c b/block.c
index f2f8ae7..0fe97de 100644
--- a/block.c
+++ b/block.c
@@ -3118,19 +3118,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
  */
@@ -3141,7 +3128,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;
@@ -3383,7 +3371,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;
@@ -3482,10 +3471,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);
@@ -3526,32 +3511,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);
 }
 
 /**
-- 
1.9.3

[Qemu-devel] [PATCH v4 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.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 63 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 0fe97de..33e586d 100644
--- a/block.c
+++ b/block.c
@@ -3403,6 +3403,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;
@@ -3425,13 +3427,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)) {
@@ -3459,21 +3487,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

[Qemu-devel] [PATCH v4 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 v4 0/3] block: Fix unaligned bdrv_aio_write_zeroes

[Paolo Bonzini]

On 27/04/2015 15:18, Fam Zheng wrote:
> 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.
> 
> 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.c                    | 123 +++++++++++++++++++++++++--------------------
>  tests/qemu-iotests/033     |  13 +++++
>  tests/qemu-iotests/033.out |  30 +++++++++++
>  3 files changed, 111 insertions(+), 55 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Fam Zheng]

On Mon, 04/27 15:23, Paolo Bonzini wrote:
> 
> 
> On 27/04/2015 15:18, Fam Zheng wrote:
> > 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.
> > 
> > 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.c                    | 123 +++++++++++++++++++++++++--------------------
> >  tests/qemu-iotests/033     |  13 +++++
> >  tests/qemu-iotests/033.out |  30 +++++++++++
> >  3 files changed, 111 insertions(+), 55 deletions(-)
> > 
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 

Thanks very much for your attentive reviewing!

Fam

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

[Kevin Wolf]

Am 27.04.2015 um 15:18 hat Fam Zheng geschrieben:
> 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.

Hi Fam,

Stefan's patch to split out block/io.c conflicts with this. Can you
please rebase?

Kevin

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

[Fam Zheng]

On Thu, 04/30 15:34, Kevin Wolf wrote:
> Am 27.04.2015 um 15:18 hat Fam Zheng geschrieben:
> > 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.
> 
> Hi Fam,
> 
> Stefan's patch to split out block/io.c conflicts with this. Can you
> please rebase?

I'll do it.

Fam

> 
> Kevin
>