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@xxxxxxxxxx> > >> --- > >> 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/ :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list