diff mbox

[v3,2/3] block: Fix NULL deference for unaligned write if qiov is NULL

Message ID 1430113219-12522-3-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng April 27, 2015, 5:40 a.m. UTC 
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(-)

Comments

Paolo Bonzini April 27, 2015, 10:45 a.m. UTC | #1 
On 27/04/2015 07:40, Fam Zheng wrote:
> +
> +    if (!qiov) {

Perhaps "if (!qiov && bytes >= align)"?

Paolo

> +        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;
>      }
Fam Zheng April 27, 2015, 12:41 p.m. UTC | #2 
On Mon, 04/27 12:45, Paolo Bonzini wrote:
> 
> 
> On 27/04/2015 07:40, Fam Zheng wrote:
> > +
> > +    if (!qiov) {
> 
> Perhaps "if (!qiov && bytes >= align)"?

Yes, that's right, we don't want 0 aligned_bytes here.

Fam

> 
> Paolo
> 
> > +        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;
> >      }
>
diff mbox

Patch

diff --git a/block.c b/block.c
index 0fe97de..d0b9a4e 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) {
+        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);