[Qemu-devel] [PATCH v2 0/2] block: Fix unaligned bdrv_aio_write_zeroes

[Qemu-devel] [PATCH v2 0/2] 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.

v2: Split to three aligned pwritev.

Fam Zheng (2):
  Revert "block: Fix unaligned zero write"
  block: Fix NULL deference for unaligned write if qiov is NULL

 block.c | 121 +++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 66 insertions(+), 55 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/2] 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 v2 2/2] 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 | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 0fe97de..cbd0708 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,37 @@ 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 {
+            memset(head_buf + (offset & (align - 1)), 0,
+                   align - (offset & (align - 1)));
+            ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
+                                       &head_qiov, 0);
+            if (ret < 0) {
+                goto fail;
+            }
+            bytes -= align - (offset & (align - 1));
+            offset = ROUND_UP(offset, align);
+        }
+    }
 
-        bytes += offset & (align - 1);
-        offset = offset & ~(align - 1);
+    if (!qiov) {
+        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 +3485,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] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL

[Paolo Bonzini]

On 24/04/2015 12:33, Fam Zheng wrote:
> 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 | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0fe97de..cbd0708 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);

Perhaps as a follow-up you can add

if ((flags & (BDRV_REQ_ZERO_WRITE|BDRV_REQ_MAY_UNMAP))
    == BDRV_REQ_ZERO_WRITE|BDRV_REQ_MAY_UNMAP) {
    qiov = NULL;
}

so that the central area is always unmapped.  You can have non-NULL qiov
if the flags were added because of detect-zeroes=unmap.

But in any case that would be a separate change.

>      if (offset & (align - 1)) {
>          QEMUIOVector head_qiov;
>          struct iovec head_iov;
> @@ -3425,13 +3427,37 @@ 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 {
> +            memset(head_buf + (offset & (align - 1)), 0,
> +                   align - (offset & (align - 1)));
> +            ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
> +                                       &head_qiov, 0);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +            bytes -= align - (offset & (align - 1));
> +            offset = ROUND_UP(offset, align);
> +        }
> +    }
>  
> -        bytes += offset & (align - 1);
> -        offset = offset & ~(align - 1);
> +    if (!qiov) {
> +        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 +3485,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);
> 

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

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

[Fam Zheng]

On Fri, 04/24 18:33, Fam Zheng wrote:
> This reverts commit fc3959e4669a1c2149b91ccb05101cfc7ae1fc05.
> 
> The core write code already handles the case, so remove this
> duplication.

Not already.

I meant to put this patch after 2/2 so there is no intermediate state. So if
apply the series, please fix the order.

Fam

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

[Paolo Bonzini]

On 24/04/2015 13:00, Paolo Bonzini wrote:
>> -        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 {
>> +            memset(head_buf + (offset & (align - 1)), 0,
>> +                   align - (offset & (align - 1)));

Actually, is the byte count correct if bytes < align?  In the case of
your testcase, you'd destroy bytes 1536..4095.

Same for the computation of bytes, below.  It could underflow.

Perhaps a qemu-iotests testcase, using qemu-io, is also necessary.

Paolo

>> +            ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
>> +                                       &head_qiov, 0);
>> +            if (ret < 0) {
>> +                goto fail;
>> +            }
>> +            bytes -= align - (offset & (align - 1));
>> +            offset = ROUND_UP(offset, align);
>> +        }
>> +    }

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

[Fam Zheng]

On Fri, 04/24 13:51, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 13:00, Paolo Bonzini wrote:
> >> -        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 {
> >> +            memset(head_buf + (offset & (align - 1)), 0,
> >> +                   align - (offset & (align - 1)));
> 
> Actually, is the byte count correct if bytes < align?  In the case of
> your testcase, you'd destroy bytes 1536..4095.

Yes, good catch!

Fam

> 
> Same for the computation of bytes, below.  It could underflow.
> 
> Perhaps a qemu-iotests testcase, using qemu-io, is also necessary.
> 
> Paolo
> 
> >> +            ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align,
> >> +                                       &head_qiov, 0);
> >> +            if (ret < 0) {
> >> +                goto fail;
> >> +            }
> >> +            bytes -= align - (offset & (align - 1));
> >> +            offset = ROUND_UP(offset, align);
> >> +        }
> >> +    }