Message ID | 1429864436-17711-3-git-send-email-famz@redhat.com |
---|---|
State | New |
Headers | show |
On 24/04/2015 10: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, use a zeroed buffer instead. > Device model who calls into block layer should check the request size. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index f2f8ae7..878a72d 100644 > --- a/block.c > +++ b/block.c > @@ -3386,6 +3386,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs, > uint64_t align = bdrv_get_align(bs); > uint8_t *head_buf = NULL; > uint8_t *tail_buf = NULL; > + uint8_t *qiov_buf = NULL; > QEMUIOVector local_qiov; > bool use_local_qiov = false; > int ret; > @@ -3436,9 +3437,15 @@ 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_init(&local_qiov, qiov ? qiov->niov + 2 : 3); > qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1)); > - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + if (qiov) { > + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + } else { If we keep this approach, I would add an assertion that the flags include BDRV_REQ_ZERO_WRITE. Same below. However, the problem here is that you drop the BDRV_REQ_MAY_UNMAP flag for the central part of the write. I think if BDRV_REQ_ZERO_WRITE is set, you should do three bdrv_aligned_pwritev rather than just one. The first without BDRV_REQ_ZERO_WRITE and inside the if (offset & (align - 1)); the second with BDRV_REQ_ZERO_WRITE and with NULL qiov, right after that if; and the third is the one that exists already. After each write you modify offset and bytes. leaving > + qiov_buf = qemu_blockalign(bs, bytes); > + memset(qiov_buf, 0, bytes); > + qemu_iovec_add(&local_qiov, qiov_buf, bytes); > + } > use_local_qiov = true; > > bytes += offset & (align - 1); > @@ -3471,8 +3478,16 @@ 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); > + qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 1 : 2); > + if (qiov) { > + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + } else { > + if (!qiov_buf) { > + qiov_buf = qemu_blockalign(bs, bytes); > + memset(qiov_buf, 0, bytes); > + } > + qemu_iovec_add(&local_qiov, qiov_buf, bytes); > + } Same here. > use_local_qiov = true; > } > > @@ -3498,6 +3513,7 @@ fail: > } > qemu_vfree(head_buf); > qemu_vfree(tail_buf); > + qemu_vfree(qiov_buf); > > return ret; > } >
diff --git a/block.c b/block.c index f2f8ae7..878a72d 100644 --- a/block.c +++ b/block.c @@ -3386,6 +3386,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs, uint64_t align = bdrv_get_align(bs); uint8_t *head_buf = NULL; uint8_t *tail_buf = NULL; + uint8_t *qiov_buf = NULL; QEMUIOVector local_qiov; bool use_local_qiov = false; int ret; @@ -3436,9 +3437,15 @@ 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_init(&local_qiov, qiov ? qiov->niov + 2 : 3); qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1)); - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); + if (qiov) { + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); + } else { + qiov_buf = qemu_blockalign(bs, bytes); + memset(qiov_buf, 0, bytes); + qemu_iovec_add(&local_qiov, qiov_buf, bytes); + } use_local_qiov = true; bytes += offset & (align - 1); @@ -3471,8 +3478,16 @@ 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); + qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 1 : 2); + if (qiov) { + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); + } else { + if (!qiov_buf) { + qiov_buf = qemu_blockalign(bs, bytes); + memset(qiov_buf, 0, bytes); + } + qemu_iovec_add(&local_qiov, qiov_buf, bytes); + } use_local_qiov = true; } @@ -3498,6 +3513,7 @@ fail: } qemu_vfree(head_buf); qemu_vfree(tail_buf); + qemu_vfree(qiov_buf); return ret; }
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, use a zeroed buffer instead. Device model who calls into block layer should check the request size. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)