tests/qemu-iotests/tests/mirror-top-perms | 121 ++++++++++++++++++ tests/qemu-iotests/tests/mirror-top-perms.out | 5 + 2 files changed, 126 insertions(+) create mode 100755 tests/qemu-iotests/tests/mirror-top-perms create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out
Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11
("block/mirror: Fix mirror_top's permissions").
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/tests/mirror-top-perms | 121 ++++++++++++++++++
tests/qemu-iotests/tests/mirror-top-perms.out | 5 +
2 files changed, 126 insertions(+)
create mode 100755 tests/qemu-iotests/tests/mirror-top-perms
create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out
diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
new file mode 100755
index 0000000000..451a0666f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -0,0 +1,121 @@
+#!/usr/bin/env python3
+# group: rw
+#
+# Test permissions taken by the mirror-top filter
+#
+# 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 qemu_img
+
+# Import qemu after iotests.py has amended sys.path
+# pylint: disable=wrong-import-order
+import qemu
+
+
+image_size = 1 * 1024 * 1024
+source = os.path.join(iotests.test_dir, 'source.img')
+
+
+class TestMirrorTopPerms(iotests.QMPTestCase):
+ def setUp(self):
+ assert qemu_img('create', '-f', iotests.imgfmt, source,
+ str(image_size)) == 0
+ self.vm = iotests.VM()
+ self.vm.add_drive(source)
+ self.vm.add_blockdev(f'null-co,node-name=null,size={image_size}')
+ self.vm.launch()
+
+ # Will be created by the test function itself
+ self.vm_b = None
+
+ def tearDown(self):
+ try:
+ self.vm.shutdown()
+ except qemu.machine.AbnormalShutdown:
+ pass
+
+ if self.vm_b is not None:
+ self.vm_b.shutdown()
+
+ os.remove(source)
+
+ def test_cancel(self):
+ """
+ Before commit 53431b9086b28, mirror-top used to not take any
+ permissions but WRITE and share all permissions. Because it
+ is inserted between the source's original parents and the
+ source, there generally was no parent that would have taken or
+ unshared any permissions on the source, which means that an
+ external process could access the image unhindered by locks.
+ (Unless there was a parent above the protocol node that would
+ take its own locks, e.g. a format driver.)
+ This is bad enough, but if the mirror job is then cancelled,
+ the mirroring VM tries to take back the image, restores the
+ original permissions taken and unshared, and assumes this must
+ just work. But it will not, and so the VM aborts.
+
+ Commit 53431b9086b28 made mirror keep the original permissions
+ and so no other process can "steal" the image.
+
+ (Note that you cannot really do the same with the target image
+ and then completing the job, because the mirror job always
+ took/unshared the correct permissions on the target. For
+ example, it does not share READ_CONSISTENT, which makes it
+ difficult to let some other qemu process open the image.)
+ """
+
+ result = self.vm.qmp('blockdev-mirror',
+ job_id='mirror',
+ device='drive0',
+ target='null',
+ sync='full')
+ self.assert_qmp(result, 'return', {})
+
+ self.vm.event_wait('BLOCK_JOB_READY')
+
+ # We want this to fail because the image cannot be locked.
+ # If it does not fail, continue still and see what happens.
+ self.vm_b = iotests.VM(path_suffix='b')
+ # Must use -blockdev -device so we can use share-rw.
+ # (And we need share-rw=on because mirror-top was always
+ # forced to take the WRITE permission so it can write to the
+ # source image.)
+ self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
+ self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
+ try:
+ self.vm_b.launch()
+ print('ERROR: VM B launched successfully, this should not have '
+ 'happened')
+ except qemu.qmp.QMPConnectError:
+ assert 'Is another process using the image' in self.vm_b.get_log()
+
+ result = self.vm.qmp('block-job-cancel',
+ device='mirror')
+ self.assert_qmp(result, 'return', {})
+
+ self.vm.event_wait('BLOCK_JOB_COMPLETED')
+
+
+if __name__ == '__main__':
+ # No metadata format driver supported, because they would for
+ # example always unshare the WRITE permission. The raw driver
+ # just passes through the permissions from the guest device, and
+ # those are the permissions that we want to test.
+ iotests.main(supported_fmts=['raw'],
+ supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/mirror-top-perms.out b/tests/qemu-iotests/tests/mirror-top-perms.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/mirror-top-perms.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
--
2.29.2
On 3/31/21 7:28 AM, Max Reitz wrote: > Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11 > ("block/mirror: Fix mirror_top's permissions"). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/tests/mirror-top-perms | 121 ++++++++++++++++++ > tests/qemu-iotests/tests/mirror-top-perms.out | 5 + > 2 files changed, 126 insertions(+) > create mode 100755 tests/qemu-iotests/tests/mirror-top-perms > create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out Safe for -rc2 if you want to get it in. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
31.03.2021 15:28, Max Reitz wrote: > Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11 > ("block/mirror: Fix mirror_top's permissions"). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/tests/mirror-top-perms | 121 ++++++++++++++++++ > tests/qemu-iotests/tests/mirror-top-perms.out | 5 + > 2 files changed, 126 insertions(+) > create mode 100755 tests/qemu-iotests/tests/mirror-top-perms > create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out > > diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms > new file mode 100755 > index 0000000000..451a0666f8 > --- /dev/null > +++ b/tests/qemu-iotests/tests/mirror-top-perms > @@ -0,0 +1,121 @@ > +#!/usr/bin/env python3 > +# group: rw > +# > +# Test permissions taken by the mirror-top filter > +# > +# 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 qemu_img > + > +# Import qemu after iotests.py has amended sys.path > +# pylint: disable=wrong-import-order > +import qemu > + > + > +image_size = 1 * 1024 * 1024 > +source = os.path.join(iotests.test_dir, 'source.img') > + > + > +class TestMirrorTopPerms(iotests.QMPTestCase): > + def setUp(self): > + assert qemu_img('create', '-f', iotests.imgfmt, source, > + str(image_size)) == 0 > + self.vm = iotests.VM() > + self.vm.add_drive(source) > + self.vm.add_blockdev(f'null-co,node-name=null,size={image_size}') > + self.vm.launch() > + > + # Will be created by the test function itself > + self.vm_b = None > + > + def tearDown(self): > + try: > + self.vm.shutdown() > + except qemu.machine.AbnormalShutdown: > + pass > + > + if self.vm_b is not None: > + self.vm_b.shutdown() > + > + os.remove(source) > + > + def test_cancel(self): > + """ > + Before commit 53431b9086b28, mirror-top used to not take any > + permissions but WRITE and share all permissions. Because it > + is inserted between the source's original parents and the > + source, there generally was no parent that would have taken or > + unshared any permissions on the source, which means that an > + external process could access the image unhindered by locks. > + (Unless there was a parent above the protocol node that would > + take its own locks, e.g. a format driver.) > + This is bad enough, but if the mirror job is then cancelled, > + the mirroring VM tries to take back the image, restores the > + original permissions taken and unshared, and assumes this must > + just work. But it will not, and so the VM aborts. > + > + Commit 53431b9086b28 made mirror keep the original permissions > + and so no other process can "steal" the image. > + > + (Note that you cannot really do the same with the target image > + and then completing the job, because the mirror job always > + took/unshared the correct permissions on the target. For > + example, it does not share READ_CONSISTENT, which makes it > + difficult to let some other qemu process open the image.) > + """ > + > + result = self.vm.qmp('blockdev-mirror', > + job_id='mirror', > + device='drive0', > + target='null', > + sync='full') > + self.assert_qmp(result, 'return', {}) > + > + self.vm.event_wait('BLOCK_JOB_READY') > + > + # We want this to fail because the image cannot be locked. > + # If it does not fail, continue still and see what happens. This comment is about vm_b.launch(), not about creating vm object. Probably better to move it down > + self.vm_b = iotests.VM(path_suffix='b') > + # Must use -blockdev -device so we can use share-rw. > + # (And we need share-rw=on because mirror-top was always > + # forced to take the WRITE permission so it can write to the > + # source image.) > + self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}') > + self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on') > + try: > + self.vm_b.launch() > + print('ERROR: VM B launched successfully, this should not have ' > + 'happened') probably iotests.log() is better here. > + except qemu.qmp.QMPConnectError: > + assert 'Is another process using the image' in self.vm_b.get_log() > + > + result = self.vm.qmp('block-job-cancel', > + device='mirror') > + self.assert_qmp(result, 'return', {}) > + > + self.vm.event_wait('BLOCK_JOB_COMPLETED') > + > + > +if __name__ == '__main__': > + # No metadata format driver supported, because they would for > + # example always unshare the WRITE permission. The raw driver > + # just passes through the permissions from the guest device, and > + # those are the permissions that we want to test. > + iotests.main(supported_fmts=['raw'], > + supported_protocols=['file']) > diff --git a/tests/qemu-iotests/tests/mirror-top-perms.out b/tests/qemu-iotests/tests/mirror-top-perms.out > new file mode 100644 > index 0000000000..ae1213e6f8 > --- /dev/null > +++ b/tests/qemu-iotests/tests/mirror-top-perms.out > @@ -0,0 +1,5 @@ > +. > +---------------------------------------------------------------------- > +Ran 1 tests > + > +OK > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
On 01.04.21 10:32, Vladimir Sementsov-Ogievskiy wrote: > 31.03.2021 15:28, Max Reitz wrote: >> Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11 >> ("block/mirror: Fix mirror_top's permissions"). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/tests/mirror-top-perms | 121 ++++++++++++++++++ >> tests/qemu-iotests/tests/mirror-top-perms.out | 5 + >> 2 files changed, 126 insertions(+) >> create mode 100755 tests/qemu-iotests/tests/mirror-top-perms >> create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out >> >> diff --git a/tests/qemu-iotests/tests/mirror-top-perms >> b/tests/qemu-iotests/tests/mirror-top-perms >> new file mode 100755 >> index 0000000000..451a0666f8 >> --- /dev/null >> +++ b/tests/qemu-iotests/tests/mirror-top-perms >> @@ -0,0 +1,121 @@ >> +#!/usr/bin/env python3 >> +# group: rw >> +# >> +# Test permissions taken by the mirror-top filter >> +# >> +# 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 qemu_img >> + >> +# Import qemu after iotests.py has amended sys.path >> +# pylint: disable=wrong-import-order >> +import qemu >> + >> + >> +image_size = 1 * 1024 * 1024 >> +source = os.path.join(iotests.test_dir, 'source.img') >> + >> + >> +class TestMirrorTopPerms(iotests.QMPTestCase): >> + def setUp(self): >> + assert qemu_img('create', '-f', iotests.imgfmt, source, >> + str(image_size)) == 0 >> + self.vm = iotests.VM() >> + self.vm.add_drive(source) >> + >> self.vm.add_blockdev(f'null-co,node-name=null,size={image_size}') >> + self.vm.launch() >> + >> + # Will be created by the test function itself >> + self.vm_b = None >> + >> + def tearDown(self): >> + try: >> + self.vm.shutdown() >> + except qemu.machine.AbnormalShutdown: >> + pass >> + >> + if self.vm_b is not None: >> + self.vm_b.shutdown() >> + >> + os.remove(source) >> + >> + def test_cancel(self): >> + """ >> + Before commit 53431b9086b28, mirror-top used to not take any >> + permissions but WRITE and share all permissions. Because it >> + is inserted between the source's original parents and the >> + source, there generally was no parent that would have taken or >> + unshared any permissions on the source, which means that an >> + external process could access the image unhindered by locks. >> + (Unless there was a parent above the protocol node that would >> + take its own locks, e.g. a format driver.) >> + This is bad enough, but if the mirror job is then cancelled, >> + the mirroring VM tries to take back the image, restores the >> + original permissions taken and unshared, and assumes this must >> + just work. But it will not, and so the VM aborts. >> + >> + Commit 53431b9086b28 made mirror keep the original permissions >> + and so no other process can "steal" the image. >> + >> + (Note that you cannot really do the same with the target image >> + and then completing the job, because the mirror job always >> + took/unshared the correct permissions on the target. For >> + example, it does not share READ_CONSISTENT, which makes it >> + difficult to let some other qemu process open the image.) >> + """ >> + >> + result = self.vm.qmp('blockdev-mirror', >> + job_id='mirror', >> + device='drive0', >> + target='null', >> + sync='full') >> + self.assert_qmp(result, 'return', {}) >> + >> + self.vm.event_wait('BLOCK_JOB_READY') >> + >> + # We want this to fail because the image cannot be locked. >> + # If it does not fail, continue still and see what happens. > > This comment is about vm_b.launch(), not about creating vm object. > Probably better to move it down I kind of meant this as a general comment on what this VM is for. I think I kind of like any comment here, and I don’t see a practical advantage in having this comment above the try-except, because the whole “launch VM, see it fail” block is kind of one monolithic concept. >> + self.vm_b = iotests.VM(path_suffix='b') >> + # Must use -blockdev -device so we can use share-rw. >> + # (And we need share-rw=on because mirror-top was always >> + # forced to take the WRITE permission so it can write to the >> + # source image.) >> + >> self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}') >> + self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on') >> + try: >> + self.vm_b.launch() >> + print('ERROR: VM B launched successfully, this should not >> have ' >> + 'happened') > > probably iotests.log() is better here. No, because logging is disabled, and so it wouldn’t be visible. I’d need to enable logging, which would make the test quite different overall. Max >> + except qemu.qmp.QMPConnectError: >> + assert 'Is another process using the image' in >> self.vm_b.get_log() >> + >> + result = self.vm.qmp('block-job-cancel', >> + device='mirror') >> + self.assert_qmp(result, 'return', {}) >> + >> + self.vm.event_wait('BLOCK_JOB_COMPLETED') >> + >> + >> +if __name__ == '__main__': >> + # No metadata format driver supported, because they would for >> + # example always unshare the WRITE permission. The raw driver >> + # just passes through the permissions from the guest device, and >> + # those are the permissions that we want to test. >> + iotests.main(supported_fmts=['raw'], >> + supported_protocols=['file']) >> diff --git a/tests/qemu-iotests/tests/mirror-top-perms.out >> b/tests/qemu-iotests/tests/mirror-top-perms.out >> new file mode 100644 >> index 0000000000..ae1213e6f8 >> --- /dev/null >> +++ b/tests/qemu-iotests/tests/mirror-top-perms.out >> @@ -0,0 +1,5 @@ >> +. >> +---------------------------------------------------------------------- >> +Ran 1 tests >> + >> +OK >> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >
Am 31.03.2021 um 14:28 hat Max Reitz geschrieben: > Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11 > ("block/mirror: Fix mirror_top's permissions"). > > Signed-off-by: Max Reitz <mreitz@redhat.com> Thanks, applied to the block branch. Kevin
© 2016 - 2024 Red Hat, Inc.