[PATCH v2 0/4] backup: Make sure that source and target size match

[PATCH v2 0/4] backup: Make sure that source and target size match

[Kevin Wolf]

v2:
- Fixed iotest 283
- Corrected commit message for patch 3 [Vladimir]
- Fixed permissions for the source node, too
- Refactored the test case to avoid some duplication [Vladimir]

Kevin Wolf (4):
  iotests/283: Use consistent size for source and target
  backup: Improve error for bdrv_getlength() failure
  backup: Make sure that source and target size match
  iotests: Backup with different source/target size

 block/backup-top.c         | 14 ++++++++-----
 block/backup.c             | 18 +++++++++++++---
 tests/qemu-iotests/055     | 42 ++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/055.out |  4 ++--
 tests/qemu-iotests/283     |  6 +++++-
 tests/qemu-iotests/283.out |  2 +-
 6 files changed, 72 insertions(+), 14 deletions(-)

-- 
2.25.3

[PATCH v2 1/4] iotests/283: Use consistent size for source and target

[Kevin Wolf]

The test case forgot to specify the null-co size for the target node.
When adding a check to backup that both sizes match, this would fail
because of the size mismatch and not the behaviour that the test really
wanted to test.

Fixes: a541fcc27c98b96da187c7d4573f3270f3ddd283
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/283     | 6 +++++-
 tests/qemu-iotests/283.out | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
index 55b7cff953..44de76effe 100644
--- a/tests/qemu-iotests/283
+++ b/tests/qemu-iotests/283
@@ -72,7 +72,11 @@ to check that crash is fixed :)
 vm = iotests.VM()
 vm.launch()
 
-vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
+vm.qmp_log('blockdev-add', **{
+    'node-name': 'target',
+    'driver': 'null-co',
+    'size': size,
+})
 
 vm.qmp_log('blockdev-add', **{
     'node-name': 'source',
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
index daaf5828c1..d8cff22cc1 100644
--- a/tests/qemu-iotests/283.out
+++ b/tests/qemu-iotests/283.out
@@ -1,4 +1,4 @@
-{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}}
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target", "size": 1048576}}
 {"return": {}}
 {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}}
 {"return": {}}
-- 
2.25.3

[PATCH v2 2/4] backup: Improve error for bdrv_getlength() failure

[Kevin Wolf]

bdrv_get_device_name() will be an empty string with modern management
tools that don't use -drive. Use bdrv_get_device_or_node_name() instead
so that the node name is used if the BlockBackend is anonymous.

While at it, start with upper case to make the message consistent with
the rest of the function.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/backup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index a7a7dcaf4c..c4c3b8cd46 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -400,8 +400,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
 
     len = bdrv_getlength(bs);
     if (len < 0) {
-        error_setg_errno(errp, -len, "unable to get length for '%s'",
-                         bdrv_get_device_name(bs));
+        error_setg_errno(errp, -len, "Unable to get length for '%s'",
+                         bdrv_get_device_or_node_name(bs));
         goto error;
     }
 
-- 
2.25.3

[PATCH v2 3/4] backup: Make sure that source and target size match

[Kevin Wolf]

Since the introduction of a backup filter node in commit 00e30f05d, the
backup block job crashes when the target image is smaller than the
source image because it will try to write after the end of the target
node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer
would have caught this and errored out gracefully.)

We can fix this and even do better than the old behaviour: Check that
source and target have the same image size at the start of the block job
and unshare BLK_PERM_RESIZE. (This permission was already unshared
before the same commit 00e30f05d, but the BlockBackend that was used to
make the restriction was removed without a replacement.) This will
immediately error out when starting the job instead of only when writing
to a block that doesn't exist in the target.

Longer target than source would technically work because we would never
write to blocks that don't exist, but semantically these are invalid,
too, because a backup is supposed to create a copy, not just an image
that starts with a copy.

Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup-top.c | 14 +++++++++-----
 block/backup.c     | 14 +++++++++++++-
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index 3b50c06e2c..79b268e6dc 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
          *
          * Share write to target (child_file), to not interfere
          * with guest writes to its disk which may be in target backing chain.
+         * Can't resize during a backup block job because we check the size
+         * only upfront.
          */
-        *nshared = BLK_PERM_ALL;
+        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
         *nperm = BLK_PERM_WRITE;
     } else {
         /* Source child */
@@ -159,7 +161,7 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
         if (perm & BLK_PERM_WRITE) {
             *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
         }
-        *nshared &= ~BLK_PERM_WRITE;
+        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
     }
 }
 
@@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
 {
     Error *local_err = NULL;
     BDRVBackupTopState *state;
-    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
-                                                 filter_node_name,
-                                                 BDRV_O_RDWR, errp);
+    BlockDriverState *top;
     bool appended = false;
 
+    assert(source->total_sectors == target->total_sectors);
+
+    top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name,
+                               BDRV_O_RDWR, errp);
     if (!top) {
         return NULL;
     }
diff --git a/block/backup.c b/block/backup.c
index c4c3b8cd46..4f13bb20a5 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -340,7 +340,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                   BlockCompletionFunc *cb, void *opaque,
                   JobTxn *txn, Error **errp)
 {
-    int64_t len;
+    int64_t len, target_len;
     BackupBlockJob *job = NULL;
     int64_t cluster_size;
     BdrvRequestFlags write_flags;
@@ -405,6 +405,18 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
+    target_len = bdrv_getlength(target);
+    if (target_len < 0) {
+        error_setg_errno(errp, -target_len, "Unable to get length for '%s'",
+                         bdrv_get_device_or_node_name(bs));
+        goto error;
+    }
+
+    if (target_len != len) {
+        error_setg(errp, "Source and target image have different sizes");
+        goto error;
+    }
+
     cluster_size = backup_calculate_cluster_size(target, errp);
     if (cluster_size < 0) {
         goto error;
-- 
2.25.3

[PATCH v2 4/4] iotests: Backup with different source/target size

[Kevin Wolf]

This tests that the backup job catches situations where the target node
has a different size than the source node. It must also forbid resize
operations when the job is already running.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/055     | 42 ++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/055.out |  4 ++--
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 82b9f5f47d..7753c6577a 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,8 +48,10 @@ class TestSingleDrive(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
-        self.vm.add_drive(blockdev_target_img, interface="none")
+        self.vm = iotests.VM()
+        self.vm.add_drive('blkdebug::' + test_img, 'node-name=source')
+        self.vm.add_drive(blockdev_target_img, 'node-name=target',
+                          interface="none")
         if iotests.qemu_default_machine == 'pc':
             self.vm.add_drive(None, 'media=cdrom', 'ide')
         self.vm.launch()
@@ -112,6 +114,42 @@ class TestSingleDrive(iotests.QMPTestCase):
     def test_pause_blockdev_backup(self):
         self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
 
+    def do_test_resize_blockdev_backup(self, device, node):
+        def pre_finalize():
+            result = self.vm.qmp('block_resize', device=device, size=65536)
+            self.assert_qmp(result, 'error/class', 'GenericError')
+
+            result = self.vm.qmp('block_resize', node_name=node, size=65536)
+            self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
+                             target='drive1', sync='full', auto_finalize=False,
+                             auto_dismiss=False)
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
+                        use_log=False)
+
+    def test_source_resize_blockdev_backup(self):
+        self.do_test_resize_blockdev_backup('drive0', 'source')
+
+    def test_target_resize_blockdev_backup(self):
+        self.do_test_resize_blockdev_backup('drive1', 'target')
+
+    def do_test_target_size(self, size):
+        result = self.vm.qmp('block_resize', device='drive1', size=size)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
+                             target='drive1', sync='full')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_small_target(self):
+        self.do_test_target_size(image_len // 2)
+
+    def test_large_target(self):
+        self.do_test_target_size(image_len * 2)
+
     def test_medium_not_found(self):
         if iotests.qemu_default_machine != 'pc':
             return
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index 5ce2f9a2ed..88bf7fa73a 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-..............................
+..................................
 ----------------------------------------------------------------------
-Ran 30 tests
+Ran 34 tests
 
 OK
-- 
2.25.3

Re: [PATCH v2 1/4] iotests/283: Use consistent size for source and target

[Vladimir Sementsov-Ogievskiy]

30.04.2020 17:27, Kevin Wolf wrote:
> The test case forgot to specify the null-co size for the target node.
> When adding a check to backup that both sizes match, this would fail
> because of the size mismatch and not the behaviour that the test really
> wanted to test.
> 
> Fixes: a541fcc27c98b96da187c7d4573f3270f3ddd283
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 3/4] backup: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy]

30.04.2020 17:27, Kevin Wolf wrote:
> Since the introduction of a backup filter node in commit 00e30f05d, the
> backup block job crashes when the target image is smaller than the
> source image because it will try to write after the end of the target
> node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer
> would have caught this and errored out gracefully.)
> 
> We can fix this and even do better than the old behaviour: Check that
> source and target have the same image size at the start of the block job
> and unshare BLK_PERM_RESIZE. (This permission was already unshared
> before the same commit 00e30f05d, but the BlockBackend that was used to
> make the restriction was removed without a replacement.) This will
> immediately error out when starting the job instead of only when writing
> to a block that doesn't exist in the target.
> 
> Longer target than source would technically work because we would never
> write to blocks that don't exist, but semantically these are invalid,
> too, because a backup is supposed to create a copy, not just an image
> that starts with a copy.
> 
> Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I'm OK with it as is, as it fixes bug:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

still, some notes below


> ---
>   block/backup-top.c | 14 +++++++++-----
>   block/backup.c     | 14 +++++++++++++-
>   2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 3b50c06e2c..79b268e6dc 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>            *
>            * Share write to target (child_file), to not interfere
>            * with guest writes to its disk which may be in target backing chain.
> +         * Can't resize during a backup block job because we check the size
> +         * only upfront.
>            */
> -        *nshared = BLK_PERM_ALL;
> +        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
>           *nperm = BLK_PERM_WRITE;
>       } else {
>           /* Source child */
> @@ -159,7 +161,7 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>           if (perm & BLK_PERM_WRITE) {
>               *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
>           }
> -        *nshared &= ~BLK_PERM_WRITE;
> +        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>       }
>   }
>   
> @@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>   {
>       Error *local_err = NULL;
>       BDRVBackupTopState *state;
> -    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
> -                                                 filter_node_name,
> -                                                 BDRV_O_RDWR, errp);
> +    BlockDriverState *top;
>       bool appended = false;
>   
> +    assert(source->total_sectors == target->total_sectors);

May be better to error-out, just to keep backup-top independent. Still, now it's not
really needed, as we have only one caller. And this function have to be refactored
anyway, when publishing this filter (open() and close() should appear, so this code
will be rewritten anyway.)

And the other thought: the permissions we declared above, will be activated only after
successful bdrv_child_refresh_perms(). I think some kind of race is possible, so that
size is changed actual permission activation. So, may be good to double check sizes after
bdrv_child_refresh_perms().. But it's a kind of paranoia.

Also, third thought: the restricted permissions doesn't save us from resizing
of the source through exactly this node, does it? Hmm, but your test works somehow. But
(I assume) it worked in a previous patch version without unsharing on source..

Ha, but bdrv_co_truncate just can't work on backup-top, because it doesn't have file child.
But, if we fix bdrv_co_truncate to skip filters, we'll need to define .bdrv_co_truncate in
backup_top, which will return something like -EBUSY.. Or just -ENOTSUP, doesn't matter.

> +
> +    top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name,
> +                               BDRV_O_RDWR, errp);
>       if (!top) {
>           return NULL;
>       }
> diff --git a/block/backup.c b/block/backup.c
> index c4c3b8cd46..4f13bb20a5 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -340,7 +340,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>                     BlockCompletionFunc *cb, void *opaque,
>                     JobTxn *txn, Error **errp)
>   {
> -    int64_t len;
> +    int64_t len, target_len;
>       BackupBlockJob *job = NULL;
>       int64_t cluster_size;
>       BdrvRequestFlags write_flags;
> @@ -405,6 +405,18 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>           goto error;
>       }
>   
> +    target_len = bdrv_getlength(target);
> +    if (target_len < 0) {
> +        error_setg_errno(errp, -target_len, "Unable to get length for '%s'",
> +                         bdrv_get_device_or_node_name(bs));
> +        goto error;
> +    }
> +
> +    if (target_len != len) {
> +        error_setg(errp, "Source and target image have different sizes");
> +        goto error;
> +    }
> +
>       cluster_size = backup_calculate_cluster_size(target, errp);
>       if (cluster_size < 0) {
>           goto error;
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 4/4] iotests: Backup with different source/target size

[Vladimir Sementsov-Ogievskiy]

30.04.2020 17:27, Kevin Wolf wrote:
> This tests that the backup job catches situations where the target node
> has a different size than the source node. It must also forbid resize
> operations when the job is already running.
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Thanks!

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 3/4] backup: Make sure that source and target size match

[Kevin Wolf]

Am 30.04.2020 um 20:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 30.04.2020 17:27, Kevin Wolf wrote:
> > Since the introduction of a backup filter node in commit 00e30f05d, the
> > backup block job crashes when the target image is smaller than the
> > source image because it will try to write after the end of the target
> > node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer
> > would have caught this and errored out gracefully.)
> > 
> > We can fix this and even do better than the old behaviour: Check that
> > source and target have the same image size at the start of the block job
> > and unshare BLK_PERM_RESIZE. (This permission was already unshared
> > before the same commit 00e30f05d, but the BlockBackend that was used to
> > make the restriction was removed without a replacement.) This will
> > immediately error out when starting the job instead of only when writing
> > to a block that doesn't exist in the target.
> > 
> > Longer target than source would technically work because we would never
> > write to blocks that don't exist, but semantically these are invalid,
> > too, because a backup is supposed to create a copy, not just an image
> > that starts with a copy.
> > 
> > Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I'm OK with it as is, as it fixes bug:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> still, some notes below
> 
> 
> > ---
> >   block/backup-top.c | 14 +++++++++-----
> >   block/backup.c     | 14 +++++++++++++-
> >   2 files changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block/backup-top.c b/block/backup-top.c
> > index 3b50c06e2c..79b268e6dc 100644
> > --- a/block/backup-top.c
> > +++ b/block/backup-top.c
> > @@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
> >            *
> >            * Share write to target (child_file), to not interfere
> >            * with guest writes to its disk which may be in target backing chain.
> > +         * Can't resize during a backup block job because we check the size
> > +         * only upfront.
> >            */
> > -        *nshared = BLK_PERM_ALL;
> > +        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
> >           *nperm = BLK_PERM_WRITE;
> >       } else {
> >           /* Source child */
> > @@ -159,7 +161,7 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
> >           if (perm & BLK_PERM_WRITE) {
> >               *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> >           }
> > -        *nshared &= ~BLK_PERM_WRITE;
> > +        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> >       }
> >   }
> > @@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
> >   {
> >       Error *local_err = NULL;
> >       BDRVBackupTopState *state;
> > -    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
> > -                                                 filter_node_name,
> > -                                                 BDRV_O_RDWR, errp);
> > +    BlockDriverState *top;
> >       bool appended = false;
> > +    assert(source->total_sectors == target->total_sectors);
> 
> May be better to error-out, just to keep backup-top independent. Still, now it's not
> really needed, as we have only one caller. And this function have to be refactored
> anyway, when publishing this filter (open() and close() should appear, so this code
> will be rewritten anyway.)

Yes, the whole function only works because it's used in this restricted
context today. For example, we only know that total_sectors is up to
date because the caller has called bdrv_getlength() just a moment ago.

I think fixing this would be beyond the scope of this patch, but
certainly a good idea anyway.

> And the other thought: the permissions we declared above, will be activated only after
> successful bdrv_child_refresh_perms(). I think some kind of race is possible, so that
> size is changed actual permission activation. So, may be good to double check sizes after
> bdrv_child_refresh_perms().. But it's a kind of paranoia.

We're not in coroutine context, so we can't yield. I don't see who could
change the size in parallel (apart from an external process, but an
external process can mess up anything).

When we make backup-top an independent driver, instead of double
checking (what would you do on error?), maybe we could move the size
initialisation (then with bdrv_getlength()) to after
bdrv_child_refresh_perms().

> Also, third thought: the restricted permissions doesn't save us from resizing
> of the source through exactly this node, does it? Hmm, but your test works somehow. But
> (I assume) it worked in a previous patch version without unsharing on source..
> 
> Ha, but bdrv_co_truncate just can't work on backup-top, because it doesn't have file child.
> But, if we fix bdrv_co_truncate to skip filters, we'll need to define .bdrv_co_truncate in
> backup_top, which will return something like -EBUSY.. Or just -ENOTSUP, doesn't matter.

Maybe this is a sign that bdrv_co_truncate shouldn't automatically skip
filters because filters might depend on a fixed size?

Or we could make the automatic skipping depend on having BLK_PERM_RESIZE
for the child. If the filter doesn't have the permission, we must not
call truncate for its child (it would crash). Then backup-top and
similar filters must just be careful not to take RESIZE permissions.

Kevin

Re: [PATCH v2 0/4] backup: Make sure that source and target size match

[Kevin Wolf]

Am 30.04.2020 um 16:27 hat Kevin Wolf geschrieben:
> v2:
> - Fixed iotest 283
> - Corrected commit message for patch 3 [Vladimir]
> - Fixed permissions for the source node, too
> - Refactored the test case to avoid some duplication [Vladimir]

Thanks for the review, applied to the block branch.

Kevin

Re: [PATCH v2 3/4] backup: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy]

05.05.2020 13:03, Kevin Wolf wrote:
> Am 30.04.2020 um 20:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 30.04.2020 17:27, Kevin Wolf wrote:
>>> Since the introduction of a backup filter node in commit 00e30f05d, the
>>> backup block job crashes when the target image is smaller than the
>>> source image because it will try to write after the end of the target
>>> node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer
>>> would have caught this and errored out gracefully.)
>>>
>>> We can fix this and even do better than the old behaviour: Check that
>>> source and target have the same image size at the start of the block job
>>> and unshare BLK_PERM_RESIZE. (This permission was already unshared
>>> before the same commit 00e30f05d, but the BlockBackend that was used to
>>> make the restriction was removed without a replacement.) This will
>>> immediately error out when starting the job instead of only when writing
>>> to a block that doesn't exist in the target.
>>>
>>> Longer target than source would technically work because we would never
>>> write to blocks that don't exist, but semantically these are invalid,
>>> too, because a backup is supposed to create a copy, not just an image
>>> that starts with a copy.
>>>
>>> Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>
>> I'm OK with it as is, as it fixes bug:
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> still, some notes below
>>
>>
>>> ---
>>>    block/backup-top.c | 14 +++++++++-----
>>>    block/backup.c     | 14 +++++++++++++-
>>>    2 files changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/block/backup-top.c b/block/backup-top.c
>>> index 3b50c06e2c..79b268e6dc 100644
>>> --- a/block/backup-top.c
>>> +++ b/block/backup-top.c
>>> @@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>>>             *
>>>             * Share write to target (child_file), to not interfere
>>>             * with guest writes to its disk which may be in target backing chain.
>>> +         * Can't resize during a backup block job because we check the size
>>> +         * only upfront.
>>>             */
>>> -        *nshared = BLK_PERM_ALL;
>>> +        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
>>>            *nperm = BLK_PERM_WRITE;
>>>        } else {
>>>            /* Source child */
>>> @@ -159,7 +161,7 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>>>            if (perm & BLK_PERM_WRITE) {
>>>                *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
>>>            }
>>> -        *nshared &= ~BLK_PERM_WRITE;
>>> +        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>>>        }
>>>    }
>>> @@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>>    {
>>>        Error *local_err = NULL;
>>>        BDRVBackupTopState *state;
>>> -    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
>>> -                                                 filter_node_name,
>>> -                                                 BDRV_O_RDWR, errp);
>>> +    BlockDriverState *top;
>>>        bool appended = false;
>>> +    assert(source->total_sectors == target->total_sectors);
>>
>> May be better to error-out, just to keep backup-top independent. Still, now it's not
>> really needed, as we have only one caller. And this function have to be refactored
>> anyway, when publishing this filter (open() and close() should appear, so this code
>> will be rewritten anyway.)
> 
> Yes, the whole function only works because it's used in this restricted
> context today. For example, we only know that total_sectors is up to
> date because the caller has called bdrv_getlength() just a moment ago.
> 
> I think fixing this would be beyond the scope of this patch, but
> certainly a good idea anyway.
> 
>> And the other thought: the permissions we declared above, will be activated only after
>> successful bdrv_child_refresh_perms(). I think some kind of race is possible, so that
>> size is changed actual permission activation. So, may be good to double check sizes after
>> bdrv_child_refresh_perms().. But it's a kind of paranoia.
> 
> We're not in coroutine context, so we can't yield. I don't see who could
> change the size in parallel (apart from an external process, but an
> external process can mess up anything).
> 
> When we make backup-top an independent driver, instead of double
> checking (what would you do on error?), maybe we could move the size
> initialisation (then with bdrv_getlength()) to after
> bdrv_child_refresh_perms().
> 
>> Also, third thought: the restricted permissions doesn't save us from resizing
>> of the source through exactly this node, does it? Hmm, but your test works somehow. But
>> (I assume) it worked in a previous patch version without unsharing on source..
>>
>> Ha, but bdrv_co_truncate just can't work on backup-top, because it doesn't have file child.
>> But, if we fix bdrv_co_truncate to skip filters, we'll need to define .bdrv_co_truncate in
>> backup_top, which will return something like -EBUSY.. Or just -ENOTSUP, doesn't matter.
> 
> Maybe this is a sign that bdrv_co_truncate shouldn't automatically skip
> filters because filters might depend on a fixed size?
> 
> Or we could make the automatic skipping depend on having BLK_PERM_RESIZE
> for the child. If the filter doesn't have the permission, we must not
> call truncate for its child (it would crash). Then backup-top and
> similar filters must just be careful not to take RESIZE permissions.
> 

Hmm this should work.. Still it's a workaround, seems out of the concept of permission system..

I think, that the problem is that .bdrv_top_child_perm can't return an error.
The handler answers the question:

- Hi, we are your owners and we want the following cumulative permissions on you. Then, which permissions do you want on your child?

And the handler can't answer: "Hi, you guys want too much, I refuse to play by your rules"..


-- 
Best regards,
Vladimir

Re: [PATCH v2 3/4] backup: Make sure that source and target size match

[Kevin Wolf]

Am 06.05.2020 um 08:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 05.05.2020 13:03, Kevin Wolf wrote:
> > Am 30.04.2020 um 20:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 30.04.2020 17:27, Kevin Wolf wrote:
> > > > Since the introduction of a backup filter node in commit 00e30f05d, the
> > > > backup block job crashes when the target image is smaller than the
> > > > source image because it will try to write after the end of the target
> > > > node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer
> > > > would have caught this and errored out gracefully.)
> > > > 
> > > > We can fix this and even do better than the old behaviour: Check that
> > > > source and target have the same image size at the start of the block job
> > > > and unshare BLK_PERM_RESIZE. (This permission was already unshared
> > > > before the same commit 00e30f05d, but the BlockBackend that was used to
> > > > make the restriction was removed without a replacement.) This will
> > > > immediately error out when starting the job instead of only when writing
> > > > to a block that doesn't exist in the target.
> > > > 
> > > > Longer target than source would technically work because we would never
> > > > write to blocks that don't exist, but semantically these are invalid,
> > > > too, because a backup is supposed to create a copy, not just an image
> > > > that starts with a copy.
> > > > 
> > > > Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
> > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
> > > > Cc: qemu-stable@nongnu.org
> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > 
> > > I'm OK with it as is, as it fixes bug:
> > > 
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > 
> > > still, some notes below
> > > 
> > > 
> > > > ---
> > > >    block/backup-top.c | 14 +++++++++-----
> > > >    block/backup.c     | 14 +++++++++++++-
> > > >    2 files changed, 22 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/block/backup-top.c b/block/backup-top.c
> > > > index 3b50c06e2c..79b268e6dc 100644
> > > > --- a/block/backup-top.c
> > > > +++ b/block/backup-top.c
> > > > @@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
> > > >             *
> > > >             * Share write to target (child_file), to not interfere
> > > >             * with guest writes to its disk which may be in target backing chain.
> > > > +         * Can't resize during a backup block job because we check the size
> > > > +         * only upfront.
> > > >             */
> > > > -        *nshared = BLK_PERM_ALL;
> > > > +        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
> > > >            *nperm = BLK_PERM_WRITE;
> > > >        } else {
> > > >            /* Source child */
> > > > @@ -159,7 +161,7 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
> > > >            if (perm & BLK_PERM_WRITE) {
> > > >                *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> > > >            }
> > > > -        *nshared &= ~BLK_PERM_WRITE;
> > > > +        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > > >        }
> > > >    }
> > > > @@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
> > > >    {
> > > >        Error *local_err = NULL;
> > > >        BDRVBackupTopState *state;
> > > > -    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
> > > > -                                                 filter_node_name,
> > > > -                                                 BDRV_O_RDWR, errp);
> > > > +    BlockDriverState *top;
> > > >        bool appended = false;
> > > > +    assert(source->total_sectors == target->total_sectors);
> > > 
> > > May be better to error-out, just to keep backup-top independent. Still, now it's not
> > > really needed, as we have only one caller. And this function have to be refactored
> > > anyway, when publishing this filter (open() and close() should appear, so this code
> > > will be rewritten anyway.)
> > 
> > Yes, the whole function only works because it's used in this restricted
> > context today. For example, we only know that total_sectors is up to
> > date because the caller has called bdrv_getlength() just a moment ago.
> > 
> > I think fixing this would be beyond the scope of this patch, but
> > certainly a good idea anyway.
> > 
> > > And the other thought: the permissions we declared above, will be activated only after
> > > successful bdrv_child_refresh_perms(). I think some kind of race is possible, so that
> > > size is changed actual permission activation. So, may be good to double check sizes after
> > > bdrv_child_refresh_perms().. But it's a kind of paranoia.
> > 
> > We're not in coroutine context, so we can't yield. I don't see who could
> > change the size in parallel (apart from an external process, but an
> > external process can mess up anything).
> > 
> > When we make backup-top an independent driver, instead of double
> > checking (what would you do on error?), maybe we could move the size
> > initialisation (then with bdrv_getlength()) to after
> > bdrv_child_refresh_perms().
> > 
> > > Also, third thought: the restricted permissions doesn't save us from resizing
> > > of the source through exactly this node, does it? Hmm, but your test works somehow. But
> > > (I assume) it worked in a previous patch version without unsharing on source..
> > > 
> > > Ha, but bdrv_co_truncate just can't work on backup-top, because it doesn't have file child.
> > > But, if we fix bdrv_co_truncate to skip filters, we'll need to define .bdrv_co_truncate in
> > > backup_top, which will return something like -EBUSY.. Or just -ENOTSUP, doesn't matter.
> > 
> > Maybe this is a sign that bdrv_co_truncate shouldn't automatically skip
> > filters because filters might depend on a fixed size?
> > 
> > Or we could make the automatic skipping depend on having BLK_PERM_RESIZE
> > for the child. If the filter doesn't have the permission, we must not
> > call truncate for its child (it would crash). Then backup-top and
> > similar filters must just be careful not to take RESIZE permissions.
> > 
> 
> Hmm this should work.. Still it's a workaround, seems out of the
> concept of permission system..

I'm not sure about this. I see the problem more with unconditionally
skipping the filter without checking whether the operation is even
allowed on the filtered child.

> I think, that the problem is that .bdrv_top_child_perm can't return an
> error.  The handler answers the question:
> 
> - Hi, we are your owners and we want the following cumulative
> permissions on you. Then, which permissions do you want on your child?
> 
> And the handler can't answer: "Hi, you guys want too much, I refuse to
> play by your rules"..

It can implement .bdrv_check_perm to do that. It's just a bit more work
for each driver to do that.

Kevin

Re: [PATCH v2 3/4] backup: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy]

06.05.2020 11:02, Kevin Wolf wrote:
> Am 06.05.2020 um 08:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 05.05.2020 13:03, Kevin Wolf wrote:
>>> Am 30.04.2020 um 20:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 30.04.2020 17:27, Kevin Wolf wrote:
>>>>> Since the introduction of a backup filter node in commit 00e30f05d, the
>>>>> backup block job crashes when the target image is smaller than the
>>>>> source image because it will try to write after the end of the target
>>>>> node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer
>>>>> would have caught this and errored out gracefully.)
>>>>>
>>>>> We can fix this and even do better than the old behaviour: Check that
>>>>> source and target have the same image size at the start of the block job
>>>>> and unshare BLK_PERM_RESIZE. (This permission was already unshared
>>>>> before the same commit 00e30f05d, but the BlockBackend that was used to
>>>>> make the restriction was removed without a replacement.) This will
>>>>> immediately error out when starting the job instead of only when writing
>>>>> to a block that doesn't exist in the target.
>>>>>
>>>>> Longer target than source would technically work because we would never
>>>>> write to blocks that don't exist, but semantically these are invalid,
>>>>> too, because a backup is supposed to create a copy, not just an image
>>>>> that starts with a copy.
>>>>>
>>>>> Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
>>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
>>>>> Cc: qemu-stable@nongnu.org
>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>
>>>> I'm OK with it as is, as it fixes bug:
>>>>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> still, some notes below
>>>>
>>>>
>>>>> ---
>>>>>     block/backup-top.c | 14 +++++++++-----
>>>>>     block/backup.c     | 14 +++++++++++++-
>>>>>     2 files changed, 22 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/block/backup-top.c b/block/backup-top.c
>>>>> index 3b50c06e2c..79b268e6dc 100644
>>>>> --- a/block/backup-top.c
>>>>> +++ b/block/backup-top.c
>>>>> @@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>>>>>              *
>>>>>              * Share write to target (child_file), to not interfere
>>>>>              * with guest writes to its disk which may be in target backing chain.
>>>>> +         * Can't resize during a backup block job because we check the size
>>>>> +         * only upfront.
>>>>>              */
>>>>> -        *nshared = BLK_PERM_ALL;
>>>>> +        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
>>>>>             *nperm = BLK_PERM_WRITE;
>>>>>         } else {
>>>>>             /* Source child */
>>>>> @@ -159,7 +161,7 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>>>>>             if (perm & BLK_PERM_WRITE) {
>>>>>                 *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
>>>>>             }
>>>>> -        *nshared &= ~BLK_PERM_WRITE;
>>>>> +        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>>>>>         }
>>>>>     }
>>>>> @@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>>>>     {
>>>>>         Error *local_err = NULL;
>>>>>         BDRVBackupTopState *state;
>>>>> -    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
>>>>> -                                                 filter_node_name,
>>>>> -                                                 BDRV_O_RDWR, errp);
>>>>> +    BlockDriverState *top;
>>>>>         bool appended = false;
>>>>> +    assert(source->total_sectors == target->total_sectors);
>>>>
>>>> May be better to error-out, just to keep backup-top independent. Still, now it's not
>>>> really needed, as we have only one caller. And this function have to be refactored
>>>> anyway, when publishing this filter (open() and close() should appear, so this code
>>>> will be rewritten anyway.)
>>>
>>> Yes, the whole function only works because it's used in this restricted
>>> context today. For example, we only know that total_sectors is up to
>>> date because the caller has called bdrv_getlength() just a moment ago.
>>>
>>> I think fixing this would be beyond the scope of this patch, but
>>> certainly a good idea anyway.
>>>
>>>> And the other thought: the permissions we declared above, will be activated only after
>>>> successful bdrv_child_refresh_perms(). I think some kind of race is possible, so that
>>>> size is changed actual permission activation. So, may be good to double check sizes after
>>>> bdrv_child_refresh_perms().. But it's a kind of paranoia.
>>>
>>> We're not in coroutine context, so we can't yield. I don't see who could
>>> change the size in parallel (apart from an external process, but an
>>> external process can mess up anything).
>>>
>>> When we make backup-top an independent driver, instead of double
>>> checking (what would you do on error?), maybe we could move the size
>>> initialisation (then with bdrv_getlength()) to after
>>> bdrv_child_refresh_perms().
>>>
>>>> Also, third thought: the restricted permissions doesn't save us from resizing
>>>> of the source through exactly this node, does it? Hmm, but your test works somehow. But
>>>> (I assume) it worked in a previous patch version without unsharing on source..
>>>>
>>>> Ha, but bdrv_co_truncate just can't work on backup-top, because it doesn't have file child.
>>>> But, if we fix bdrv_co_truncate to skip filters, we'll need to define .bdrv_co_truncate in
>>>> backup_top, which will return something like -EBUSY.. Or just -ENOTSUP, doesn't matter.
>>>
>>> Maybe this is a sign that bdrv_co_truncate shouldn't automatically skip
>>> filters because filters might depend on a fixed size?
>>>
>>> Or we could make the automatic skipping depend on having BLK_PERM_RESIZE
>>> for the child. If the filter doesn't have the permission, we must not
>>> call truncate for its child (it would crash). Then backup-top and
>>> similar filters must just be careful not to take RESIZE permissions.
>>>
>>
>> Hmm this should work.. Still it's a workaround, seems out of the
>> concept of permission system..
> 
> I'm not sure about this. I see the problem more with unconditionally
> skipping the filter without checking whether the operation is even
> allowed on the filtered child.

Agree. Blindly skipping the filter is wrong anyway.

> 
>> I think, that the problem is that .bdrv_top_child_perm can't return an
>> error.  The handler answers the question:
>>
>> - Hi, we are your owners and we want the following cumulative
>> permissions on you. Then, which permissions do you want on your child?
>>
>> And the handler can't answer: "Hi, you guys want too much, I refuse to
>> play by your rules"..
> 
> It can implement .bdrv_check_perm to do that. It's just a bit more work
> for each driver to do that.
> 

OK, so, it actually can.


-- 
Best regards,
Vladimir