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>

diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 2cc6ff7a42206dac26f9abfbd9fcb148eb91e609..8b66ee8d1f6afc45940a67ee71b216793f3b4541 100644 (file)
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -469,8 +469,8 @@ qemuBackupDiskPrepareOneStorage(virDomainObjPtr vm,
         dd->created = true;
     }
 
-    if (qemuDomainStorageSourceAccessAllow(priv->driver, vm, dd->store, false,
-                                           true) < 0)
+    if (qemuDomainStorageSourceAccessAllow(priv->driver, vm, dd->store,
+                                           false, true, true) < 0)
         return -1;
 
     dd->labelled = true;

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 71df0d1ab2830f78b88bd37e954d96472b41413a..e894e1634db512d8d7a07723bdd001c6024fba46 100644 (file)
--- 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, true);
 
     baseparent->backingStore = NULL;
     job->data.commit.topparent->backingStore = job->data.commit.base;

diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index a387e7dfe743eecde928cf2eed70be4f7a5e4aed..ea87b09aa09f1f42b8577f07f30b5006bb83ff1f 100644 (file)
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -296,7 +296,8 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
     for (next = reopenimages; next; next = next->next) {
         virStorageSourcePtr src = next->data;
 
-        if (qemuDomainStorageSourceAccessAllow(driver, vm, src, false, false) < 0)
+        if (qemuDomainStorageSourceAccessAllow(driver, vm, src,
+                                               false, false, false) < 0)
             goto relabel;
 
         relabelimages = g_slist_prepend(relabelimages, src);
@@ -311,7 +312,8 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
     for (next = relabelimages; next; next = next->next) {
         virStorageSourcePtr src = next->data;
 
-        ignore_value(qemuDomainStorageSourceAccessAllow(driver, vm, src, true, false));
+        ignore_value(qemuDomainStorageSourceAccessAllow(driver, vm, src,
+                                                        true, false, false));
     }
 
     return rc;

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 33c2158eb5eea6b0cb18892909d876df4e0f5ec4..3d3f796d8544775a946aaf59ec54ac218916da4a 100644 (file)
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11668,6 +11668,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 the top image of the backing chain (e.g. disk source) */
+    QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP = 1 << 6,
 } qemuDomainStorageSourceAccessFlags;
 
 
@@ -11745,6 +11747,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver,
     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 revoke = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_REVOKE;
+    bool chain_top = flags & QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP;
     int rc;
     bool was_readonly = src->readonly;
     bool revoke_cgroup = false;
@@ -11791,7 +11794,7 @@ qemuDomainStorageSourceAccessModify(virQEMUDriverPtr driver,
         revoke_namespace = true;
     }
 
-    if (qemuSecuritySetImageLabel(driver, vm, src, chain) < 0)
+    if (qemuSecuritySetImageLabel(driver, vm, src, chain, chain_top) < 0)
         goto revoke;
 
     revoke_label = true;
@@ -11854,7 +11857,8 @@ qemuDomainStorageSourceChainAccessAllow(virQEMUDriverPtr driver,
                                         virDomainObjPtr vm,
                                         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);
 }
@@ -11866,7 +11870,8 @@ qemuDomainStorageSourceChainAccessRevoke(virQEMUDriverPtr driver,
                                          virStorageSourcePtr src)
 {
     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);
 }
@@ -11896,6 +11901,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
+ * @chainTop: @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
@@ -11903,13 +11909,20 @@ 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.
+ *
+ * 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
 qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver,
                                    virDomainObjPtr vm,
                                    virStorageSourcePtr elem,
                                    bool readonly,
-                                   bool newSource)
+                                   bool newSource,
+                                   bool chainTop)
 {
     qemuDomainStorageSourceAccessFlags flags = QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_SKIP_REVOKE;
 
@@ -11921,6 +11934,9 @@ qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver,
     if (!newSource)
         flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_MODIFY_ACCESS;
 
+    if (chainTop)
+        flags |= QEMU_DOMAIN_STORAGE_SOURCE_ACCESS_CHAIN_TOP;
+
     return qemuDomainStorageSourceAccessModify(driver, vm, elem, flags);
 }
 

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 476056c73f4e3097580a5d76be6257badb8da0e5..3929ee9ca1f475b0a1e48c3d104a6af49e32cfa2 100644 (file)
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -896,7 +896,8 @@ int qemuDomainStorageSourceAccessAllow(virQEMUDriverPtr driver,
                                        virDomainObjPtr vm,
                                        virStorageSourcePtr elem,
                                        bool readonly,
-                                       bool newSource);
+                                       bool newSource,
+                                       bool chainTop);
 
 int qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDefPtr disk,
                                            virStorageSourcePtr src,

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3707448f499d39fd148f6c908220810a86ea9c11..cd761f87b581527bc446d0a07718fcf280542f8a 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15142,7 +15142,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;
@@ -18490,11 +18491,19 @@ 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 ||
-        (top_parent && top_parent != disk->src &&
-         qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, false, false) < 0))
+    if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource,
+                                           false, false, false) < 0)
         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,
                                           baseSource,
                                           flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE,
@@ -18552,9 +18561,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);
         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 bec822a2ae52a1d7dfde6a76c0cad1d6621a146c..499d39a9202ccac95c56fc315d2eb1bd845f4457 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7856,7 +7856,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
                 (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 ||
                  qemuSetupImageChainCgroup(vm, disk->mirror) < 0 ||
                  qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror,
-                                           true) < 0))
+                                           true, true) < 0))
                 goto cleanup;
         }
     }

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 2aa2b5b9c653e6fb9333c99d2738814779a8277f..484fc345525f8b9d87537bd47deddd8022ebd463 100644 (file)
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -98,7 +98,8 @@ int
 qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
                           virDomainObjPtr vm,
                           virStorageSourcePtr src,
-                          bool backingChain)
+                          bool backingChain,
+                          bool chainTop)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
     pid_t pid = -1;
@@ -108,6 +109,9 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
     if (backingChain)
         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))
         pid = vm->pid;
 

diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index a8c648ece1b272a80882e5fcdac16f455b9aa2e0..c8516005ac43a64097b0bb403070fa6ac4e9d781 100644 (file)
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -36,7 +36,8 @@ void qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
 int qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
                               virDomainObjPtr vm,
                               virStorageSourcePtr src,
-                              bool backingChain);
+                              bool backingChain,
+                              bool chainTop);
 
 int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
                                   virDomainObjPtr vm,