5:42 a.m.
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
5:42 a.m.
New subject: [PATCH 1/8] security_dac: Use g_autofree
Signed-off-by: Michal Privoznik <mprivozn(a)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
5:42 a.m.
New subject: [PATCH 2/8] security_dac: Introduce virSecurityDACChownItemFree()
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(a)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
5:42 a.m.
New subject: [PATCH 3/8] security_dac: Introduce g_autoptr for virSecurityDACChownList
Signed-off-by: Michal Privoznik <mprivozn(a)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
5:42 a.m.
New subject: [PATCH 4/8] security_dac: Don't check for !priv in virSecurityDACSetOwnershipInternal()
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(a)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
5:42 a.m.
New subject: [PATCH 5/8] virSecurityDACSetOwnershipInternal: Drop dead code
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(a)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
7:29 a.m.
New subject: [PATCH 5/8] virSecurityDACSetOwnershipInternal: Drop dead code
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(a)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
5:42 a.m.
New subject: [PATCH 6/8] virSecurityDACSetOwnershipInternal: Don't overwrite @path argument
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(a)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
5:42 a.m.
New subject: [PATCH 7/8] virSecurityDACSetOwnershipInternal: Fix WIN32 code
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(a)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
5:42 a.m.
New subject: [PATCH 8/8] qemu: Deduplicate code in qemuSecurityChownCallback()
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(a)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
7:30 a.m.
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(a)redhat.com>
Jano
1254
days inactive
1254
days old
10 comments
2 participants
participants (2)
-
Jano Tomko -
Michal Privoznik