[PATCH 6/6] security: Try harder to run transactions

[Pavel Mores]

On Wed, Mar 18, 2020 at 06:32:16PM +0100, Michal Privoznik wrote:
> When a QEMU process dies in the middle of a hotplug, then we fail
> to restore the seclabels on the device. The problem is that if
> the thread doing hotplug locks the domain object first and thus
> blocks the thread that wants to do qemuProcessStop(), the
> seclabel cleanup code will see vm->pid still set and mount
> namespace used and therefore try to enter the namespace
> represented by the PID. But the PID is gone really and thus
> entering will fail and no restore is done. What we can do is to
> try enter the namespace (if requested to do so) but if entering
> fails, fall back to no NS mode.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814481
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_dac.c     | 16 ++++++++++++----
>  src/security/security_selinux.c | 16 ++++++++++++----
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 9046b51004..11fff63bc7 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -640,15 +640,23 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>  
>      list->lock = lock;
>  
> +    if (pid != -1) {
> +        rc = virProcessRunInMountNamespace(pid,
> +                                           virSecurityDACTransactionRun,
> +                                           list);
> +        if (rc < 0) {
> +            if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR)
> +                pid = -1;
> +            else
> +                goto cleanup;
> +        }
> +    }
> +
>      if (pid == -1) {
>          if (lock)
>              rc = virProcessRunInFork(virSecurityDACTransactionRun, list);
>          else
>              rc = virSecurityDACTransactionRun(pid, list);
> -    } else {
> -        rc = virProcessRunInMountNamespace(pid,
> -                                           virSecurityDACTransactionRun,
> -                                           list);
>      }
>  
>      if (rc < 0)
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index c94f31727c..8aeb6e45a5 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1163,15 +1163,23 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>  
>      list->lock = lock;
>  
> +    if (pid != -1) {
> +        rc = virProcessRunInMountNamespace(pid,
> +                                           virSecuritySELinuxTransactionRun,
> +                                           list);
> +        if (rc < 0) {
> +            if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR)
> +                pid = -1;
> +            else
> +                goto cleanup;
> +        }
> +    }
> +
>      if (pid == -1) {
>          if (lock)
>              rc = virProcessRunInFork(virSecuritySELinuxTransactionRun, list);
>          else
>              rc = virSecuritySELinuxTransactionRun(pid, list);
> -    } else {
> -        rc = virProcessRunInMountNamespace(pid,
> -                                           virSecuritySELinuxTransactionRun,
> -                                           list);
>      }
>  
>      if (rc < 0)
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores <pmores at redhat.com>