On 27.02.2013 01:23, Eric Blake wrote:
> 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.
I haven't take look at that.
>
>> ---
>> 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.
My idea is; store userName:groupName whenever possible; When one of them
is not accessible, use +NNN instead. The rationale is - whenever a user
or a gropu changes its ID, we will follow it. For instance, a file X has
owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed
from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as
A's ID is 2 not 1. That means I have to do parsing and all the
virAsprintf magic. However, maybe this is not what we want and I should
really remember just numeric values of IDs which has nice tradeoff -
much simpler code.
>
>> + 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?
This in fact is thread safe since the security manager from layer just
above locks itself before calling any security driver's method. So
there's not locking visible here, as we rely on inherited sec driver's
lock. But agreed that turning this into virAtomic does no harm.
Moreover, we don't need to care for it if we ever drop the upper layer lock.
>
>> + 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?
No. Will reorder.
>
>> +
>> +/**
>> + * 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()
Aaah. Okay.
>
>
>> @@ -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.
Yeah, my intent was to keep things consistent with current libvirt
behavior.
>
>
>> @@ -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.
Yep. I don't know how it got there.
>
>
>> @@ -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?
>
Yes. That is what the next two patches do. Moreover, the design may look
slightly like an overkill for now, but it is just for easier
implementation for migrating the internal state of security driver.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list