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

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(a)redhat.com&gt; >> --- >> 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.

> 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?