| Message ID | 20200312081949.5350-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/io: fix bdrv_co_do_copy_on_readv | expand |
On 3/12/20 4:19 AM, Vladimir Sementsov-Ogievskiy wrote: > Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up > buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end > anyway. > > But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on > part of original qiov, defined by qiov_offset and bytes. So we must not > touch qiov behind qiov_offset+bytes bound. Fix it. > For the purposes of the stable branch commit log, how does the bug manifest? Are there known cases? What's the impact? (Do we have tests?) > Cc: qemu-stable@nongnu.org # v4.2 > Fixes: 1143ec5ebf4 > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/io.c b/block/io.c > index 7e4cb74cf4..aba67f66b9 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1399,7 +1399,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child, > if (!(flags & BDRV_REQ_PREFETCH)) { > qemu_iovec_from_buf(qiov, qiov_offset + progress, > bounce_buffer + skip_bytes, > - pnum - skip_bytes); > + MIN(pnum - skip_bytes, bytes - progress)); > } > } else if (!(flags & BDRV_REQ_PREFETCH)) { > /* Read directly into the destination */ > Even if I don't understand the bug, the tighter bound seems provably correct anyway, so... Reviewed-by: John Snow <jsnow@redhat.com>
13.03.2020 2:09, John Snow wrote: > > > On 3/12/20 4:19 AM, Vladimir Sementsov-Ogievskiy wrote: >> Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up >> buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end >> anyway. >> >> But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on >> part of original qiov, defined by qiov_offset and bytes. So we must not >> touch qiov behind qiov_offset+bytes bound. Fix it. >> > > For the purposes of the stable branch commit log, how does the bug > manifest? Are there known cases? What's the impact? > > (Do we have tests?) Sorry, nothing of these things. I just saw it while working with this code. > >> Cc: qemu-stable@nongnu.org # v4.2 >> Fixes: 1143ec5ebf4 >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/io.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/io.c b/block/io.c >> index 7e4cb74cf4..aba67f66b9 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -1399,7 +1399,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child, >> if (!(flags & BDRV_REQ_PREFETCH)) { >> qemu_iovec_from_buf(qiov, qiov_offset + progress, >> bounce_buffer + skip_bytes, >> - pnum - skip_bytes); >> + MIN(pnum - skip_bytes, bytes - progress)); >> } >> } else if (!(flags & BDRV_REQ_PREFETCH)) { >> /* Read directly into the destination */ >> > Even if I don't understand the bug, the tighter bound seems provably > correct anyway, so... > > Reviewed-by: John Snow <jsnow@redhat.com> > Thanks!
On Thu, Mar 12, 2020 at 11:19:49AM +0300, Vladimir Sementsov-Ogievskiy wrote: > Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up > buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end > anyway. > > But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on > part of original qiov, defined by qiov_offset and bytes. So we must not > touch qiov behind qiov_offset+bytes bound. Fix it. > > Cc: qemu-stable@nongnu.org # v4.2 > Fixes: 1143ec5ebf4 > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
diff --git a/block/io.c b/block/io.c index 7e4cb74cf4..aba67f66b9 100644 --- a/block/io.c +++ b/block/io.c @@ -1399,7 +1399,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child, if (!(flags & BDRV_REQ_PREFETCH)) { qemu_iovec_from_buf(qiov, qiov_offset + progress, bounce_buffer + skip_bytes, - pnum - skip_bytes); + MIN(pnum - skip_bytes, bytes - progress)); } } else if (!(flags & BDRV_REQ_PREFETCH)) { /* Read directly into the destination */
Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end anyway. But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on part of original qiov, defined by qiov_offset and bytes. So we must not touch qiov behind qiov_offset+bytes bound. Fix it. Cc: qemu-stable@nongnu.org # v4.2 Fixes: 1143ec5ebf4 Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)