[PATCH 0/2] block-backend: Retain permissions after migration

[PATCH 0/2] block-backend: Retain permissions after migration

[Hanna Reitz]

Hi,

Peng Liang has reported an issue regarding migration of raw images here:
https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00673.html

It turns out that after migrating, all permissions are shared when they
weren’t before.  The cause of the problem is that we deliberately delay
restricting the shared permissions until migration is really done (until
the runstate is no longer INMIGRATE) and first share all permissions;
but this causes us to lose the original shared permission mask and
overwrites it with BLK_PERM_ALL, so once we do try to restrict the
shared permissions, we only again share them all.

Fix this by saving the set of shared permissions through the first
blk_perm_set() call that shares all; and add a regression test.


I don’t believe we have to fix this in 6.2, because I think this bug has
existed for four years now.  (I.e. it isn’t critical, and it’s no
regression.)


Hanna Reitz (2):
  block-backend: Retain permissions after migration
  iotests/migration-permissions: New test

 block/block-backend.c                         |  11 ++
 .../qemu-iotests/tests/migration-permissions  | 101 ++++++++++++++++++
 .../tests/migration-permissions.out           |   5 +
 3 files changed, 117 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/migration-permissions
 create mode 100644 tests/qemu-iotests/tests/migration-permissions.out

-- 
2.33.1

[PATCH 1/2] block-backend: Retain permissions after migration

[Hanna Reitz]

After migration, the permissions the guest device wants to impose on its
BlockBackend are stored in blk->perm and blk->shared_perm.  In
blk_root_activate(), we take our permissions, but keep all shared
permissions open by calling `blk_set_perm(blk->perm, BLK_PERM_ALL)`.

Only afterwards (immediately or later, depending on the runstate) do we
restrict the shared permissions by calling
`blk_set_perm(blk->perm, blk->shared_perm)`.  Unfortunately, our first
call with shared_perm=BLK_PERM_ALL has overwritten blk->shared_perm to
be BLK_PERM_ALL, so this is a no-op and the set of shared permissions is
not restricted.

Fix this bug by saving the set of shared permissions before invoking
blk_set_perm() with BLK_PERM_ALL and restoring it afterwards.

Fixes: 5f7772c4d0cf32f4e779fcd5a69ae4dae24aeebf
       ("block-backend: Defer shared_perm tightening migration
       completion")
Reported-by: Peng Liang <liangpeng10@huawei.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/block-backend.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 12ef80ea17..41e388fe1f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -190,6 +190,7 @@ static void blk_root_activate(BdrvChild *child, Error **errp)
 {
     BlockBackend *blk = child->opaque;
     Error *local_err = NULL;
+    uint64_t saved_shared_perm;
 
     if (!blk->disable_perm) {
         return;
@@ -197,12 +198,22 @@ static void blk_root_activate(BdrvChild *child, Error **errp)
 
     blk->disable_perm = false;
 
+    /*
+     * blk->shared_perm contains the permissions we want to share once
+     * migration is really completely done.  For now, we need to share
+     * all; but we also need to retain blk->shared_perm, which is
+     * overwritten by a successful blk_set_perm() call.  Save it and
+     * restore it below.
+     */
+    saved_shared_perm = blk->shared_perm;
+
     blk_set_perm(blk, blk->perm, BLK_PERM_ALL, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         blk->disable_perm = true;
         return;
     }
+    blk->shared_perm = saved_shared_perm;
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         /* Activation can happen when migration process is still active, for
-- 
2.33.1

[PATCH 2/2] iotests/migration-permissions: New test

[Hanna Reitz]

This test checks that a raw image in use by a virtio-blk device does not
share the WRITE permission both before and after migration.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 .../qemu-iotests/tests/migration-permissions  | 101 ++++++++++++++++++
 .../tests/migration-permissions.out           |   5 +
 2 files changed, 106 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/migration-permissions
 create mode 100644 tests/qemu-iotests/tests/migration-permissions.out

diff --git a/tests/qemu-iotests/tests/migration-permissions b/tests/qemu-iotests/tests/migration-permissions
new file mode 100755
index 0000000000..6be02581c7
--- /dev/null
+++ b/tests/qemu-iotests/tests/migration-permissions
@@ -0,0 +1,101 @@
+#!/usr/bin/env python3
+# group: migration
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import imgfmt, qemu_img_create, qemu_io
+
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+mig_sock = os.path.join(iotests.sock_dir, 'mig.sock')
+
+
+class TestMigrationPermissions(iotests.QMPTestCase):
+    def setUp(self):
+        qemu_img_create('-f', imgfmt, test_img, '1M')
+
+        # Set up two VMs (source and destination) accessing the same raw
+        # image file with a virtio-blk device; prepare the destination for
+        # migration with .add_incoming() and enable migration events
+        vms = [None, None]
+        for i in range(2):
+            vms[i] = iotests.VM(path_suffix=f'{i}')
+            vms[i].add_blockdev(f'file,node-name=prot,filename={test_img}')
+            vms[i].add_blockdev(f'{imgfmt},node-name=fmt,file=prot')
+            vms[i].add_device('virtio-blk,drive=fmt')
+
+            if i == 1:
+                vms[i].add_incoming(f'unix:{mig_sock}')
+
+            vms[i].launch()
+
+            result = vms[i].qmp('migrate-set-capabilities',
+                                capabilities=[
+                                    {'capability': 'events', 'state': True}
+                                ])
+            self.assert_qmp(result, 'return', {})
+
+        self.vm_s = vms[0]
+        self.vm_d = vms[1]
+
+    def tearDown(self):
+        self.vm_s.shutdown()
+        self.vm_d.shutdown()
+        try:
+            os.remove(mig_sock)
+        except FileNotFoundError:
+            pass
+        os.remove(test_img)
+
+    # Migrate an image in use by a virtio-blk device to another VM and
+    # verify that the WRITE permission is unshared both before and after
+    # migration
+    def test_post_migration_permissions(self):
+        # Try to access the image R/W, which should fail because virtio-blk
+        # has not been configured with share-rw=on
+        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+        if not log.strip():
+            print('ERROR (pre-migration): qemu-io should not be able to '
+                  'access this image, but it reported no error')
+        else:
+            # This is the expected output
+            assert 'Is another process using the image' in log
+
+        # Now migrate the VM
+        self.vm_s.qmp('migrate', uri=f'unix:{mig_sock}')
+        assert self.vm_s.wait_migration(None)
+        assert self.vm_d.wait_migration(None)
+
+        # Try the same qemu-io access again, verifying that the WRITE
+        # permission remains unshared
+        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+        if not log.strip():
+            print('ERROR (post-migration): qemu-io should not be able to '
+                  'access this image, but it reported no error')
+        else:
+            # This is the expected output
+            assert 'Is another process using the image' in log
+
+
+if __name__ == '__main__':
+    # Only works with raw images because we are testing the
+    # BlockBackend permissions; image format drivers may additionally
+    # unshare permissions and thus tamper with the result
+    iotests.main(supported_fmts=['raw'],
+                 supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/migration-permissions.out b/tests/qemu-iotests/tests/migration-permissions.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/migration-permissions.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
-- 
2.33.1

Re: [PATCH 1/2] block-backend: Retain permissions after migration

[Philippe Mathieu-Daudé]

On 11/25/21 14:53, Hanna Reitz wrote:
> After migration, the permissions the guest device wants to impose on its
> BlockBackend are stored in blk->perm and blk->shared_perm.  In
> blk_root_activate(), we take our permissions, but keep all shared
> permissions open by calling `blk_set_perm(blk->perm, BLK_PERM_ALL)`.
> 
> Only afterwards (immediately or later, depending on the runstate) do we
> restrict the shared permissions by calling
> `blk_set_perm(blk->perm, blk->shared_perm)`.  Unfortunately, our first
> call with shared_perm=BLK_PERM_ALL has overwritten blk->shared_perm to
> be BLK_PERM_ALL, so this is a no-op and the set of shared permissions is
> not restricted.
> 
> Fix this bug by saving the set of shared permissions before invoking
> blk_set_perm() with BLK_PERM_ALL and restoring it afterwards.
> 
> Fixes: 5f7772c4d0cf32f4e779fcd5a69ae4dae24aeebf
>        ("block-backend: Defer shared_perm tightening migration
>        completion")
> Reported-by: Peng Liang <liangpeng10@huawei.com>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  block/block-backend.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/2] block-backend: Retain permissions after migration

[Peng Liang via]

On 11/25/2021 9:53 PM, Hanna Reitz wrote:
> After migration, the permissions the guest device wants to impose on its
> BlockBackend are stored in blk->perm and blk->shared_perm.  In
> blk_root_activate(), we take our permissions, but keep all shared
> permissions open by calling `blk_set_perm(blk->perm, BLK_PERM_ALL)`.
> 
> Only afterwards (immediately or later, depending on the runstate) do we
> restrict the shared permissions by calling
> `blk_set_perm(blk->perm, blk->shared_perm)`.  Unfortunately, our first
> call with shared_perm=BLK_PERM_ALL has overwritten blk->shared_perm to
> be BLK_PERM_ALL, so this is a no-op and the set of shared permissions is
> not restricted.
> 
> Fix this bug by saving the set of shared permissions before invoking
> blk_set_perm() with BLK_PERM_ALL and restoring it afterwards.
> 
> Fixes: 5f7772c4d0cf32f4e779fcd5a69ae4dae24aeebf
>        ("block-backend: Defer shared_perm tightening migration
>        completion")
> Reported-by: Peng Liang <liangpeng10@huawei.com>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  block/block-backend.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

Thanks for your patch!

Tested-by: Peng Liang <liangpeng10@huawei.com>

Re: [PATCH 0/2] block-backend: Retain permissions after migration

[Kevin Wolf]

Am 25.11.2021 um 14:53 hat Hanna Reitz geschrieben:
> Hi,
> 
> Peng Liang has reported an issue regarding migration of raw images here:
> https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00673.html
> 
> It turns out that after migrating, all permissions are shared when they
> weren’t before.  The cause of the problem is that we deliberately delay
> restricting the shared permissions until migration is really done (until
> the runstate is no longer INMIGRATE) and first share all permissions;
> but this causes us to lose the original shared permission mask and
> overwrites it with BLK_PERM_ALL, so once we do try to restrict the
> shared permissions, we only again share them all.
> 
> Fix this by saving the set of shared permissions through the first
> blk_perm_set() call that shares all; and add a regression test.
> 
> 
> I don’t believe we have to fix this in 6.2, because I think this bug has
> existed for four years now.  (I.e. it isn’t critical, and it’s no
> regression.)

Feels a bit like a hack, but I guess as long as it works... :-)

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 0/2] block-backend: Retain permissions after migration

[Peng Liang via]

On 11/25/2021 9:53 PM, Hanna Reitz wrote:
> Hi,
> 
> Peng Liang has reported an issue regarding migration of raw images here:
> https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00673.html
> 
> It turns out that after migrating, all permissions are shared when they
> weren’t before.  The cause of the problem is that we deliberately delay
> restricting the shared permissions until migration is really done (until
> the runstate is no longer INMIGRATE) and first share all permissions;
> but this causes us to lose the original shared permission mask and
> overwrites it with BLK_PERM_ALL, so once we do try to restrict the
> shared permissions, we only again share them all.
> 
> Fix this by saving the set of shared permissions through the first
> blk_perm_set() call that shares all; and add a regression test.
> 
> 
> I don’t believe we have to fix this in 6.2, because I think this bug has
> existed for four years now.  (I.e. it isn’t critical, and it’s no
> regression.)
> 
> 
> Hanna Reitz (2):
>   block-backend: Retain permissions after migration
>   iotests/migration-permissions: New test
> 
>  block/block-backend.c                         |  11 ++
>  .../qemu-iotests/tests/migration-permissions  | 101 ++++++++++++++++++
>  .../tests/migration-permissions.out           |   5 +
>  3 files changed, 117 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/migration-permissions
>  create mode 100644 tests/qemu-iotests/tests/migration-permissions.out
> 

Hi Hanna,
QEMU 6.3 development tree has been opened.  Will this fix be merged in 6.3?

Thanks,
Peng

Re: [PATCH 0/2] block-backend: Retain permissions after migration

[Hanna Reitz]

On 25.11.21 14:53, Hanna Reitz wrote:
> Hi,
>
> Peng Liang has reported an issue regarding migration of raw images here:
> https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00673.html
>
> It turns out that after migrating, all permissions are shared when they
> weren’t before.  The cause of the problem is that we deliberately delay
> restricting the shared permissions until migration is really done (until
> the runstate is no longer INMIGRATE) and first share all permissions;
> but this causes us to lose the original shared permission mask and
> overwrites it with BLK_PERM_ALL, so once we do try to restrict the
> shared permissions, we only again share them all.
>
> Fix this by saving the set of shared permissions through the first
> blk_perm_set() call that shares all; and add a regression test.
>
>
> I don’t believe we have to fix this in 6.2, because I think this bug has
> existed for four years now.  (I.e. it isn’t critical, and it’s no
> regression.)

Thanks for the reviews, applied to my block branch:

https://gitlab.com/hreitz/qemu/-/commits/block

Hanna

Re: [PATCH 0/2] block-backend: Retain permissions after migration

[Hanna Reitz]

On 10.01.22 12:51, Peng Liang wrote:
> On 11/25/2021 9:53 PM, Hanna Reitz wrote:
>> Hi,
>>
>> Peng Liang has reported an issue regarding migration of raw images here:
>> https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00673.html
>>
>> It turns out that after migrating, all permissions are shared when they
>> weren’t before.  The cause of the problem is that we deliberately delay
>> restricting the shared permissions until migration is really done (until
>> the runstate is no longer INMIGRATE) and first share all permissions;
>> but this causes us to lose the original shared permission mask and
>> overwrites it with BLK_PERM_ALL, so once we do try to restrict the
>> shared permissions, we only again share them all.
>>
>> Fix this by saving the set of shared permissions through the first
>> blk_perm_set() call that shares all; and add a regression test.
>>
>>
>> I don’t believe we have to fix this in 6.2, because I think this bug has
>> existed for four years now.  (I.e. it isn’t critical, and it’s no
>> regression.)
>>
>>
>> Hanna Reitz (2):
>>    block-backend: Retain permissions after migration
>>    iotests/migration-permissions: New test
>>
>>   block/block-backend.c                         |  11 ++
>>   .../qemu-iotests/tests/migration-permissions  | 101 ++++++++++++++++++
>>   .../tests/migration-permissions.out           |   5 +
>>   3 files changed, 117 insertions(+)
>>   create mode 100755 tests/qemu-iotests/tests/migration-permissions
>>   create mode 100644 tests/qemu-iotests/tests/migration-permissions.out
>>
> Hi Hanna,
> QEMU 6.3 development tree has been opened.  Will this fix be merged in 6.3?

Oh, yes, right.  Thanks for the reminder! :)

Hanna