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
