* [Qemu-devel] [PATCH v2 0/2] block: Fix unaligned bdrv_aio_write_zeroes @ 2015-04-24 10:33 Fam Zheng 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 1/2] Revert "block: Fix unaligned zero write" Fam Zheng 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL Fam Zheng 0 siblings, 2 replies; 7+ messages in thread From: Fam Zheng @ 2015-04-24 10:33 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, pbonzini, qemu-block, qemu-stable, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] Revert "block: Fix unaligned zero write" 2015-04-24 10:33 [Qemu-devel] [PATCH v2 0/2] block: Fix unaligned bdrv_aio_write_zeroes Fam Zheng @ 2015-04-24 10:33 ` Fam Zheng 2015-04-24 11:12 ` Fam Zheng 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL Fam Zheng 1 sibling, 1 reply; 7+ messages in thread From: Fam Zheng @ 2015-04-24 10:33 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, pbonzini, qemu-block, qemu-stable, Stefan Hajnoczi 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] Revert "block: Fix unaligned zero write" 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 1/2] Revert "block: Fix unaligned zero write" Fam Zheng @ 2015-04-24 11:12 ` Fam Zheng 0 siblings, 0 replies; 7+ messages in thread From: Fam Zheng @ 2015-04-24 11:12 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, pbonzini, Stefan Hajnoczi, qemu-stable, qemu-block 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL 2015-04-24 10:33 [Qemu-devel] [PATCH v2 0/2] block: Fix unaligned bdrv_aio_write_zeroes Fam Zheng 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 1/2] Revert "block: Fix unaligned zero write" Fam Zheng @ 2015-04-24 10:33 ` Fam Zheng 2015-04-24 11:00 ` Paolo Bonzini 1 sibling, 1 reply; 7+ messages in thread From: Fam Zheng @ 2015-04-24 10:33 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, pbonzini, qemu-block, qemu-stable, Stefan Hajnoczi 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL Fam Zheng @ 2015-04-24 11:00 ` Paolo Bonzini 2015-04-24 11:51 ` Paolo Bonzini 0 siblings, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2015-04-24 11:00 UTC (permalink / raw) To: Fam Zheng, qemu-devel Cc: Kevin Wolf, Stefan Hajnoczi, qemu-stable, qemu-block 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL 2015-04-24 11:00 ` Paolo Bonzini @ 2015-04-24 11:51 ` Paolo Bonzini 2015-04-27 5:17 ` Fam Zheng 0 siblings, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2015-04-24 11:51 UTC (permalink / raw) To: Fam Zheng, qemu-devel; +Cc: qemu-block, qemu-stable, Stefan Hajnoczi 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); >> + } >> + } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL 2015-04-24 11:51 ` Paolo Bonzini @ 2015-04-27 5:17 ` Fam Zheng 0 siblings, 0 replies; 7+ messages in thread From: Fam Zheng @ 2015-04-27 5:17 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-block, qemu-devel, Stefan Hajnoczi, qemu-stable 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); > >> + } > >> + } ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-27 5:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-04-24 10:33 [Qemu-devel] [PATCH v2 0/2] block: Fix unaligned bdrv_aio_write_zeroes Fam Zheng 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 1/2] Revert "block: Fix unaligned zero write" Fam Zheng 2015-04-24 11:12 ` Fam Zheng 2015-04-24 10:33 ` [Qemu-devel] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL Fam Zheng 2015-04-24 11:00 ` Paolo Bonzini 2015-04-24 11:51 ` Paolo Bonzini 2015-04-27 5:17 ` Fam Zheng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).