[libvirt] [PATCH 04/10] qemuSecurityRestoreAllLabel: Don't use transactions

[Daniel P. Berrange]

On Fri, Jan 20, 2017 at 11:34:06AM +0100, Michal Privoznik wrote:
> On 01/20/2017 11:18 AM, Daniel P. Berrange wrote:
> > On Fri, Jan 20, 2017 at 10:42:44AM +0100, Michal Privoznik wrote:
> >> Because of the nature of security driver transactions, it is
> >> impossible to use them properly. The thing is, transactions enter
> >> the domain namespace and commit all the seclabel changes.
> >> However, in RestoreAllLabel() this is impossible - the qemu
> >> process, the only process running in the namespace, is gone. And
> >> thus is the namespace. Therefore we shouldn't use the transactions
> >> as there is no namespace to enter.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >> ---
> >>  src/qemu/qemu_security.c | 25 +++++++++----------------
> >>  1 file changed, 9 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> >> index 544feeb4a..13d99cdbd 100644
> >> --- a/src/qemu/qemu_security.c
> >> +++ b/src/qemu/qemu_security.c
> >> @@ -73,22 +73,15 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
> >>                              virDomainObjPtr vm,
> >>                              bool migrated)
> >>  {
> >> -    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> >> -        virSecurityManagerTransactionStart(driver->securityManager) < 0)
> >> -        goto cleanup;
> >> -
> >> -    if (virSecurityManagerRestoreAllLabel(driver->securityManager,
> >> -                                          vm->def,
> >> -                                          migrated) < 0)
> >> -        goto cleanup;
> >> -
> >> -    if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> >> -        virSecurityManagerTransactionCommit(driver->securityManager,
> >> -                                            vm->pid) < 0)
> >> -        goto cleanup;
> >> -
> >> - cleanup:
> >> -    virSecurityManagerTransactionAbort(driver->securityManager);
> >> +    /* In contrast to qemuSecuritySetAllLabel, do not use
> >> +     * secdriver transactions here. This function is called from
> >> +     * qemuProcessStop() which is meant to do cleanup after qemu
> >> +     * process died. If it did do, the namespace is gone as qemu
> >> +     * was the only process running there. We would not succeed
> >> +     * in entering the namespace then. */
> >> +    virSecurityManagerRestoreAllLabel(driver->securityManager,
> >> +                                      vm->def,
> >> +                                      migrated);
> > 
> > This means we'll be running restore on /dev/BLAH in the host namespace,
> > which is a file we didn't set a label/permissions on originally. I think
> > this is probably safe though, as we'd just be resetting it to a label
> > that it should already have and thus be a no-op. I'm a little more
> > concerned about file permiissions though, as we might end up changing
> > group from "disk" to "root" for example.
> > 
> 
> Which is no different from current situation with namespaces disabled or
> using just any pre 3.0.0 release.

Hmm, good point. No need to solve this right now then - it can just be
a todo item

> > It feels like we need to explicitly skip restore for block devices
> > if we had namespaces in use previously.
> 
> If we want to go this way, sure. But I'm not quite sure how to design
> this. E.g. have a filter callback that would return "yes restore label
> on this path", or "no, do not restore anything on this path". And
> obviously set the callback only for the RestoreAll() if the namespaces
> are enabled? Or what do you suggest?

I guess, lets not special case this right now - we have the long standing
problem of (not) correctly restoring permissions for all paths, whether
files or block devs.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|