From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34669) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlbLZ-00010L-Cg for qemu-devel@nongnu.org; Fri, 24 Apr 2015 07:00:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlbLY-0003TY-9b for qemu-devel@nongnu.org; Fri, 24 Apr 2015 07:00:37 -0400 Sender: Paolo Bonzini Message-ID: <553A2245.9060304@redhat.com> Date: Fri, 24 Apr 2015 13:00:21 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1429871600-10180-1-git-send-email-famz@redhat.com> <1429871600-10180-3-git-send-email-famz@redhat.com> In-Reply-To: <1429871600-10180-3-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , qemu-stable@nongnu.org, qemu-block@nongnu.org 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 > --- > 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