[PATCH 2/2] qemu: Tell secdrivers which images are top parent

Peter Krempa pkrempa at redhat.com
Fri Feb 28 08:24:05 UTC 2020


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 at 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.




More information about the libvir-list mailing list