[libvirt] [PATCH 3/6] security driver: Introduce transaction APIs

[John Ferlan]

On 12/19/2016 10:57 AM, Michal Privoznik wrote:
> With our new qemu namespace code in place, the relabelling of
> devices is done not as good is it could: a child process is
> spawned, it enters the mount namespace of the qemu process and
> then runs desired API of the security driver.

Extra CR/LF between paragraphs

> Problem with this approach is that internal state transition of
> the security driver done in the child process is not reflected in
> the parent process. While currently it wouldn't matter that much,
> it is fairly easy to forge about that. We should take the extra

s/forge/forget

> step now while this limitation is still freshly in our minds.

s/freshly/fresh

> 
> Three new APIs are introduced here:
>   virSecurityManagerTransactionStart()
>   virSecurityManagerTransactionCommit()
>   virSecurityManagerTransactionAbort()
> 
> The Start() is going to be used to let security driver know that
> we are starting a new transaction. During a transaction no
> security labels are actually touched, but rather recorded and
> only at Commit() phase they are actually updated. Should
> something go wrong Abort() aborts the transaction freeing up all
> memory allocated by transaction.

So what happens if one is halfway through a commit? No chance to
rollback the already committed and we drop the yet to be committed.

I wonder if there should be a Rollback() API as well...  That would mean
Commit would need to provide more context upon return though.

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/libvirt_private.syms        |  3 +++
>  src/security/security_driver.h  |  9 ++++++++
>  src/security/security_manager.c | 38 ++++++++++++++++++++++++++++++++
>  src/security/security_manager.h |  5 +++++
>  src/security/security_stack.c   | 49 +++++++++++++++++++++++++++++++++++++++++

I know qemu doesn't used it, but would _apparmor also benefit from
similar functionality?

What's here is OK, but concerns raised in patch 4 make me pause on ACK.

John

>  5 files changed, 104 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 4c0170c03..400295b7b 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1172,6 +1172,9 @@ virSecurityManagerSetSavedStateLabel;
>  virSecurityManagerSetSocketLabel;
>  virSecurityManagerSetTapFDLabel;
>  virSecurityManagerStackAddNested;
> +virSecurityManagerTransactionAbort;
> +virSecurityManagerTransactionCommit;
> +virSecurityManagerTransactionStart;
>  virSecurityManagerVerify;
>  
>  
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index b48f2aaed..fa65eb359 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -51,6 +51,11 @@ typedef const char *(*virSecurityDriverGetBaseLabel) (virSecurityManagerPtr mgr,
>  
>  typedef int (*virSecurityDriverPreFork) (virSecurityManagerPtr mgr);
>  
> +typedef int (*virSecurityDriverTransactionStart) (virSecurityManagerPtr mgr);
> +typedef int (*virSecurityDriverTransactionCommit) (virSecurityManagerPtr mgr,
> +                                                   pid_t pid);
> +typedef void (*virSecurityDriverTransactionAbort) (virSecurityManagerPtr mgr);
> +
>  typedef int (*virSecurityDomainRestoreDiskLabel) (virSecurityManagerPtr mgr,
>                                                    virDomainDefPtr def,
>                                                    virDomainDiskDefPtr disk);
> @@ -135,6 +140,10 @@ struct _virSecurityDriver {
>  
>      virSecurityDriverPreFork preFork;
>  
> +    virSecurityDriverTransactionStart transactionStart;
> +    virSecurityDriverTransactionCommit transactionCommit;
> +    virSecurityDriverTransactionAbort transactionAbort;
> +
>      virSecurityDomainSecurityVerify domainSecurityVerify;
>  
>      virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel;
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index f98c7d2d1..4b6b4a926 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -235,6 +235,44 @@ virSecurityManagerPostFork(virSecurityManagerPtr mgr)
>      virObjectUnlock(mgr);
>  }
>  
> +
> +int
> +virSecurityManagerTransactionStart(virSecurityManagerPtr mgr)
> +{
> +    int ret = 0;
> +
> +    virObjectLock(mgr);
> +    if (mgr->drv->transactionStart)
> +        ret = mgr->drv->transactionStart(mgr);
> +    virObjectUnlock(mgr);
> +    return ret;
> +}
> +
> +
> +int
> +virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr,
> +                                    pid_t pid)
> +{
> +    int ret = 0;
> +
> +    virObjectLock(mgr);
> +    if (mgr->drv->transactionCommit)
> +        ret = mgr->drv->transactionCommit(mgr, pid);
> +    virObjectUnlock(mgr);
> +    return ret;
> +}
> +
> +
> +void
> +virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr)
> +{
> +    virObjectLock(mgr);
> +    if (mgr->drv->transactionAbort)
> +        mgr->drv->transactionAbort(mgr);
> +    virObjectUnlock(mgr);
> +}
> +
> +
>  void *
>  virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
>  {
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 80fceddc8..bae8493ee 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -76,6 +76,11 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
>  int virSecurityManagerPreFork(virSecurityManagerPtr mgr);
>  void virSecurityManagerPostFork(virSecurityManagerPtr mgr);
>  
> +int virSecurityManagerTransactionStart(virSecurityManagerPtr mgr);
> +int virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr,
> +                                        pid_t pid);
> +void virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr);
> +
>  void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr);
>  
>  const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr);
> diff --git a/src/security/security_stack.c b/src/security/security_stack.c
> index c123931a0..6056ae321 100644
> --- a/src/security/security_stack.c
> +++ b/src/security/security_stack.c
> @@ -137,6 +137,51 @@ virSecurityStackPreFork(virSecurityManagerPtr mgr)
>      return rc;
>  }
>  
> +
> +static int
> +virSecurityStackTransactionStart(virSecurityManagerPtr mgr)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerTransactionStart(item->securityManager) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
> +
> +static int
> +virSecurityStackTransactionCommit(virSecurityManagerPtr mgr,
> +                                  pid_t pid)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    int rc = 0;
> +
> +    for (; item; item = item->next) {
> +        if (virSecurityManagerTransactionCommit(item->securityManager, pid) < 0)
> +            rc = -1;
> +    }
> +
> +    return rc;
> +}
> +
> +
> +static void
> +virSecurityStackTransactionAbort(virSecurityManagerPtr mgr)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +
> +    for (; item; item = item->next)
> +        virSecurityManagerTransactionAbort(item->securityManager);
> +}
> +
> +
>  static int
>  virSecurityStackVerify(virSecurityManagerPtr mgr,
>                         virDomainDefPtr def)
> @@ -612,6 +657,10 @@ virSecurityDriver virSecurityDriverStack = {
>  
>      .preFork                            = virSecurityStackPreFork,
>  
> +    .transactionStart                   = virSecurityStackTransactionStart,
> +    .transactionCommit                  = virSecurityStackTransactionCommit,
> +    .transactionAbort                   = virSecurityStackTransactionAbort,
> +
>      .domainSecurityVerify               = virSecurityStackVerify,
>  
>      .domainSetSecurityDiskLabel         = virSecurityStackSetDiskLabel,
>