On Thu, Feb 27, 2020 at 13:07:36 +0100, Michal Privoznik wrote: > When preparing images for block jobs we modify their seclabels so > that QEMU can open them. However, as mentioned in the previous > commit, secdrivers base some it their decisions whether the image > they are working on is top parent or not. Fortunately, in places top of the backing chain > where we call secdrivers we know this and the information can be > passed to secdrivers. > > This fixes the problem described in the linked bugzilla. The That's what patches usually do. Either state the problem or omit this sentece. > problem is the following: after the first blockcommit from the > base to one of the parents the XATTRs on the base image are not > cleared and therefore the second attempt to do another Oh you do state the problem. So just omit that sentence. > blockcommit fails. This is caused by blockcommit code calling > qemuSecuritySetImageLabel() over the base image and never calling > the corresponding qemuSecurityRestoreImageLabel(). A naive fix > would be to call the restore function. But this is not possible, > because that would deny QEMU the access to the base image. Well this kind of misleads. We want to modify the security label so that the VM has read-write or read-only access. The security label is then corrected by the call to another qemuSecuritySetImageLabel. You then correctly mention that using qemuSecurityRestoreImageLabel would cut the access to the image. > Fortunately, we can use the fact that seclabels are remembered > only for the top parent and not for the rest of the backing 'top of backing chain' > chain. And thanks to the previous commit we can tell secdrivers > which images are top parents. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1803551 > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > src/qemu/qemu_backup.c | 4 ++-- > src/qemu/qemu_blockjob.c | 6 ++++-- > src/qemu/qemu_checkpoint.c | 6 ++++-- > src/qemu/qemu_domain.c | 15 +++++++++++++-- > src/qemu/qemu_domain.h | 3 ++- > src/qemu/qemu_driver.c | 15 ++++++++++----- > src/qemu/qemu_process.c | 2 +- > src/qemu/qemu_security.c | 6 +++++- > src/qemu/qemu_security.h | 3 ++- > 9 files changed, 43 insertions(+), 17 deletions(-) [...] > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c > index 71df0d1ab2..9db1b71a3e 100644 > --- a/src/qemu/qemu_blockjob.c > +++ b/src/qemu/qemu_blockjob.c > @@ -1105,9 +1105,11 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver, > return; > > /* revert access to images */ > - qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.base, true, false); > + qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.base, > + true, false, false); > if (job->data.commit.topparent != job->disk->src) > - qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.topparent, true, false); > + qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.topparent, > + true, false, false); Here you see the misleading name. This is called to relabel an image called 'topparent' but yet set the 'topparent' flag to false. > baseparent->backingStore = NULL; > job->data.commit.topparent->backingStore = job->data.commit.base; [...] > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 3dfa71650d..32e8220d98 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -11589,6 +11589,8 @@ typedef enum { > QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 4, > /* VM already has access to the source and we are just modifying it */ > QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 5, > + /* whether the image is top parent of backing chain */ > + QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_TOP_PARENT = 1 << 6, whether the image is the top image of the backing chain (e.g. disk source) QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP > } qemuDomainStorageSourceAccessFlags; > > > @@ -11817,6 +11820,7 @@ qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver, > * @elem: source structure to set access for > * @readonly: setup read-only access if true > * @newSource: @elem describes a storage source which @vm can't access yet > + * @topparent: @elem is top parent of backing chain > * > * Allow a VM access to a single element of a disk backing chain; this helper > * ensures that the lock manager, cgroup device controller, and security manager > @@ -11824,13 +11828,17 @@ qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver, > * > * When modifying permissions of @elem which @vm can already access (is in the > * backing chain) @newSource needs to be set to false. > + * > + * When the @elem is top parent of a backing chain, then @topparent must be > + * true, otherwise it must be false. You want to specify this better. The flag must be set if the image is the topmost image of a given backing chain or meant to become the topmost image (for e.g. snapshots, or blockcopy or even in the end for active layer block commit, where we discard the top of the backing chain so one of the intermediates (the base) becomes the top of the chain. > */ > int > qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virStorageSourcePtr elem, > bool readonly, > - bool newSource) > + bool newSource, > + bool topparent) > { > qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE; > [...] > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 35ade1ef37..39c29a0d47 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -15141,7 +15141,8 @@ qemuDomainSnapshotDiskPrepareOne(virQEMUDriverPtr driver, > } > > /* set correct security, cgroup and locking options on the new image */ > - if (qemuDomainStorageSourceAccessAllow(driver, vm, dd->src, false, true) < 0) > + if (qemuDomainStorageSourceAccessAllow(driver, vm, dd->src, > + false, true, true) < 0) > return -1; > > dd->prepared = true; > @@ -18489,9 +18490,11 @@ qemuDomainBlockCommit(virDomainPtr dom, > * operation succeeds, but doing that requires tracking the > * operation in XML across libvirtd restarts. */ > clean_access = true; > - if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, false, false) < 0 || > + if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, > + false, false, false) < 0 || base may become the top layer of the chain after finishing the blockjob if we are doing an active commit. The finalizing code AFAIK does not relabel that image any more. > (top_parent && top_parent != disk->src && > - qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, false, false) < 0)) > + qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, > + false, false, false) < 0)) > goto endjob; > > if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, > @@ -18551,9 +18554,11 @@ qemuDomainBlockCommit(virDomainPtr dom, > virErrorPtr orig_err; > virErrorPreserveLast(&orig_err); > /* Revert access to read-only, if possible. */ > - qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, true, false); > + qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, > + true, false, false); Now this is fun. If this were an active block commit, the 'base' will no longer want to become the top image, so this might require some more trickery. > if (top_parent && top_parent != disk->src) > - qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, true, false); > + qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, > + true, false, false); > > virErrorRestore(&orig_err); > } > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index d9035055e8..425a21d77e 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -7851,7 +7851,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload, > (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 || > qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || > qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror, > - true) < 0)) > + true, false) < 0)) disk->mirror is the top of the chain. It may eventually even become the source of the disk > goto cleanup; > } > } This patch is not fixing qemuDomainStorageSourceChainAccessAllow which is also introducing new images (with backing chain) and neither the revoke functions used in hot-unplug where we remove the top image or in block copy finalizing where we unplug the whole old disk chain.
