This function has funny approach to retvals. Document them more clearly. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 9857223bbf..0523613d4a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1257,9 +1257,20 @@ virSecuritySELinuxGetProcessLabel(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. */ +/** + * virSecuritySELinuxSetFileconImpl: + * @path: path to the file to set context on + * @tcon: target context to set + * @optional: whether to treat errors as fatal + * @privileged: whether running as privileged user + * + * Set @tcon SELinux context on @path. If unable to do so, check SELinux + * configuration and produce sensible error message suggesting solution. + * + * Returns: -1 if failed to set context and SELinux is in enforcing mode + * 1 if failed to set context and @optional is true + * 0 otherwise. + */ static int virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, bool optional, bool privileged) -- 2.21.0

The only thing that the @optional argument does is that it makes the function return 1 instead of 0 if setting SELinux context failed in a non-critical fashion. Drop the argument then and return 1 in that case. This enables caller to learn if SELinux context was set or not. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0523613d4a..35385f4a23 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1261,19 +1261,23 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * virSecuritySELinuxSetFileconImpl: * @path: path to the file to set context on * @tcon: target context to set - * @optional: whether to treat errors as fatal * @privileged: whether running as privileged user * * Set @tcon SELinux context on @path. If unable to do so, check SELinux * configuration and produce sensible error message suggesting solution. + * It may happen that setting context fails but hypervisor will be able to + * open the @path successfully. This is because some file systems don't + * support SELinux, are RO, or the @path had the correct context from the + * start. If that is the case, a positive one is returned. * * Returns: -1 if failed to set context and SELinux is in enforcing mode - * 1 if failed to set context and @optional is true - * 0 otherwise. + * 1 if failed to set context, + * 0 if context was set successfully. */ static int -virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, - bool optional, bool privileged) +virSecuritySELinuxSetFileconImpl(const char *path, + const char *tcon, + bool privileged) { security_context_t econ; @@ -1289,7 +1293,7 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ - return optional ? 1 : 0; + return 1; } freecon(econ); } @@ -1326,9 +1330,9 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon, VIR_INFO("Setting security context '%s' on '%s' not supported", tcon, path); } - if (optional) - return 1; } + + return 1; } return 0; } @@ -1388,7 +1392,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) + if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0) goto cleanup; ret = 0; @@ -1553,7 +1557,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(newpath, fcon, false, privileged) < 0) + if (virSecuritySELinuxSetFileconImpl(newpath, fcon, privileged) < 0) goto cleanup; ret = 0; -- 2.21.0

There is no real difference between virSecuritySELinuxSetFilecon() and virSecuritySELinuxSetFileconOptional(). Drop the latter in favour of the former. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 53 ++++++++++++++------------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 35385f4a23..0d9790829e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1419,15 +1419,6 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } -static int -virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, - const char *path, - const char *tcon, - bool remember) -{ - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, remember); -} - static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, @@ -1884,28 +1875,28 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, parent_seclabel->label, remember); } else if (!parent || parent == src) { if (src->shared) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->file_context, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + data->file_context, + remember); } else if (src->readonly) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->content_context, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + data->content_context, + remember); } else if (secdef->imagelabel) { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - secdef->imagelabel, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + secdef->imagelabel, + remember); } else { ret = 0; } } else { - ret = virSecuritySELinuxSetFileconOptional(mgr, - src->path, - data->content_context, - remember); + ret = virSecuritySELinuxSetFilecon(mgr, + src->path, + data->content_context, + remember); } if (ret == 1 && !disk_seclabel) { @@ -2045,14 +2036,14 @@ virSecuritySELinuxSetSCSILabel(virSCSIDevicePtr dev, return 0; if (virSCSIDeviceGetShareable(dev)) - return virSecuritySELinuxSetFileconOptional(mgr, file, - data->file_context, true); + return virSecuritySELinuxSetFilecon(mgr, file, + data->file_context, true); else if (virSCSIDeviceGetReadonly(dev)) - return virSecuritySELinuxSetFileconOptional(mgr, file, - data->content_context, true); + return virSecuritySELinuxSetFilecon(mgr, file, + data->content_context, true); else - return virSecuritySELinuxSetFileconOptional(mgr, file, - secdef->imagelabel, true); + return virSecuritySELinuxSetFilecon(mgr, file, + secdef->imagelabel, true); } static int -- 2.21.0

Now, that we don't need to remember if setting context is 'optional' (the argument only made virSecuritySELinuxSetFileconImpl() return a different success code), we can drop it from the _virSecuritySELinuxContextItem structure as we don't need to remember it in transactions. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0d9790829e..e7cf5f2e53 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -81,7 +81,6 @@ typedef virSecuritySELinuxContextItem *virSecuritySELinuxContextItemPtr; struct _virSecuritySELinuxContextItem { char *path; char *tcon; - bool optional; bool remember; /* Whether owner remembering should be done for @path/@src */ bool restore; /* Whether current operation is 'set' or 'restore' */ }; @@ -122,7 +121,6 @@ static int virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *path, const char *tcon, - bool optional, bool remember, bool restore) { @@ -135,7 +133,6 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, if (VIR_STRDUP(item->path, path) < 0 || VIR_STRDUP(item->tcon, tcon) < 0) goto cleanup; - item->optional = optional; item->remember = remember; item->restore = restore; @@ -170,7 +167,6 @@ virSecuritySELinuxContextListFree(void *opaque) * virSecuritySELinuxTransactionAppend: * @path: Path to chown * @tcon: target context - * @optional: true if setting @tcon is optional * @remember: if the original owner should be recorded/recalled * @restore: if current operation is set or restore * @@ -187,7 +183,6 @@ virSecuritySELinuxContextListFree(void *opaque) static int virSecuritySELinuxTransactionAppend(const char *path, const char *tcon, - bool optional, bool remember, bool restore) { @@ -198,7 +193,7 @@ virSecuritySELinuxTransactionAppend(const char *path, return 0; if (virSecuritySELinuxContextListAppend(list, path, tcon, - optional, remember, restore) < 0) + remember, restore) < 0) return -1; return 1; @@ -234,7 +229,6 @@ virSecuritySELinuxRecallLabel(const char *path, static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, const char *path, const char *tcon, - bool optional, bool remember); @@ -290,7 +284,6 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, rv = virSecuritySELinuxSetFileconHelper(list->manager, item->path, item->tcon, - item->optional, remember); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, @@ -1342,7 +1335,6 @@ static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, const char *path, const char *tcon, - bool optional, bool remember) { bool privileged = virSecurityManagerGetPrivileged(mgr); @@ -1353,7 +1345,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, int ret = -1; if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, - optional, remember, false)) < 0) + remember, false)) < 0) return -1; else if (rc > 0) return 0; @@ -1425,7 +1417,7 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *tcon, bool remember) { - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, remember); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, remember); } static int @@ -1512,7 +1504,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, } if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, - false, recall, true)) < 0) { + recall, true)) < 0) { goto cleanup; } else if (rc > 0) { ret = 0; -- 2.21.0

This function is no longer needed because after previous commits it's just an alias to virSecuritySELinuxSetFilecon. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e7cf5f2e53..855eaafdda 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -226,7 +226,7 @@ virSecuritySELinuxRecallLabel(const char *path, } -static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, +static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, const char *path, const char *tcon, bool remember); @@ -281,10 +281,10 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, const bool remember = item->remember && list->lock; if (!item->restore) { - rv = virSecuritySELinuxSetFileconHelper(list->manager, - item->path, - item->tcon, - remember); + rv = virSecuritySELinuxSetFilecon(list->manager, + item->path, + item->tcon, + remember); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, item->path, @@ -1332,10 +1332,10 @@ virSecuritySELinuxSetFileconImpl(const char *path, static int -virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, - const char *path, - const char *tcon, - bool remember) +virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, + const char *path, + const char *tcon, + bool remember) { bool privileged = virSecurityManagerGetPrivileged(mgr); security_context_t econ = NULL; @@ -1411,15 +1411,6 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } -static int -virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, - const char *path, - const char *tcon, - bool remember) -{ - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, remember); -} - static int virSecuritySELinuxFSetFilecon(int fd, char *tcon) { -- 2.21.0

There are some network file systems that do support XATTRs (e.g. gluster via FUSE). And they appear to support SELinux too. However, not really. Problem is, that it is impossible to change SELinux label of a file stored there, and yet we claim success (rightfully - hypervisor succeeds in opening the file). But this creates a problem for us - from XATTR bookkeeping POV, we haven't changed the label and thus if we remembered any label, we must roll back and remove it. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740506 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 855eaafdda..4d0c7a46ae 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1384,12 +1384,22 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0) + if ((rc = virSecuritySELinuxSetFileconImpl(path, tcon, privileged)) < 0) goto cleanup; + /* At this point, we can claim success. However, + * virSecuritySELinuxSetFileconImpl() could returned 0 + * (SELinux label changed) or 1 (SELinux label NOT changed in + * a non-critical fashion). If the label was NOT changed, we + * must remove remembered label then - there's nothing to + * remember, is there? But of the label was changed, don't + * remove the remembered label. It's valid. */ + if (rc == 0) + rollback = false; + ret = 0; cleanup: - if (ret < 0 && rollback) { + if (rollback) { virErrorPtr origerr; virErrorPreserveLast(&origerr); -- 2.21.0