[PATCH 0/8] security_dac: Couple of cleanups
I've started looking at how we could fix the following bug: https://bugzilla.redhat.com/show_bug.cgi?id=1786222 Long story short, we are not using just async signal safe functions in a forked off thread that runs chown() from within the domain's private namespace. Hence, we ran into a deadlock (although the case of the bug the deadlock is in glibc not gluster). So far I don't have any fix, but I have couple of cleanups. Michal Prívozník (8): security_dac: Use g_autofree security_dac: Introduce virSecurityDACChownItemFree() security_dac: Introduce g_autoptr for virSecurityDACChownList security_dac: Don't check for !priv in virSecurityDACSetOwnershipInternal() virSecurityDACSetOwnershipInternal: Drop dead code virSecurityDACSetOwnershipInternal: Don't overwrite @path argument virSecurityDACSetOwnershipInternal: Fix WIN32 code qemu: Deduplicate code in qemuSecurityChownCallback() src/qemu/qemu_driver.c | 22 +--- src/security/security_dac.c | 201 ++++++++++++++------------------ src/security/security_manager.h | 13 ++- 3 files changed, 98 insertions(+), 138 deletions(-) -- 2.31.1
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 107 +++++++++++++++--------------------- 1 file changed, 43 insertions(+), 64 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 76bfce7762..956e57247a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -211,11 +211,10 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED, { virSecurityDACChownList *list = opaque; virSecurityManagerMetadataLockState *state; - const char **paths = NULL; + g_autofree const char **paths = NULL; size_t npaths = 0; size_t i; int rv = 0; - int ret = -1; if (list->lock) { paths = g_new0(const char *, list->nItems); @@ -229,7 +228,7 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED, } if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths))) - goto cleanup; + return -1; for (i = 0; i < list->nItems; i++) { virSecurityDACChownItem *item = list->items[i]; @@ -287,12 +286,9 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED, virSecurityManagerMetadataUnlock(list->manager, &state); if (rv < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FREE(paths); - return ret; + return 0; } @@ -438,14 +434,11 @@ virSecurityDACRememberLabel(virSecurityDACData *priv G_GNUC_UNUSED, uid_t uid, gid_t gid) { - char *label = NULL; - int ret = -1; + g_autofree char *label = NULL; label = g_strdup_printf("+%u:+%u", (unsigned int)uid, (unsigned int)gid); - ret = virSecuritySetRememberedLabel(SECURITY_DAC_NAME, path, label); - VIR_FREE(label); - return ret; + return virSecuritySetRememberedLabel(SECURITY_DAC_NAME, path, label); } /** @@ -469,8 +462,7 @@ virSecurityDACRecallLabel(virSecurityDACData *priv G_GNUC_UNUSED, uid_t *uid, gid_t *gid) { - char *label; - int ret = -1; + g_autofree char *label = NULL; int rv; rv = virSecurityGetRememberedLabel(SECURITY_DAC_NAME, path, &label); @@ -481,12 +473,9 @@ virSecurityDACRecallLabel(virSecurityDACData *priv G_GNUC_UNUSED, return 1; if (virParseOwnershipIds(label, uid, gid) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FREE(label); - return ret; + return 0; } static virSecurityDriverStatus @@ -512,8 +501,8 @@ static int virSecurityDACClose(virSecurityManager *mgr) { virSecurityDACData *priv = virSecurityManagerGetPrivateData(mgr); - VIR_FREE(priv->groups); - VIR_FREE(priv->baselabel); + g_clear_pointer(&priv->groups, g_free); + g_clear_pointer(&priv->baselabel, g_free); return 0; } @@ -536,7 +525,7 @@ virSecurityDACPreFork(virSecurityManager *mgr) virSecurityDACData *priv = virSecurityManagerGetPrivateData(mgr); int ngroups; - VIR_FREE(priv->groups); + g_clear_pointer(&priv->groups, g_free); priv->ngroups = 0; if ((ngroups = virGetGroupList(priv->user, priv->group, &priv->groups)) < 0) @@ -1500,8 +1489,8 @@ virSecurityDACSetChardevLabelHelper(virSecurityManager *mgr, virSecurityDACData *priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDef *seclabel; virSecurityDeviceLabelDef *chr_seclabel = NULL; - char *in = NULL, *out = NULL; - int ret = -1; + g_autofree char *in = NULL; + g_autofree char *out = NULL; uid_t user; gid_t group; @@ -1529,9 +1518,11 @@ virSecurityDACSetChardevLabelHelper(virSecurityManager *mgr, switch ((virDomainChrType)dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(mgr, NULL, - dev_source->data.file.path, - user, group, remember); + if (virSecurityDACSetOwnership(mgr, NULL, + dev_source->data.file.path, + user, group, remember) < 0) { + return -1; + } break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1539,14 +1530,14 @@ virSecurityDACSetChardevLabelHelper(virSecurityManager *mgr, out = g_strdup_printf("%s.out", dev_source->data.file.path); if (virFileExists(in) && virFileExists(out)) { if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, remember) < 0 || - virSecurityDACSetOwnership(mgr, NULL, out, user, group, remember) < 0) - goto done; + virSecurityDACSetOwnership(mgr, NULL, out, user, group, remember) < 0) { + return -1; + } } else if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, user, group, remember) < 0) { - goto done; + return -1; } - ret = 0; break; case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -1558,10 +1549,10 @@ virSecurityDACSetChardevLabelHelper(virSecurityManager *mgr, * and passed via FD */ if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, - user, group, remember) < 0) - goto done; + user, group, remember) < 0) { + return -1; + } } - ret = 0; break; case VIR_DOMAIN_CHR_TYPE_SPICEPORT: @@ -1574,14 +1565,10 @@ virSecurityDACSetChardevLabelHelper(virSecurityManager *mgr, case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_NMDM: case VIR_DOMAIN_CHR_TYPE_LAST: - ret = 0; break; } - done: - VIR_FREE(in); - VIR_FREE(out); - return ret; + return 0; } @@ -1604,8 +1591,8 @@ virSecurityDACRestoreChardevLabelHelper(virSecurityManager *mgr, bool recall) { virSecurityDeviceLabelDef *chr_seclabel = NULL; - char *in = NULL, *out = NULL; - int ret = -1; + g_autofree char *in = NULL; + g_autofree char *out = NULL; chr_seclabel = virDomainChrSourceDefGetSecurityLabelDef(dev_source, SECURITY_DAC_NAME); @@ -1621,9 +1608,11 @@ virSecurityDACRestoreChardevLabelHelper(virSecurityManager *mgr, switch ((virDomainChrType)dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL, - dev_source->data.file.path, - recall); + if (virSecurityDACRestoreFileLabelInternal(mgr, NULL, + dev_source->data.file.path, + recall) < 0) { + return -1; + } break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1631,14 +1620,14 @@ virSecurityDACRestoreChardevLabelHelper(virSecurityManager *mgr, in = g_strdup_printf("%s.in", dev_source->data.file.path); if (virFileExists(in) && virFileExists(out)) { if (virSecurityDACRestoreFileLabelInternal(mgr, NULL, out, recall) < 0 || - virSecurityDACRestoreFileLabelInternal(mgr, NULL, in, recall) < 0) - goto done; + virSecurityDACRestoreFileLabelInternal(mgr, NULL, in, recall) < 0) { + return -1; + } } else if (virSecurityDACRestoreFileLabelInternal(mgr, NULL, dev_source->data.file.path, recall) < 0) { - goto done; + return -1; } - ret = 0; break; case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -1646,9 +1635,8 @@ virSecurityDACRestoreChardevLabelHelper(virSecurityManager *mgr, virSecurityDACRestoreFileLabelInternal(mgr, NULL, dev_source->data.nix.path, recall) < 0) { - goto done; + return -1; } - ret = 0; break; case VIR_DOMAIN_CHR_TYPE_NULL: @@ -1661,14 +1649,10 @@ virSecurityDACRestoreChardevLabelHelper(virSecurityManager *mgr, case VIR_DOMAIN_CHR_TYPE_SPICEPORT: case VIR_DOMAIN_CHR_TYPE_NMDM: case VIR_DOMAIN_CHR_TYPE_LAST: - ret = 0; break; } - done: - VIR_FREE(in); - VIR_FREE(out); - return ret; + return 0; } @@ -2378,8 +2362,7 @@ virSecurityDACGetProcessLabelInternal(pid_t pid, virSecurityLabelPtr seclabel) { struct stat sb; - char *path = NULL; - int ret = -1; + g_autofree char *path = NULL; VIR_DEBUG("Getting DAC user and group on process '%d'", pid); @@ -2389,16 +2372,12 @@ virSecurityDACGetProcessLabelInternal(pid_t pid, virReportSystemError(errno, _("unable to get uid and gid for PID %d via procfs"), pid); - goto cleanup; + return -1; } g_snprintf(seclabel->label, VIR_SECURITY_LABEL_BUFLEN, "+%u:+%u", (unsigned int)sb.st_uid, (unsigned int)sb.st_gid); - ret = 0; - - cleanup: - VIR_FREE(path); - return ret; + return 0; } #elif defined(__FreeBSD__) static int -- 2.31.1
Introduce a function that frees individual items on the chown list and declare and use g_autoptr() for it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 956e57247a..70617759c9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -89,6 +89,18 @@ struct _virSecurityDACChownList { virThreadLocal chownList; +static void +virSecurityDACChownItemFree(virSecurityDACChownItem *item) +{ + if (!item) + return; + + g_free(item->path); + g_free(item); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecurityDACChownItem, virSecurityDACChownItemFree); + static int virSecurityDACChownListAppend(virSecurityDACChownList *list, const char *path, @@ -98,15 +110,11 @@ virSecurityDACChownListAppend(virSecurityDACChownList *list, bool remember, bool restore) { - int ret = -1; - char *tmp = NULL; - virSecurityDACChownItem *item = NULL; + g_autoptr(virSecurityDACChownItem) item = NULL; item = g_new0(virSecurityDACChownItem, 1); - tmp = g_strdup(path); - - item->path = g_steal_pointer(&tmp); + item->path = g_strdup(path); item->src = src; item->uid = uid; item->gid = gid; @@ -114,13 +122,9 @@ virSecurityDACChownListAppend(virSecurityDACChownList *list, item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FREE(tmp); - VIR_FREE(item); - return ret; + return 0; } static void @@ -132,10 +136,8 @@ virSecurityDACChownListFree(void *opaque) if (!list) return; - for (i = 0; i < list->nItems; i++) { - g_free(list->items[i]->path); - g_free(list->items[i]); - } + for (i = 0; i < list->nItems; i++) + virSecurityDACChownItemFree(list->items[i]); g_free(list->items); virObjectUnref(list->manager); g_free(list); -- 2.31.1
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 70617759c9..6b8ff5cdef 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -143,6 +143,8 @@ virSecurityDACChownListFree(void *opaque) g_free(list); } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecurityDACChownList, virSecurityDACChownListFree); + /** * virSecurityDACTransactionAppend: @@ -552,10 +554,9 @@ virSecurityDACPreFork(virSecurityManager *mgr) static int virSecurityDACTransactionStart(virSecurityManager *mgr) { - virSecurityDACChownList *list; + g_autoptr(virSecurityDACChownList) list = NULL; - list = virThreadLocalGet(&chownList); - if (list) { + if (virThreadLocalGet(&chownList)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Another relabel transaction is already started")); return -1; @@ -568,9 +569,9 @@ virSecurityDACTransactionStart(virSecurityManager *mgr) if (virThreadLocalSet(&chownList, list) < 0) { virReportSystemError(errno, "%s", _("Unable to set thread local variable")); - virSecurityDACChownListFree(list); return -1; } + list = NULL; return 0; } @@ -601,21 +602,20 @@ virSecurityDACTransactionCommit(virSecurityManager *mgr G_GNUC_UNUSED, pid_t pid, bool lock) { - virSecurityDACChownList *list; + g_autoptr(virSecurityDACChownList) list = NULL; int rc; - int ret = -1; list = virThreadLocalGet(&chownList); if (!list) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No transaction is set")); - goto cleanup; + return -1; } if (virThreadLocalSet(&chownList, NULL) < 0) { virReportSystemError(errno, "%s", _("Unable to clear thread local variable")); - goto cleanup; + return -1; } list->lock = lock; @@ -628,7 +628,7 @@ virSecurityDACTransactionCommit(virSecurityManager *mgr G_GNUC_UNUSED, if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR) pid = -1; else - goto cleanup; + return -1; } } @@ -640,12 +640,9 @@ virSecurityDACTransactionCommit(virSecurityManager *mgr G_GNUC_UNUSED, } if (rc < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virSecurityDACChownListFree(list); - return ret; + return 0; } /** @@ -657,7 +654,7 @@ virSecurityDACTransactionCommit(virSecurityManager *mgr G_GNUC_UNUSED, static void virSecurityDACTransactionAbort(virSecurityManager *mgr G_GNUC_UNUSED) { - virSecurityDACChownList *list; + g_autoptr(virSecurityDACChownList) list = NULL; list = virThreadLocalGet(&chownList); if (!list) @@ -665,7 +662,6 @@ virSecurityDACTransactionAbort(virSecurityManager *mgr G_GNUC_UNUSED) if (virThreadLocalSet(&chownList, NULL) < 0) VIR_DEBUG("Unable to clear thread local variable"); - virSecurityDACChownListFree(list); } -- 2.31.1
The virSecurityDACSetOwnershipInternal() has two callers and in both the private data (@priv) is obtained via virSecurityManagerGetPrivateData(). But in case of DAC driver the private data can never be NULL. This is because the private data is allocated in virSecurityManagerNewDriver() according to .privateDataLen attribute of secdriver. In case of DAC driver the attribute is set to sizeof(virSecurityDACData). NB, no other function within DAC driver checks for !priv. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 6b8ff5cdef..b6323a7df1 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -677,7 +677,7 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ - if (priv && src && priv->chownCallback) { + if (src && priv->chownCallback) { rc = priv->chownCallback(src, uid, gid); /* here path is used only for error messages */ path = NULLSTR(src->path); -- 2.31.1
The virSecurityDACSetOwnershipInternal() function accepts two arguments (among others): @path and @src. The idea being that in some cases @path is NULL and @src is not and then @path is filled from @src->path. However, this is done in both callers already (because of seclabel remembering/recall). Therefore, this code in virSecurityDACSetOwnershipInternal() is dead, effectively. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b6323a7df1..e2a6461375 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -688,15 +688,8 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, } else { struct stat sb; - if (!path) { - if (!src || !src->path) - return 0; - - if (!virStorageSourceIsLocalStorage(src)) - return 0; - - path = src->path; - } + if (!path) + return 0; if (stat(path, &sb) < 0) { virReportSystemError(errno, _("unable to stat: %s"), path); -- 2.31.1
On 6/17/21 12:42 PM, Michal Privoznik wrote:
The virSecurityDACSetOwnershipInternal() function accepts two arguments (among others): @path and @src. The idea being that in some cases @path is NULL and @src is not and then @path is filled from @src->path. However, this is done in both callers already (because of seclabel remembering/recall). Therefore, this code in virSecurityDACSetOwnershipInternal() is dead, effectively.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
Similar dead code is also repeated in debug prints: VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", NULLSTR(src ? src->path : path), (long)uid, (long)gid); For the next time you get in a cleaning mood ;) Jano
As shown in the previous commit, @path can be NULL. However, in that case @src->path is also NULL. Therefore, trying to "fix" @path to be not NULL is not going to succeed. The real value of NULLSTR() is in providing a non-NULL string for error reporting. Well, that can be done in the error reporting without overwriting argument. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e2a6461375..603d5b98ef 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -679,8 +679,6 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, if (src && priv->chownCallback) { rc = priv->chownCallback(src, uid, gid); - /* here path is used only for error messages */ - path = NULLSTR(src->path); /* on -2 returned an error was already reported */ if (rc == -2) @@ -712,20 +710,20 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, if (errno == EOPNOTSUPP || errno == EINVAL) { VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not " "supported by filesystem", - (long)uid, (long)gid, path); + (long)uid, (long)gid, NULLSTR(path)); } else if (errno == EPERM) { VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not " "permitted", - (long)uid, (long)gid, path); + (long)uid, (long)gid, NULLSTR(path)); } else if (errno == EROFS) { VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not " "possible on readonly filesystem", - (long)uid, (long)gid, path); + (long)uid, (long)gid, NULLSTR(path)); } else { virReportSystemError(errno, _("unable to set user and group to '%ld:%ld' " "on '%s'"), - (long)uid, (long)gid, path); + (long)uid, (long)gid, NULLSTR(path)); return -1; } } -- 2.31.1
I must admit, I have no idea why we build such POSIX dependent code as DAC driver for something such not POSIX as WIN32. Anyway, the code which is supposed to set error is not doing that. The proper way is to mimic what chown() does: On error, -1 is returned, and errno is set to indicate the error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 603d5b98ef..7ba367755a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -700,7 +700,8 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, } #ifdef WIN32 - rc = ENOSYS; + rc = -1; + errno = ENOSYS; #else /* !WIN32 */ rc = chown(path, uid, gid); #endif /* !WIN32 */ -- 2.31.1
The DAC security driver has an option to register a callback that is called instead of chown(). So far QEMU is the only user of this feature and it's used to set labels on non-local disks (like gluster), where exists notion of owners but regular chown() can't be used. However, this callback (if set) is called always, even for local disks. And thus the QEMU's implementation duplicated parts of the DAC driver to deal with chown(). If the DAC driver would call the callback only for non-local disks then the QEMU's callback can be shorter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 22 ++-------------------- src/security/security_dac.c | 6 ++++-- src/security/security_manager.h | 13 ++++++++++--- 3 files changed, 16 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1ee0e7ebc0..235f575901 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -228,31 +228,13 @@ qemuSecurityChownCallback(const virStorageSource *src, uid_t uid, gid_t gid) { - struct stat sb; int save_errno = 0; int ret = -1; int rv; g_autoptr(virStorageSource) cpy = NULL; - if (virStorageSourceIsLocalStorage(src)) { - /* use direct chown for local files so that the file doesn't - * need to be initialized */ - if (!src->path) - return 0; - - if (stat(src->path, &sb) >= 0) { - if (sb.st_uid == uid && - sb.st_gid == gid) { - /* It's alright, there's nothing to change anyway. */ - return 0; - } - } - - if (chown(src->path, uid, gid) < 0) - return -1; - - return 0; - } + if (virStorageSourceIsLocalStorage(src)) + return -3; if ((rv = virStorageSourceSupportsSecurityDriver(src)) <= 0) return rv; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 7ba367755a..4909107fcc 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -672,7 +672,7 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, uid_t uid, gid_t gid) { - int rc; + int rc = 0; /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ @@ -683,7 +683,9 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, /* on -2 returned an error was already reported */ if (rc == -2) return -1; - } else { + } + + if (rc == 0 || rc == -3) { struct stat sb; if (!path) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index b5c81e9d98..57047ccb13 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -53,9 +53,16 @@ int virSecurityManagerStackAddNested(virSecurityManager *stack, * @uid: target uid * @gid: target gid * - * A function callback to chown image files described by the disk source struct - * @src. The callback shall return 0 on success, -1 on error and errno set (no - * libvirt error reported) OR -2 and a libvirt error reported. */ + * A function callback to chown image files described by the disk + * source struct @src. The callback can decide to skip given @src + * and thus let DAC driver chown the file instead (signalled by + * returning -3). + * + * Returns: 0 on success, + * -1 on error and errno set (no libvirt error reported), + * -2 and a libvirt error reported. + * -3 if callback did not handle chown + */ typedef int (*virSecurityManagerDACChownCallback)(const virStorageSource *src, uid_t uid, -- 2.31.1
On 6/17/21 12:42 PM, Michal Privoznik wrote:
I've started looking at how we could fix the following bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1786222
Long story short, we are not using just async signal safe functions in a forked off thread that runs chown() from within the domain's private namespace. Hence, we ran into a deadlock (although the case of the bug the deadlock is in glibc not gluster).
So far I don't have any fix, but I have couple of cleanups.
Michal Prívozník (8): security_dac: Use g_autofree security_dac: Introduce virSecurityDACChownItemFree() security_dac: Introduce g_autoptr for virSecurityDACChownList security_dac: Don't check for !priv in virSecurityDACSetOwnershipInternal() virSecurityDACSetOwnershipInternal: Drop dead code virSecurityDACSetOwnershipInternal: Don't overwrite @path argument virSecurityDACSetOwnershipInternal: Fix WIN32 code qemu: Deduplicate code in qemuSecurityChownCallback()
src/qemu/qemu_driver.c | 22 +--- src/security/security_dac.c | 201 ++++++++++++++------------------ src/security/security_manager.h | 13 ++- 3 files changed, 98 insertions(+), 138 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jano Tomko -
Michal Privoznik