Re: [PATCH 8/8] qemu: migration: Don't remember seclabel for images shared from current host
On Wed, Jul 24, 2024 at 05:34:32PM GMT, Peter Krempa wrote:
+static void
+qemuMigrationDstPrepareDiskSeclabels(virDomainObj *vm,
+ size_t nmigrate_disks,
+ const char **migrate_disks,
+ unsigned int flags)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
+ size_t i;
+
+ /* In case when storage is exported from this host, security label
+ * remembering would behave differently compared to the host which mounts
+ * the exported filesystem. Specifically for incoming migration remembering
+ * a seclabel would remember a seclabel already allowing access to the image,
+ * which is not desired. Thus we skip remembering of seclabels for images
+ * which are local to this host but accessed in a shared way from another
+ * host.
+ */
+ if (!cfg->sharedFilesystems ||
+ cfg->sharedFilesystems[0] == NULL)
+ return;
+
+ for (i = 0; i < vm->def->ndisks; i++) {
+ virDomainDiskDef *disk = vm->def->disks[i];
This only iterates over disks, so it's probably not surprising that
it fails when other types of storage are involved.
For example, if the domain uses UEFI, local -> remote migration works
but when attempting to migrate back I get
error: Requested operation is not valid: Setting different SELinux
label on /var/lib/libvirt/qemu/nvram/cirros-efi_VARS.fd which is
already in use
If I add TPM into the mix, local -> remote migration fails with
error: unable to lock
/var/lib/libvirt/swtpm/50601047-47f7-4f45-9b41-9112dfaa3539/tpm2/.lock
for metadata change: Resource temporarily unavailable
Setting remember_owner=0 in qemu.conf makes all these errors go away,
but of course that's a very big hammer and the idea here is to use a
much smaller, more targeted one.
I think it might be enough to extend this logic beyond disks so that
it covers UEFI and TPM too, but I'm not entirely sure those use a
virStorageSource behind the scenes. Hopefully that's the case and the
changes needed on top are minimal, but I figure you're in a better
position than me to look into it.
+ /* We care only about existing local storage */
+ if (virStorageSourceIsEmpty(disk->src) ||
+ !virStorageSourceIsLocalStorage(disk->src))
+ continue;
+
+ /* Only paths which are on local filesystem but shared elsewhere are
+ * relevant */
+ if (!virFileIsSharedFSOverride(disk->src->path,
cfg->sharedFilesystems))
+ continue;
+
+ /* Any storage that was migrated via NBD is technically fully local so
+ * we want seclabels remembered */
+ if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) {
+ if (qemuMigrationAnyCopyDisk(disk, nmigrate_disks, migrate_disks))
+ continue;
+ }
+
+ disk->src->seclabelSkipRemeber = true;
You misspelled "remember" here.
@@ -3154,6 +3201,8 @@ qemuMigrationDstPrepareActive(virQEMUDriver
*driver,
dataFD[0])))
goto error;
+ qemuMigrationDstPrepareDiskSeclabels(vm, nmigrate_disks, migrate_disks, flags);
+
if (qemuProcessPrepareDomain(driver, vm, startFlags) < 0)
goto error;
One thing that concerns me is that the changes appear to only be on
the destination side of the migration. So only the remote -> local
path is affected. But what about the local -> remote path?
If we assume remember_owner=1, then obviously the various XATTRs
related to remembering will be set on domain startup. What will take
care of collecting that garbage once local -> remote migration has
been performed? I think things might be fine based on the fact that
the XATTRs seems to be dropped for disks even in the current state,
but I thought I'd mention this specifically just in case.
Thanks a lot for looking into this!
--
Andrea Bolognani / Red Hat / Virtualization