qemu: Tell secdrivers which images are top parent

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 of of the backing chain. Fortunately,
in places where we call secdrivers we know this and the
information can be passed to secdrivers.

The 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
blockcommit fails. This is caused by blockcommit code calling
qemuSecuritySetImageLabel() over the base image, possibly
multiple times (to ensure RW/RO access). 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.  Fortunately, we
can use the fact that seclabels are remembered only for the top
of the backing chain and not for the rest of the backing chain.
And thanks to the previous commit we can tell secdrivers which
images are top of the backing chain.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1803551

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This commit is contained in:
Michal Privoznik 2020-02-27 11:20:51 +01:00
parent 62f3d8adbc
commit 13eb6c1468
9 changed files with 57 additions and 20 deletions

View File

@ -469,8 +469,8 @@ qemuBackupDiskPrepareOneStorage(virDomainObjPtr vm,
dd->created = true; dd->created = true;
} }
if (qemuDomainStorageSourceAccessAllow(priv->driver, vm, dd->store, false, if (qemuDomainStorageSourceAccessAllow(priv->driver, vm, dd->store,
true) < 0) false, true, true) < 0)
return -1; return -1;
dd->labelled = true; dd->labelled = true;

View File

@ -1105,9 +1105,11 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver,
return; return;
/* revert access to images */ /* 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) 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, true);
baseparent->backingStore = NULL; baseparent->backingStore = NULL;
job->data.commit.topparent->backingStore = job->data.commit.base; job->data.commit.topparent->backingStore = job->data.commit.base;

View File

@ -296,7 +296,8 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
for (next = reopenimages; next; next = next->next) { for (next = reopenimages; next; next = next->next) {
virStorageSourcePtr src = next->data; virStorageSourcePtr src = next->data;
if (qemuDomainStorageSourceAccessAllow(driver, vm, src, false, false) < 0) if (qemuDomainStorageSourceAccessAllow(driver, vm, src,
false, false, false) < 0)
goto relabel; goto relabel;
relabelimages = g_slist_prepend(relabelimages, src); relabelimages = g_slist_prepend(relabelimages, src);
@ -311,7 +312,8 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
for (next = relabelimages; next; next = next->next) { for (next = relabelimages; next; next = next->next) {
virStorageSourcePtr src = next->data; virStorageSourcePtr src = next->data;
ignore_value(qemuDomainStorageSourceAccessAllow(driver, vm, src, true, false)); ignore_value(qemuDomainStorageSourceAccessAllow(driver, vm, src,
true, false, false));
} }
return rc; return rc;

View File

@ -11668,6 +11668,8 @@ typedef enum {
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 4, QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE = 1 << 4,
/* VM already has access to the source and we are just modifying it */ /* VM already has access to the source and we are just modifying it */
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 5, QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS = 1 << 5,
/* whether the image is the top image of the backing chain (e.g. disk source) */
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP = 1 << 6,
} qemuDomainStorageSourceAccessFlags; } qemuDomainStorageSourceAccessFlags;
@ -11745,6 +11747,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver,
bool force_ro = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_FORCE_READ_ONLY; bool force_ro = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_FORCE_READ_ONLY;
bool force_rw = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_FORCE_READ_WRITE; bool force_rw = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_FORCE_READ_WRITE;
bool revoke = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE; bool revoke = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE;
bool chain_top = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP;
int rc; int rc;
bool was_readonly = src->readonly; bool was_readonly = src->readonly;
bool revoke_cgroup = false; bool revoke_cgroup = false;
@ -11791,7 +11794,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver,
revoke_namespace = true; revoke_namespace = true;
} }
if (qemuSecuritySetImageLabel(driver, vm, src, chain) < 0) if (qemuSecuritySetImageLabel(driver, vm, src, chain, chain_top) < 0)
goto revoke; goto revoke;
revoke_label = true; revoke_label = true;
@ -11854,7 +11857,8 @@ qemuDomainStorageSourceChainAccessAllow(virQEMUDriverPtr driver,
virDomainObjPtr vm, virDomainObjPtr vm,
virStorageSourcePtr src) virStorageSourcePtr src)
{ {
qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN; qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN |
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP;
return qemuDomainStorageSourceAccessModify(driver, vm, src, flags); return qemuDomainStorageSourceAccessModify(driver, vm, src, flags);
} }
@ -11866,7 +11870,8 @@ qemuDomainStorageSourceChainAccessRevoke(virQEMUDriverPtr driver,
virStorageSourcePtr src) virStorageSourcePtr src)
{ {
qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE | qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE |
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN; QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN |
QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP;
return qemuDomainStorageSourceAccessModify(driver, vm, src, flags); return qemuDomainStorageSourceAccessModify(driver, vm, src, flags);
} }
@ -11896,6 +11901,7 @@ qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver,
* @elem: source structure to set access for * @elem: source structure to set access for
* @readonly: setup read-only access if true * @readonly: setup read-only access if true
* @newSource: @elem describes a storage source which @vm can't access yet * @newSource: @elem describes a storage source which @vm can't access yet
* @chainTop: @elem is top parent of backing chain
* *
* Allow a VM access to a single element of a disk backing chain; this helper * 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 * ensures that the lock manager, cgroup device controller, and security manager
@ -11903,13 +11909,20 @@ qemuDomainStorageSourceAccessRevoke(virQEMUDriverPtr driver,
* *
* When modifying permissions of @elem which @vm can already access (is in the * When modifying permissions of @elem which @vm can already access (is in the
* backing chain) @newSource needs to be set to false. * backing chain) @newSource needs to be set to false.
*
* The @chainTop flag must be set if the @elem 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 int
qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver, qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver,
virDomainObjPtr vm, virDomainObjPtr vm,
virStorageSourcePtr elem, virStorageSourcePtr elem,
bool readonly, bool readonly,
bool newSource) bool newSource,
bool chainTop)
{ {
qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE; qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE;
@ -11921,6 +11934,9 @@ qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver,
if (!newSource) if (!newSource)
flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS; flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS;
if (chainTop)
flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP;
return qemuDomainStorageSourceAccessModify(driver, vm, elem, flags); return qemuDomainStorageSourceAccessModify(driver, vm, elem, flags);
} }

View File

@ -896,7 +896,8 @@ int qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver,
virDomainObjPtr vm, virDomainObjPtr vm,
virStorageSourcePtr elem, virStorageSourcePtr elem,
bool readonly, bool readonly,
bool newSource); bool newSource,
bool chainTop);
int qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDefPtr disk, int qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDefPtr disk,
virStorageSourcePtr src, virStorageSourcePtr src,

View File

@ -15142,7 +15142,8 @@ qemuDomainSnapshotDiskPrepareOne(virQEMUDriverPtr driver,
} }
/* set correct security, cgroup and locking options on the new image */ /* 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; return -1;
dd->prepared = true; dd->prepared = true;
@ -18490,11 +18491,19 @@ qemuDomainBlockCommit(virDomainPtr dom,
* operation succeeds, but doing that requires tracking the * operation succeeds, but doing that requires tracking the
* operation in XML across libvirtd restarts. */ * operation in XML across libvirtd restarts. */
clean_access = true; clean_access = true;
if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, false, false) < 0 || if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource,
(top_parent && top_parent != disk->src && false, false, false) < 0)
qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, false, false) < 0))
goto endjob; goto endjob;
if (top_parent && top_parent != disk->src) {
/* While top_parent is topmost image, we don't need to remember its
* owner as it will be overwritten upon finishing the commit. Hence,
* pass chainTop = false. */
if (qemuDomainStorageSourceAccessAllow(driver, vm, top_parent,
false, false, false) < 0)
goto endjob;
}
if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource,
baseSource, baseSource,
flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE,
@ -18552,9 +18561,11 @@ qemuDomainBlockCommit(virDomainPtr dom,
virErrorPtr orig_err; virErrorPtr orig_err;
virErrorPreserveLast(&orig_err); virErrorPreserveLast(&orig_err);
/* Revert access to read-only, if possible. */ /* Revert access to read-only, if possible. */
qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, true, false); qemuDomainStorageSourceAccessAllow(driver, vm, baseSource,
true, false, false);
if (top_parent && top_parent != disk->src) 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); virErrorRestore(&orig_err);
} }

View File

@ -7856,7 +7856,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
(qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 || (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 ||
qemuSetupImageChainCgroup(vm, disk->mirror) < 0 || qemuSetupImageChainCgroup(vm, disk->mirror) < 0 ||
qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror, qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror,
true) < 0)) true, true) < 0))
goto cleanup; goto cleanup;
} }
} }

View File

@ -98,7 +98,8 @@ int
qemuSecuritySetImageLabel(virQEMUDriverPtr driver, qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm, virDomainObjPtr vm,
virStorageSourcePtr src, virStorageSourcePtr src,
bool backingChain) bool backingChain,
bool chainTop)
{ {
qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjPrivatePtr priv = vm->privateData;
pid_t pid = -1; pid_t pid = -1;
@ -108,6 +109,9 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
if (backingChain) if (backingChain)
labelFlags |= VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN; labelFlags |= VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN;
if (chainTop)
labelFlags |= VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP;
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid; pid = vm->pid;

View File

@ -36,7 +36,8 @@ void qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
int qemuSecuritySetImageLabel(virQEMUDriverPtr driver, int qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm, virDomainObjPtr vm,
virStorageSourcePtr src, virStorageSourcePtr src,
bool backingChain); bool backingChain,
bool chainTop);
int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm, virDomainObjPtr vm,