| Message ID | 20200206164245.17781-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: fix crash on zero-length unaligned write and read | expand |
On 06.02.20 17:42, Vladimir Sementsov-Ogievskiy wrote: > Commit 7a3f542fbd "block/io: refactor padding" occasionally dropped > aligning for zero-length request: bdrv_init_padding() blindly return > false if bytes == 0, like there is nothing to align. > > This leads the following command to crash: > > ./qemu-io --image-opts -c 'write 1 0' \ > driver=blkdebug,align=512,image.driver=null-co,image.size=512 > >>> qemu-io: block/io.c:1955: bdrv_aligned_pwritev: Assertion > `(offset & (align - 1)) == 0' failed. >>> Aborted (core dumped) > > Prior to 7a3f542fbd we does aligning of such zero requests. Instead of > recovering this behavior let's just do nothing on such requests as it > is useless. > > Note that driver may have special meaning of zero-length reqeusts, like > qcow2_co_pwritev_compressed_part, so we can't skip any zero-length > operation. But for unaligned ones, we can't pass it to driver anyway. > > This commit also fixes crash in iotest 80 running with -nocache: > > ./check -nocache -qcow2 80 > > which crashes on same assertion due to trying to read empty extra data > in qcow2_do_read_snapshots(). > > Cc: qemu-stable@nongnu.org # v4.2 > Fixes: 7a3f542fbd > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/io.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) Zero-length reads would still trigger CORs when padded. But there is no reason to assume or rely on this, so: Reviewed-by: Max Reitz <mreitz@redhat.com> (block/io.c is Stefan’s department. :-)) Max
On Thu, Feb 06, 2020 at 07:42:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Commit 7a3f542fbd "block/io: refactor padding" occasionally dropped > aligning for zero-length request: bdrv_init_padding() blindly return > false if bytes == 0, like there is nothing to align. > > This leads the following command to crash: > > ./qemu-io --image-opts -c 'write 1 0' \ > driver=blkdebug,align=512,image.driver=null-co,image.size=512 > > >> qemu-io: block/io.c:1955: bdrv_aligned_pwritev: Assertion > `(offset & (align - 1)) == 0' failed. > >> Aborted (core dumped) > > Prior to 7a3f542fbd we does aligning of such zero requests. Instead of > recovering this behavior let's just do nothing on such requests as it > is useless. > > Note that driver may have special meaning of zero-length reqeusts, like > qcow2_co_pwritev_compressed_part, so we can't skip any zero-length > operation. But for unaligned ones, we can't pass it to driver anyway. > > This commit also fixes crash in iotest 80 running with -nocache: > > ./check -nocache -qcow2 80 > > which crashes on same assertion due to trying to read empty extra data > in qcow2_do_read_snapshots(). > > Cc: qemu-stable@nongnu.org # v4.2 > Fixes: 7a3f542fbd > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/io.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
07.02.2020 19:47, Stefan Hajnoczi wrote: > On Thu, Feb 06, 2020 at 07:42:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> Commit 7a3f542fbd "block/io: refactor padding" occasionally dropped >> aligning for zero-length request: bdrv_init_padding() blindly return >> false if bytes == 0, like there is nothing to align. >> >> This leads the following command to crash: >> >> ./qemu-io --image-opts -c 'write 1 0' \ >> driver=blkdebug,align=512,image.driver=null-co,image.size=512 >> >>>> qemu-io: block/io.c:1955: bdrv_aligned_pwritev: Assertion >> `(offset & (align - 1)) == 0' failed. >>>> Aborted (core dumped) >> >> Prior to 7a3f542fbd we does aligning of such zero requests. Instead of >> recovering this behavior let's just do nothing on such requests as it >> is useless. >> >> Note that driver may have special meaning of zero-length reqeusts, like >> qcow2_co_pwritev_compressed_part, so we can't skip any zero-length >> operation. But for unaligned ones, we can't pass it to driver anyway. >> >> This commit also fixes crash in iotest 80 running with -nocache: >> >> ./check -nocache -qcow2 80 >> >> which crashes on same assertion due to trying to read empty extra data >> in qcow2_do_read_snapshots(). >> >> Cc: qemu-stable@nongnu.org # v4.2 >> Fixes: 7a3f542fbd >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/io.c | 28 +++++++++++++++++++++++++++- >> 1 file changed, 27 insertions(+), 1 deletion(-) > > Thanks, applied to my block tree: > https://github.com/stefanha/qemu/commits/block > Thanks!
diff --git a/block/io.c b/block/io.c index 1eb2b2bddc..7e4cb74cf4 100644 --- a/block/io.c +++ b/block/io.c @@ -1565,10 +1565,12 @@ static bool bdrv_init_padding(BlockDriverState *bs, pad->tail = align - pad->tail; } - if ((!pad->head && !pad->tail) || !bytes) { + if (!pad->head && !pad->tail) { return false; } + assert(bytes); /* Nothing good in aligning zero-length requests */ + sum = pad->head + bytes + pad->tail; pad->buf_len = (sum > align && pad->head && pad->tail) ? 2 * align : align; pad->buf = qemu_blockalign(bs, pad->buf_len); @@ -1706,6 +1708,18 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child, return ret; } + if (bytes == 0 && !QEMU_IS_ALIGNED(offset, bs->bl.request_alignment)) { + /* + * Aligning zero request is nonsense. Even if driver has special meaning + * of zero-length (like qcow2_co_pwritev_compressed_part), we can't pass + * it to driver due to request_alignment. + * + * Still, no reason to return an error if someone do unaligned + * zero-length read occasionally. + */ + return 0; + } + bdrv_inc_in_flight(bs); /* Don't do copy-on-read if we read data before write operation */ @@ -2116,6 +2130,18 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child, return -ENOTSUP; } + if (bytes == 0 && !QEMU_IS_ALIGNED(offset, bs->bl.request_alignment)) { + /* + * Aligning zero request is nonsense. Even if driver has special meaning + * of zero-length (like qcow2_co_pwritev_compressed_part), we can't pass + * it to driver due to request_alignment. + * + * Still, no reason to return an error if someone do unaligned + * zero-length write occasionally. + */ + return 0; + } + bdrv_inc_in_flight(bs); /* * Align write if necessary by performing a read-modify-write cycle.