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@xxxxxxxxxx>
> ---
> 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,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
