[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [PATCH v2 2/2] block: Fix NULL deference for unaligned
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-stable] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL |
Date: |
Fri, 24 Apr 2015 13:00:21 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>