[...]
>>>
>> Two blank lines between new functions and we like Free instead of Delete
>> unless of course this is something more specific...
>
> This is something more specifc. The TPM emulator writes state into files
> in a dedicated directory. That state would normally be written into
> NVRAM of a TPM so that, after it is powered up again, it has that old
> state again (keys etc.) . We only delete this state and the per-VM
> directory tree when the domain is undefined.
>
Probably just an API naming thing as I was reading forward... Lot of
context to plow through on the first pass for me!
>>
>>
[...]
>>> @@ -27548,6 +27587,10 @@ virDomainDeleteConfig(const char *configDir,
>>> goto cleanup;
>>> }
>>> + /* in case domain is NOT running, remove any TPM storage */
>>> + if (!dom->persistent)
>> Well this has less to do with running and more to do with persistence
>> vs. transient
>
> Iirc this comes from the following: I ran into this case where I had a
> running domain and undefined it. The domain keeps running of course, the
> XML definition is gone. In this case we cannot just delete the TPM
> emulator's storage while it is running but we have to do it later once
> that VM terminates.
>
I wonder if this is something where using the /var/run/libvirt (or
whatever the temporary running domain path is - my brain is fried from
so many reviews)... Using this would cause/allow cleanup when the
process eventually dies
>>
>>> + virDomainTPMDelete(dom->def);
>>> +
[...]
>>> virQEMUDriverConfigNew(bool privileged)
>>> &cfg->nfirmwares) < 0)
>>> goto error;
>>> + if (virGetUserID("tss", &cfg->swtpm_user) < 0)
>>> + cfg->swtpm_user = 0; /* root */
>>> +
>> Something doesn't feel right here especially for unprivileged mode.
>
> I haven't tried it in unprivileged mode. Not sure how to invoke it even...
>
-c qemu:///session
>>
>> If there isn't a "tss" user, then are things configured correctly?
>
> There are only two choice, one is tss and the other one is root.
>
>>
>> Should we follow the other callers to virGetUserID and set to -1 instead?
>
> Set swtpm_user = -1; ? What happens when we need to actually use this
> variable then ? Report an error ?
>
When using the virFile* API's if the incoming uid/gid is -1, then it's
not specifically and it IIRC it takes whatever the current default
running mode is. If it is set, then it's set specifically to the value.
Been a while since I traversed through all that code.
>>
>> Still 0 is the default for @cfg members anyway.
>>
[...]
>>> @@ -7510,6 +7514,9 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>>> if (!(vm = qemuDomObjFromDomain(dom)))
>>> return -1;
>>> + if (qemuExtDevicesInitPaths(driver, vm->def) < 0)
>>> + return -1;
>> Again, confused over failure in an undefine path? Or is this just git
>> diff fooling me?
>
> This is for a domain that's being undefined but was never started (which
> would have caused the path initialization) since libvirtd was restarted.
> Again, we need to initialize the paths. If there was a better place to
> put these paths, I'd be curious where that is.
>
Perhaps "stateDir" - it just came to me - hopefully it's what I was
thinking about ;-)
>>
[...]
>>> +{
>>> + struct dirent *ent;
>>> + int ret;
>>> + DIR *dir;
>>> + char *path;
>>> +
>>> + if (virDirOpen(&dir, name) < 0)
>>> + return -1;
>> Hope that @name isn't a file path on an NFS volume - there's some other
>
> That would depend on the configuration of the host
>
>> consumers of chown in this module that you may want to have a look at.
>
> The other consumers stat the file before and check whether chown() is
> necessary. Hm, is that the recommended way. I just try to change all of
> them without looking at the state.
>
The NFS thing is something that came to mind while thinking of uid/gid
mgmt and rootsquash mode (or something like that) which absolutely
causes nightmares w/r/t using root...
>>
>>> +
>>> + while ((ret = virDirRead(dir, &ent, name)) > 0) {
>>> + if (ent->d_type != DT_REG)
>>> + continue;
>>> +
>>> + if (virAsprintf(&path, "%s/%s", name, ent->d_name) < 0) {
>>> + ret = -1;
>>> + break;
>>> + }
>>> + if (chown(path, uid, gid) < 0) {
>> If uid/gid change from non root user X to user Y, then wouldn't this
>> just fail anyway?
>
> Why would it fail if root does that? libvirtd runs as root.
>
Guess I was thinking of session mode or unprivileged mode.
>>
>> Still using the -1, -1 logic like other chown/chmod consumers may
>> actually be a benefit here at least w/r/t not failing.
>
> Not sure what you mean. uid = -1 passed to chown() means, don't change
> it. Same for gid = -1.
>
>
I think I redescribed that a bit above. It wasn't fresh in my mind, but
essentially looking at the various virFile API's that would check
uid/gid vs. -1 and thinking how qemu.conf and storage pool <permissions>
used -1, -1... Again the details escape me at the moment.
>>
[...]
>>> +static int
>>> +virTPMCreateEmulatorStorage(const char *storagepath,
>>> + bool *created,
>>> + uid_t swtpm_user)
>>> +{
>>> + int ret = -1;
>>> + char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath);
>>> +
>>> + if (!swtpmStorageDir)
>>> + return -1;
>>> +
>>> + if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0)
>>> + return -1;
>>> +
>>> + *created = false;
>>> +
>>> + if (!virFileExists(storagepath))
>>> + *created = true;> +
>>> + if (virDirCreate(storagepath, 0700, swtpm_user, swtpm_user,
>> IIRC: We only checked if swtpm_user was a user - we did not check if it
>> was also a gid... You cannot use the same value for both, can you?
>
> You are right. I need a swtpm_group, eh ? On my system tss happens to be
> mapped to 59 for uid and gid.
>
>>
>> Still may want to consider using the -1, -1 because it is handled via
>> the File API's.
>
> -1 uid and -1 gid passed to chown() would mean don't change it.
> One problem is that a user may edit /etc/libvirt/qemu.conf and change
> this line here from root to tss:
>
> swtpm_user = "tss"
>
> In that case all file ownerships have to be adjusted so that the swtpm
> can access the files. That's why I am always adjusting the ownerships
> before swtpm or the other tools are started.
>
I suspect by this time you've dug in a bit more and perhaps have more
details than I can recall at the moment.
>>
[...]
>
> I broke this patch up into several patches. Thanks for looking at it.
>
> Stefan
Thanks... I know it's painful from experience ;-)
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
