Re: [libvirt] [PATCH RFC 1/3] security_dac: Remember owner prior chown() and restore on relabel

On 02/26/2013 09:08 AM, Michal Privoznik wrote: > Currently, if we label a file to match qemu process DAC label, we > do not store the original owner anywhere. So when relabeling > back, the only option we have is to relabel to root:root > which is obviously wrong. > > However, bare remembering is not enough. We need to keep track of > how many times we labeled a file so only the last restore > chown()-s file back to the original owner. Definitely important for a read-only file shared by more than one domain. > > In order to not pollute domain XML, this info is kept in driver's > private data in a hash table with path being key and pair > <oldLabel, refcount> being value. Makes sense. Have you looked at what it would take to use ACLs to grant access to qemu without having to do a full-blown chown? That would also need to use the hash table to undo the ACL at the end of the day, and we would need to fall back to chown() on file systems where ACL doesn't work, but it certainly sounds like that would be sharing some of the work in this patch.

> --- > src/security/security_dac.c | 351 ++++++++++++++++++++++++++++++++++------- > src/security/security_driver.h | 3 + > 2 files changed, 296 insertions(+), 58 deletions(-) Couple of questions below: > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index 0b274b7..4b8f0a2 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -42,8 +42,121 @@ struct _virSecurityDACData { > uid_t user; > gid_t group; > bool dynamicOwnership; > + virHashTablePtr oldOwners; /* to hold pairs <path, virOldLabelPtr> */ > }; > > +struct _virOldLabel { > + char *owner; Are you really planning on storing a string uid:gid? Wouldn't it be simpler to store a uid_t and gid_t as read from struct stat, as long as the data is only in memory? And when storing the data to disk in XML to survive libvirtd restarts, it seems like storing two attributes user='nnn' group='nnn' is nicer than storing one attribute owner='+nnn:+nnn' that requires further parsing back into user and group.

> + int refCount; > +}; > + > +static void virOldLabelFree(virOldLabelPtr label) Line break after void. > +/** > + * virSecurityDACRememberLabel: > + * @priv: private DAC driver data > + * @path: path which is about to be relabelled > + * > + * Store the original DAC label of @path. > + * Returns: number of references of @path, or -1 on error > + */ > +static int > +virSecurityDACRememberLabel(virSecurityDACDataPtr priv, > + const char *path) > +{ > + struct stat sb; > + virOldLabelPtr oldLabel = NULL; > + char *user = NULL, *group = NULL; > + > + oldLabel = virHashLookup(priv->oldOwners, path); > + > + if (oldLabel) { > + /* just increment ref counter */ > + oldLabel->refCount++; Is this threadsafe, or do we need to use virAtomic?

> + goto cleanup; > + } > + > + if (stat(path, &sb) < 0) { > + virReportSystemError(errno, _("Unable to stat %s"), path); > + goto cleanup; > + } > + > + user = virGetUserName(sb.st_uid); > + group = virGetGroupName(sb.st_gid); > + if ((!user && (virAsprintf(&user, "+%ld", (long) sb.st_uid) < 0)) || > + (!group && (virAsprintf(&group, "+%ld", (long) sb.st_gid) < 0)) || Given Philipp's recent series, it's probably better to mix %u and (unsigned), rather than %ld and (long) (that is, we've already asserted that uid_t fits within an int, and that unsigned is more likely than signed to represent all users except for the sentinel of -1, and -1 is not going to appear as stat() results). Again, if your struct has a uid_t and gid_t instead of a char*, then you don't have to convert to string here. > + (VIR_ALLOC(oldLabel) < 0) || > + (virAsprintf(&oldLabel->owner, "%s:%s", user, group) < 0)) { Worse, you are printing into two temporaries only to create yet another malloc'd string. If you do keep a char*, why not just: virAsprintf(&oldLabel->owner, "+%u:+%u", (unsigned) sb.st_uid, (unsigned) sb.st_gid); > + > +static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); Any reason you can't topologically sort that function to be first, and avoid a forward declaration?

> + > +/** > + * virSecurityDACGetRememberedLabel: > + * @priv: private DAC driver data > + * @path: path which we want to restore label on > + * @user: original owner of @path > + * @group: original owner of @path > + * > + * Decrements reference counter on @path. If this was the last > + * reference, we need to restore the original label, in which > + * case @user and @group are updated. > + * Returns: the number of references of @path > + * 0 if we need to restore the label > + * -1 on error > + */ > +static int > +virSecurityDACGetRememberedLabel(virSecurityDACDataPtr priv, > + const char *path, > + uid_t *user, > + gid_t *group) > +{ > + int ret = -1; > + virOldLabelPtr oldLabel = NULL; > + > + oldLabel = virHashLookup(priv->oldOwners, path); > + if (!oldLabel) > + goto cleanup; > + > + ret = --oldLabel->refCount; Thread safety again. virAtomic? > + > + if (!ret) { > + ret = parseIds(oldLabel->owner, user, group); If the hash value struct has uid_t and gid_t fields, then no parsing is needed here. > static int > -virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) > +virSecurityDACOpen(virSecurityManagerPtr mgr) > { > - return 0; > + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > + > + priv->oldOwners = virHashCreate(0, hashDataFree); > + return priv->oldOwners ? 0 : -1; Caller doesn't report an error, so you need a virReportOOMError()

> @@ -382,26 +503,14 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, > virDomainDiskDefPtr disk, > int migrated) > { > + int ret; > virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > + uid_t user = 0; > + gid_t group = 0; I prefer -1 when initializing an unknown uid or gid; 0 has special meaning, and there are cases where we really do want to restore to uid 0.

> @@ -411,7 +520,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, > */ > if (migrated) { > int rc = virStorageFileIsSharedFS(disk->src); > - if (rc < 0) > + if (rc > 0) > return -1; > if (rc == 1) { > VIR_DEBUG("Skipping image label restore on %s because FS is shared", This seems like a spurious change.

> @@ -752,13 +956,29 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, > mgr) < 0) > rc = -1; > > - if (def->os.kernel && > - virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) > - rc = -1; > + if (def->os.kernel) { > + i = virSecurityDACGetRememberedLabel(priv, def->os.kernel, &user, &group); > + if (i < 0) > + rc = -1; Using 'i' for a non-iteration is unconventional; maybe 'rc' would be a better name. > @@ -815,11 +1035,13 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, > return -1; > > if (def->os.kernel && > - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) > + ((virSecurityDACRememberLabel(priv, def->os.kernel) < 0) || > + (virSecurityDACSetOwnership(def->os.kernel, user, group) < 0))) Indentation looks off; and also redundant (): if (def->os.kernel && (virSecurityDACRememberLabel(priv, def->os.kernel) < 0 || virSecurityDACSetOwnership(def->os.kernel, user, group) < 0)) > @@ -835,7 +1057,8 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, > gid_t group; > virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > > - if (virSecurityDACGetImageIds(def, priv, &user, &group)) > + if (virSecurityDACGetImageIds(def, priv, &user, &group) || > + (virSecurityDACRememberLabel(priv, savefile) < 0)) Redundant () How is this information is preserved across a libvirtd restart? You'll need code that outputs private XML on the save side, then parses it back on the load side - is that later in the series?