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

[John Ferlan]

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?

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.

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.


> 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.

> 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