block/io: fix bdrv_co_do_copy_on_readv

[PATCH] block/io: fix bdrv_co_do_copy_on_readv

Posted by Vladimir Sementsov-Ogievskiy 5 years, 8 months ago

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(-)

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 */
-- 
2.21.0

Re: [PATCH] block/io: fix bdrv_co_do_copy_on_readv

Posted by John Snow 5 years, 8 months ago


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>

Re: [PATCH] block/io: fix bdrv_co_do_copy_on_readv

Posted by Vladimir Sementsov-Ogievskiy 5 years, 8 months ago

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!

-- 
Best regards,
Vladimir

Re: [PATCH] block/io: fix bdrv_co_do_copy_on_readv

Posted by Stefan Hajnoczi 5 years, 8 months ago

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