Re: [libvirt] [PATCH v3 27/28] security_dac: Move transaction handling up one level

So far the whole transaction handling is done virSecurityDACSetOwnershipInternal(). This needs to change for the sake of security label remembering and locking. Otherwise we would be locking a path when only appending it to transaction list and not when actually relabelling it.

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 65 ++++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 7528d8ba7d..2115e00e07 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -77,12 +77,13 @@ struct _virSecurityDACChownItem { const virStorageSource *src; uid_t uid; gid_t gid; + bool restore; }; typedef struct _virSecurityDACChownList virSecurityDACChownList; typedef virSecurityDACChownList *virSecurityDACChownListPtr; struct _virSecurityDACChownList { - virSecurityDACDataPtr priv; + virSecurityManagerPtr manager; virSecurityDACChownItemPtr *items; size_t nItems; }; @@ -95,7 +96,8 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, const char *path, const virStorageSource *src, uid_t uid, - gid_t gid) + gid_t gid, + bool restore) { int ret = -1; char *tmp = NULL; @@ -111,6 +113,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, item->src = src; item->uid = uid; item->gid = gid; + item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) goto cleanup; @@ -159,25 +162,29 @@ static int virSecurityDACTransactionAppend(const char *path, const virStorageSource *src, uid_t uid, - gid_t gid) + gid_t gid, + bool restore) { virSecurityDACChownListPtr list = virThreadLocalGet(&chownList); if (!list) return 0; - if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0) + if (virSecurityDACChownListAppend(list, path, src, uid, gid, restore) < 0) return -1; return 1; } -static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, - const virStorageSource *src, - const char *path, - uid_t uid, - gid_t gid); +static int virSecurityDACSetOwnership(virSecurityManagerPtr mgr, + const virStorageSource *src, + const char *path, + uid_t uid, + gid_t gid); +static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, + const virStorageSource *src, + const char *path); /** * virSecurityDACTransactionRun: * @pid: process pid @@ -201,11 +208,16 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, virSecurityDACChownItemPtr item = list->items[i]; /* TODO Implement rollback */

- if (virSecurityDACSetOwnershipInternal(list->priv, - item->src, - item->path, - item->uid, - item->gid) < 0) + if ((!item->restore && + virSecurityDACSetOwnership(list->manager, + item->src, + item->path, + item->uid, + item->gid) < 0) || + (item->restore && + virSecurityDACRestoreFileLabelInternal(list->manager, + item->src, + item->path) < 0))

return -1; } @@ -455,7 +467,6 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) static int virSecurityDACTransactionStart(virSecurityManagerPtr mgr) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityDACChownListPtr list; list = virThreadLocalGet(&chownList); @@ -468,7 +479,7 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) if (VIR_ALLOC(list) < 0) return -1; - list->priv = priv; + list->manager = mgr; if (virThreadLocalSet(&chownList, list) < 0) { virReportSystemError(errno, "%s", @@ -558,11 +569,6 @@ 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 ((rc = virSecurityDACTransactionAppend(path, src, uid, gid)) < 0) - return -1; - else if (rc > 0) - return 0; - if (priv && src && priv->chownCallback) { rc = priv->chownCallback(src, uid, gid); /* here path is used only for error messages */ @@ -631,11 +637,20 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); struct stat sb; + int rc; if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) path = src->path; + /* Be aware that this function might run in a separate process. + * Therefore, any driver state changes would be thrown away. */ + + if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid, false)) < 0) + return -1; + else if (rc > 0) + return 0; + if (path) { if (stat(path, &sb) < 0) { virReportSystemError(errno, _("unable to stat: %s"), path); @@ -667,6 +682,14 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, virStorageSourceIsLocalStorage(src)) path = src->path; + /* Be aware that this function might run in a separate process. + * Therefore, any driver state changes would be thrown away. */ + + if ((rv = virSecurityDACTransactionAppend(path, src, uid, gid, true)) < 0) + return -1; + else if (rv > 0) + return 0; + if (path) { rv = virSecurityDACRecallLabel(priv, path, &uid, &gid); if (rv < 0)