| Message ID | 20210331122815.51491-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Test mirror-top filter permissions | expand |
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>
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>
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
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
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