Re: [PATCH] block/io: fix bdrv_co_do_copy_on_readv

[Stefan Hajnoczi]

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-sta...@nongnu.org # v4.2
> Fixes: 1143ec5ebf4
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  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


signature.asc
Description: PGP signature

Re: [PATCH] block/io: fix bdrv_co_do_copy_on_readv

[Vladimir Sementsov-Ogievskiy]

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-sta...@nongnu.org # v4.2
Fixes: 1143ec5ebf4
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  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 



Thanks!

--
Best regards,
Vladimir

Re: [PATCH] block/io: fix bdrv_co_do_copy_on_readv

[John Snow]

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-sta...@nongnu.org # v4.2
> Fixes: 1143ec5ebf4
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  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