2:24 a.m.
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(a)redhat.com>
---
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.