[libvirt] [PATCH v3 03/13] security: Always spawn process for transactions

[John Ferlan]

On 11/9/18 7:42 AM, Michal Privoznik wrote:
> On 11/08/2018 11:45 PM, John Ferlan wrote:
>>
>>
>> On 10/17/18 5:06 AM, Michal Privoznik wrote:
>>> In the next commit the virSecurityManagerMetadataLock() is going
>>> to be turned thread unsafe. Therefore, we have to spawn a
>>> separate process for it. Always.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/security/security_dac.c     | 2 +-
>>>  src/security/security_selinux.c | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> Based slightly upon the KVM Forum presentation detailing some
>> performance issues related to the usage of virFork for
>> virQEMUCapsIsValid and calls to virFileAccessibleAs:
>>
>> https://www.redhat.com/archives/libvir-list/2018-October/msg01333.html
>>
>> Given that this patch would thus virFork "always", what kind of impact
>> could that have? Have you been able to do any performance profiling?
>> What would cause a single round of SELinux & DAC settings?
> 
> This is what I was discussing with Daniel. Although I can't recall
> where. Anyway, fork() is expensive sure, but my argumentation in
> previous versions was that we are doing it already anyway. Namespaces
> are enabled by default and unless users turned them off this adds no new
> fork(). Only if namespaces are turned off then there is new fork(). At
> any rate, this is one fork per one secdriver call. It's not one fork per
> path (which would be horrible).
> 

I too recall seeing the convo w/ Daniel, but couldn't find it in the
recent patches... I wonder if it was on internal IRC.

I did do a small amount of profiling before I asked, but my config
doesn't have some of the qemu_command qemuSecuritySet* calls for sockets
and tapfd's that are outside the batched qemuSecuritySetAllLabel. As an
aside, qemuSecurityDomainSetPathLabel strays from the normal
qemuSecurity{Set|Restore}* nomenclature (/me being grumpy about searches
for call patterns and would gladly R-by a qemuSecuritySetDomain* patch
as well as one that describes the functionality including the well
there's no need to run the Restore because these are domain transient
things that get removed anyway ;-)).

Anyway, currently it's just a "handful" of calls, but some in areas that
we seem to get flack on for. If we can get ahead of that flack because
we know what we're about to do w/r/t virFork that'd be good.

>>
>> I know in an earlier patch I wasn't including performance of virFork
>> because I believed that the only time it would be used would be for
>> metadata locking when (re)labeling would be batched and for that case
>> the "one off" virFork would seem reasonable.
> 
> This is still true. There is no extra fork() if you have namespaces
> enabled. Unfortunately, POSIX file locking is fubared and doing fork is
> the least painful option.
> 
>>
>> Since it is possible to turn off the NS code and thus go through the
>> direct call, I think there's "room for it" here too for those singular
>> cases we could use "pid == -1" to indicate direct calls without virFork
>> and "pid == 0" to batch together calls using virProcessRunInFork.
>>
>> That way when you *know* you are batching together more than singular
>> changes, then the caller can choose to run in a fork knowing the
>> possible performance penalty, but with the benefits. For those that are
>> batched we're stuck, but my memory on all the metadata locking paths is
>> fuzzy already.
>>
>> What's here does work, but after that KVM Forum preso I guess we
>> definitely need to pay attention to areas that can be perf bottlenecks
>> for paths that may be really common.
>>
>> Having a way to disable or avoid virFork is something we may just need
>> to have. I'm willing to be convinced otherwise - I'm just "wary" now.
> 
> The metadata locking needs to be there so that the setting seclabels is
> atomic. I mean, so far chown() and setfcon() are atomic. However, there
> will be some xattr related calls around those. Therefore to make the
> whole critical section atomic there has to be a lock:
> 
> lock(file);
> xattr(file);
> chown(file);
> xattr(file);
> unlock(file);
> 
> The xattr() calls set/recall the original owner of the file. I can make
> this configurable, but if there is no locking the only thing libvirt can
> do is chown(), not the xattr() because that wouldn't be atomic. (I'm
> saying only chown(), but it is the same story for setfcon().) Therefore,
> if users disable metadata locking the original owner of the file can't
> be preserved. On the other hand, we can enable it by default and have an
> opt out for cases where it doesn't work - just like we have for
> namespaces. And I believe people did disable them in their early days
> (even though I don't understand why, they were bugfree :-P)
> 

I understand (generically) why we need the lock. I'm OK with it being
enabled by default. That's not the question/ask. Building in a way to
allow disabling usage of virFork and/or metadata lock knowing the
"penalty" or downside to doing so goes beyond bug free or performance,
it's just that "choice" we allow someone to make. You know there are
those out there that will bemoan "choosing" this is as the default. If
they want to disable in order to gain whatever at the cost of something
else, then so be it. In some ways it's a CYA exercise.

John


>>
>>
>>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>>> index da4a6c72fe..580d800bb1 100644
>>> --- a/src/security/security_dac.c
>>> +++ b/src/security/security_dac.c
>>
>> The function description would need an update way to describe this @pid
>> usage which differs from the current description.
>>
>>> @@ -563,7 +563,7 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>>>      }
>>>  
>>>      if ((pid == -1 &&
>>> -         virSecurityDACTransactionRun(pid, list) < 0) ||
>>> +         virProcessRunInFork(virSecurityDACTransactionRun, list) < 0) ||
>>>          (pid != -1 &&
>>>           virProcessRunInMountNamespace(pid,
>>>                                         virSecurityDACTransactionRun,
>>
>> I think I've previously disclosed my dislike of the format, why not:
>>
>>     if (pid > 0) {
>>         rc = virProcessRunInMountNamespace(pid, ...);
>>     } else {
>>         if (pid == -1)
>>             rc = virSecurityDACTransactionRun(-pid, list);
>>         else
>>             rc = virProcessRunInFork(virSecurityDACTransactionRun, list));
>>     }
>>     if (rc < 0)
>>         goto cleanup;
>>
>> to me that's a lot cleaner and doesn't involve trying to look at
>> multiple if statements with ||'s.
> 
> Okay, I'll change this.
> 
>>
>>> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>>> index 467d1e6bfe..0e24b9b3ca 100644
>>> --- a/src/security/security_selinux.c
>>> +++ b/src/security/security_selinux.c
>>> @@ -1104,7 +1104,7 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>>>      }
>>>  
>>>      if ((pid == -1 &&
>>> -         virSecuritySELinuxTransactionRun(pid, list) < 0) ||
>>> +         virProcessRunInFork(virSecuritySELinuxTransactionRun, list) < 0) ||
>>>          (pid != -1 &&
>>>           virProcessRunInMountNamespace(pid,
>>>                                         virSecuritySELinuxTransactionRun,
>>>
>>
>> ditto.
>>
>> John
>>
> 
> Michal
>