6:25 p.m.
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