I noticed that the logs contained messages like this: 2011-12-05 23:32:40.382+0000: 26569: warning : SELinuxRestoreSecurityFileLabel:533 : cannot lookup default selinux label for /nfs/libvirt/images/dom.img for all my domain images living on NFS. But if we would just remember that on domain creation that we were unable to set a SELinux label (due to NFSv3 lacking labels, or NFSv4 not being configured to expose attributes), then we could avoid wasting the time trying to clear the label on domain shutdown. This in turn is one less point of NFS failure, especially since there have been documented cases of virDomainDestroy hanging during an attempted operation on a failed NFS connection. I tested that this works over libvirtd restart for <disk> images; it cannot work for other labeled file types unless we also enhance those in-memory representations to track whether a label is in force. * src/conf/domain_conf.h (_virDomainDiskDef): Add member. * src/conf/domain_conf.c (virDomainDiskDefParseXML): Parse it. (virDomainDiskDefFormat): Output it. * src/security/security_selinux.c (SELinuxSetFilecon): Move guts... (SELinuxSetFileconHelper): ...to new function. (SELinuxSetFileconOptional): New function. (SELinuxSetSecurityFileLabel): Remember if labeling failed. (SELinuxRestoreSecurityImageLabelInt): Skip relabeling on NFS. --- This is an attempt to address the primary symptom (but NOT the root cause) of https://bugzilla.redhat.com/746666, since that is a documented case of a hang during the label restoration that would be skipped had we remembered that no label was applied at domain start. A full solution to that problem will involve much more invasive lock refactoring, and probably the creation of one thread per VM rather than the current approach of reusing RPC threads for VM operations, where the per-VM thread is used for any operation that might hang on NFS. https://www.redhat.com/archives/libvir-list/2011-November/msg00267.html src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + src/security/security_selinux.c | 39 ++++++++++++++++++++++++++++----------- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 75e51a0..38d24e0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2774,6 +2774,9 @@ virDomainDiskDefParseXML(virCapsPtr caps, } else if (xmlStrEqual(cur->name, BAD_CAST "transient")) { def->transient = 1; } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && + xmlStrEqual(cur->name, BAD_CAST "nolabel")) { + def->noSecurityLabel = 1; + } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ devaddr = virXMLPropString(cur, "devaddr"); @@ -9856,6 +9859,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, " <shareable/>\n"); if (def->transient) virBufferAddLit(buf, " <transient/>\n"); + if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && def->noSecurityLabel) + virBufferAddLit(buf, " <nolabel/>\n"); virBufferEscapeString(buf, " <serial>%s</serial>\n", def->serial); if (def->encryption) { virBufferAdjustIndent(buf, 6); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d6ed898..97786f7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -359,6 +359,7 @@ struct _virDomainDiskDef { unsigned int readonly : 1; unsigned int shared : 1; unsigned int transient : 1; + unsigned int noSecurityLabel : 1; virDomainDeviceInfo info; virStorageEncryptionPtr encryption; }; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 78c0d45..864b35e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -394,8 +394,11 @@ SELinuxGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +/* Attempt to change the label of PATH to TCON. If OPTIONAL is true, + * return 1 if labelling was not possible. Otherwise, require a label + * change, and return 0 for success, -1 for failure. */ static int -SELinuxSetFilecon(const char *path, char *tcon) +SELinuxSetFileconHelper(const char *path, char *tcon, bool optional) { security_context_t econ; @@ -408,7 +411,7 @@ SELinuxSetFilecon(const char *path, char *tcon) if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ - return 0; + return optional ? 1 : 0; } freecon(econ); } @@ -440,12 +443,26 @@ SELinuxSetFilecon(const char *path, char *tcon) VIR_INFO("Setting security context '%s' on '%s' not supported", tcon, path); } + if (optional) + return 1; } } return 0; } static int +SELinuxSetFileconOptional(const char *path, char *tcon) +{ + return SELinuxSetFileconHelper(path, tcon, true); +} + +static int +SELinuxSetFilecon(const char *path, char *tcon) +{ + return SELinuxSetFileconHelper(path, tcon, false); +} + +static int SELinuxFSetFilecon(int fd, char *tcon) { security_context_t econ; @@ -549,7 +566,7 @@ SELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - if (secdef->norelabel) + if (secdef->norelabel || disk->noSecurityLabel) return 0; /* Don't restore labels on readoly/shared disks, because @@ -606,21 +623,21 @@ SELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (depth == 0) { if (disk->shared) { - ret = SELinuxSetFilecon(path, default_image_context); + ret = SELinuxSetFileconOptional(path, default_image_context); } else if (disk->readonly) { - ret = SELinuxSetFilecon(path, default_content_context); + ret = SELinuxSetFileconOptional(path, default_content_context); } else if (secdef->imagelabel) { - ret = SELinuxSetFilecon(path, secdef->imagelabel); + ret = SELinuxSetFileconOptional(path, secdef->imagelabel); } else { ret = 0; } } else { - ret = SELinuxSetFilecon(path, default_content_context); + ret = SELinuxSetFileconOptional(path, default_content_context); + } + if (ret == 1) { + disk->noSecurityLabel = 1; + ret = 0; } - if (ret < 0 && - virStorageFileIsSharedFSType(path, - VIR_STORAGE_FILE_SHFS_NFS) == 1) - ret = 0; return ret; } -- 1.7.7.3