block/io: fix bdrv_co_do_copy_on_readv error handling

[Qemu-devel] [PATCH] block/io: fix bdrv_co_do_copy_on_readv error handling

Vladimir Sementsov-Ogievskiy posted 1 patch 5 years, 1 month ago

Download mbox

Test asan failed

Test docker-clang@ubuntu passed

Test docker-mingw@fedora passed

Test checkpatch passed

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190314101415.15427-1-vsementsov@virtuozzo.com

Maintainers: Max Reitz <mreitz@redhat.com>, Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>

block/io.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

Expand all Fold all

[Qemu-devel] [PATCH] block/io: fix bdrv_co_do_copy_on_readv error handling

Posted by Vladimir Sementsov-Ogievskiy 5 years, 1 month ago

It's not safe to treat bdrv_is_allocated error as unallocated: if we
mistake we may rewrite guest data.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index 2ba603c7bc..dccad64d46 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1193,11 +1193,13 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
         ret = bdrv_is_allocated(bs, cluster_offset,
                                 MIN(cluster_bytes, max_transfer), &pnum);
         if (ret < 0) {
-            /* Safe to treat errors in querying allocation as if
-             * unallocated; we'll probably fail again soon on the
-             * read, but at least that will set a decent errno.
+            /*
+             * We must fail here, and can't treat error as allocated or
+             * unallocated: if we mistake, it will lead to not done copy-on-read
+             * in first case and to rewriting guest data that is already in top
+             * layer in the second case.
              */
-            pnum = MIN(cluster_bytes, max_transfer);
+            goto err;
         }
 
         /* Stop at EOF if the image ends in the middle of the cluster */
@@ -1208,7 +1210,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
 
         assert(skip_bytes < pnum);
 
-        if (ret <= 0) {
+        if (ret == 0) {
             /* Must copy-on-read; use the bounce buffer */
             pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
             qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
-- 
2.18.0

Re: [Qemu-devel] [PATCH] block/io: fix bdrv_co_do_copy_on_readv error handling

Posted by Vladimir Sementsov-Ogievskiy 5 years, 1 month ago

14.03.2019 13:14, Vladimir Sementsov-Ogievskiy wrote:
> It's not safe to treat bdrv_is_allocated error as unallocated: if we
> mistake we may rewrite guest data.

... with same data, which is not so bad.

So, it's ok, I'm wrong, drop it.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/io.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 2ba603c7bc..dccad64d46 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1193,11 +1193,13 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>           ret = bdrv_is_allocated(bs, cluster_offset,
>                                   MIN(cluster_bytes, max_transfer), &pnum);
>           if (ret < 0) {
> -            /* Safe to treat errors in querying allocation as if
> -             * unallocated; we'll probably fail again soon on the
> -             * read, but at least that will set a decent errno.
> +            /*
> +             * We must fail here, and can't treat error as allocated or
> +             * unallocated: if we mistake, it will lead to not done copy-on-read
> +             * in first case and to rewriting guest data that is already in top
> +             * layer in the second case.
>                */
> -            pnum = MIN(cluster_bytes, max_transfer);
> +            goto err;
>           }
>   
>           /* Stop at EOF if the image ends in the middle of the cluster */
> @@ -1208,7 +1210,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>   
>           assert(skip_bytes < pnum);
>   
> -        if (ret <= 0) {
> +        if (ret == 0) {
>               /* Must copy-on-read; use the bounce buffer */
>               pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
>               qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
> 


-- 
Best regards,
Vladimir