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

[Peter Krempa]

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