qemu_blockjob: Remove secdriver metadata for whole backing chain on job completion

[libvirt] [PATCH] qemu_blockjob: Remove secdriver metadata for whole backing chain on job completion

Posted by Michal Privoznik 4 years, 7 months ago

Turns out, block mirror is not the only job a disk can have. It
can also do commits of one layer into the other. Or possibly some
other tricks too. Problem is that while we set seclabels on given
layers of backing chain when the job is starting (via
qemuDomainStorageSourceAccessAllow()) we don't restore them when
job finishes. This leaves XATTRs set and corresponding images
unusable.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

Not sure if we want to remove XATTRs for the top layer too or just the
rest of the backing chain (n=disk->src  vs.  n=disk->src->backingStore).
Peter?

 src/qemu/qemu_blockjob.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index a991309ee7..6408f95e4e 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -658,9 +658,9 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
         virObjectUnref(disk->src);
         disk->src = disk->mirror;
     } else {
+        virStorageSourcePtr n;
+
         if (disk->mirror) {
-            virStorageSourcePtr n;
-
             virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
 
             /* Ideally, we would restore seclabels on the backing chain here
@@ -678,6 +678,16 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
 
             virObjectUnref(disk->mirror);
         }
+
+        for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
+            if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) {
+                VIR_WARN("Unable to remove disk metadata on "
+                         "vm %s from %s (disk target %s)",
+                         vm->def->name,
+                         NULLSTR(n->path),
+                         disk->dst);
+            }
+        }
     }
 
     /* Recompute the cached backing chain to match our
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu_blockjob: Remove secdriver metadata for whole backing chain on job completion

Posted by Peter Krempa 4 years, 7 months ago

On Mon, Sep 16, 2019 at 12:34:32 +0200, Michal Privoznik wrote:
> Turns out, block mirror is not the only job a disk can have. It
> can also do commits of one layer into the other. Or possibly some
> other tricks too. Problem is that while we set seclabels on given
> layers of backing chain when the job is starting (via
> qemuDomainStorageSourceAccessAllow()) we don't restore them when
> job finishes. This leaves XATTRs set and corresponding images
> unusable.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> Not sure if we want to remove XATTRs for the top layer too or just the
> rest of the backing chain (n=disk->src  vs.  n=disk->src->backingStore).
> Peter?

That depends on the job. In case of the legacy handler both job types
which modify which top level image (thus the image which is stored as
disk->src will no longer be used after the handler returns) is being
used are handled via the
"if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {"
code path.

That case already handles the top level image where the metadata is
transferred, but both block copy and active block commit may remove a
chain of images from use.

All other cases are handled via the '} else {' branch:

In case of block pull, libvirt does not need to modify any permissions
as the data is pulled into the top level image directly.

In case of (non active layer) block commit (assume the following backing
chain for example's sake: #activeimage -> #dummy1 -> #top ->
#intermediate1..n -> #base -> #endofchain). Libvirt modifies #dummy1's
and #base's permissions to read write. #dummy1 gets the backing store
specifier updated and #base gets all the data. Then #top and all
#intermediateH images are dropped from the chain. #activeimage stays
read-write.

This means that we probably shouldn't modify/drop labels for
#activeimage (disk->src) as it will be used same way it was.

Note that there's still the question of failed cancelled jobs. We still
do the relabelling when starting the job and the images are still used,
the question is whether it's cleaned up during VM shutdown.

> 
>  src/qemu/qemu_blockjob.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index a991309ee7..6408f95e4e 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -658,9 +658,9 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
>          virObjectUnref(disk->src);
>          disk->src = disk->mirror;
>      } else {
> +        virStorageSourcePtr n;
> +
>          if (disk->mirror) {
> -            virStorageSourcePtr n;
> -
>              virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
>  
>              /* Ideally, we would restore seclabels on the backing chain here
> @@ -678,6 +678,16 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
>  
>              virObjectUnref(disk->mirror);
>          }
> +
> +        for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {

disk->src is probably okay to stay as-is.

> +            if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) {
> +                VIR_WARN("Unable to remove disk metadata on "
> +                         "vm %s from %s (disk target %s)",
> +                         vm->def->name,
> +                         NULLSTR(n->path),
> +                         disk->dst);
> +            }
> +        }
>      }
>  
>      /* Recompute the cached backing chain to match our
> -- 
> 2.21.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu_blockjob: Remove secdriver metadata for whole backing chain on job completion

Posted by Peter Krempa 4 years, 6 months ago

On Mon, Sep 16, 2019 at 16:13:39 +0200, Peter Krempa wrote:
> On Mon, Sep 16, 2019 at 12:34:32 +0200, Michal Privoznik wrote:
> > Turns out, block mirror is not the only job a disk can have. It
> > can also do commits of one layer into the other. Or possibly some
> > other tricks too. Problem is that while we set seclabels on given
> > layers of backing chain when the job is starting (via
> > qemuDomainStorageSourceAccessAllow()) we don't restore them when
> > job finishes. This leaves XATTRs set and corresponding images
> > unusable.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> > 
> > Not sure if we want to remove XATTRs for the top layer too or just the
> > rest of the backing chain (n=disk->src  vs.  n=disk->src->backingStore).
> > Peter?
> 
> That depends on the job. In case of the legacy handler both job types
> which modify which top level image (thus the image which is stored as
> disk->src will no longer be used after the handler returns) is being
> used are handled via the
> "if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {"
> code path.
> 
> That case already handles the top level image where the metadata is
> transferred, but both block copy and active block commit may remove a
> chain of images from use.
> 
> All other cases are handled via the '} else {' branch:
> 
> In case of block pull, libvirt does not need to modify any permissions
> as the data is pulled into the top level image directly.
> 
> In case of (non active layer) block commit (assume the following backing
> chain for example's sake: #activeimage -> #dummy1 -> #top ->
> #intermediate1..n -> #base -> #endofchain). Libvirt modifies #dummy1's
> and #base's permissions to read write. #dummy1 gets the backing store
> specifier updated and #base gets all the data. Then #top and all
> #intermediateH images are dropped from the chain. #activeimage stays
> read-write.
> 
> This means that we probably shouldn't modify/drop labels for
> #activeimage (disk->src) as it will be used same way it was.
> 
> Note that there's still the question of failed cancelled jobs. We still
> do the relabelling when starting the job and the images are still used,
> the question is whether it's cleaned up during VM shutdown.
> 
> > 
> >  src/qemu/qemu_blockjob.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > index a991309ee7..6408f95e4e 100644
> > --- a/src/qemu/qemu_blockjob.c
> > +++ b/src/qemu/qemu_blockjob.c
> > @@ -658,9 +658,9 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
> >          virObjectUnref(disk->src);
> >          disk->src = disk->mirror;
> >      } else {
> > +        virStorageSourcePtr n;
> > +
> >          if (disk->mirror) {
> > -            virStorageSourcePtr n;
> > -
> >              virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
> >  
> >              /* Ideally, we would restore seclabels on the backing chain here
> > @@ -678,6 +678,16 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
> >  
> >              virObjectUnref(disk->mirror);
> >          }
> > +
> > +        for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
> 
> disk->src is probably okay to stay as-is.
> 
> > +            if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) {
> > +                VIR_WARN("Unable to remove disk metadata on "
> > +                         "vm %s from %s (disk target %s)",
> > +                         vm->def->name,
> > +                         NULLSTR(n->path),
> > +                         disk->dst);
> > +            }
> > +        }
> >      }
> >  
> >      /* Recompute the cached backing chain to match our
> > -- 

ACK as I've forgot to add it here.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list