On 09/10/2018 05:36 AM, Michal Privoznik wrote:
> And by "more frequently" I mean always. This is needed so that we
> have a single place where all the paths a thread wants to relabel
> are stored. This enables us to lock them all at once (for
> metadata), do the relabel and unlock at once again.
>
Perhaps the first sentence or so "differently said":
Now that committing transactions using pid == -1 means that we're not
fork()-ing to run the transaction in a specific namespace, we can
utilize the transaction processing semantics in order to start, run a or
multiple commands, and then commit the transaction without being
concerned with other interactions or transactions interrupting the
processing. This will eventually allow us to have a single place where
all the paths... (as you've already written).
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_security.c | 216 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 129 insertions(+), 87 deletions(-)
>
This patch made Coverity unhappy because some of the void callers to
virSecurityManagerTransactionStart or
virSecurityManagerTransactionCommit don't check status:
qemuSecurityStartTPMEmulator (cleanup: processing)
qemuSecurityCleanupTPMEmulator
qemuSecurityRestoreAllLabel
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index c64fbdda38..b538e08616 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -39,9 +39,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
> {
> int ret = -1;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> + pid_t pid = -1;
>
> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> - virSecurityManagerTransactionStart(driver->securityManager) < 0)
> + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
> + pid = vm->pid;
> +
> + if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
> goto cleanup;
I know existing, if a transaction fails to start does it need to be
aborted... I know it doesn't matter by looking at the code, but that
thought could be important later on.
>
> if (virSecurityManagerSetAllLabel(driver->securityManager,
> @@ -50,9 +53,7 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
> priv->chardevStdioLogd) < 0)
> goto cleanup;
>
> - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
> - virSecurityManagerTransactionCommit(driver->securityManager,
> - vm->pid) < 0)
> + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0)
> goto cleanup;
>
> ret = 0;
> @@ -69,16 +70,21 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> - /* In contrast to qemuSecuritySetAllLabel, do not use
> - * secdriver transactions here. This function is called from
> - * qemuProcessStop() which is meant to do cleanup after qemu
> - * process died. If it did do, the namespace is gone as qemu
> - * was the only process running there. We would not succeed
> - * in entering the namespace then. */
> + /* In contrast to qemuSecuritySetAllLabel, do not use vm->pid
> + * here. This function is called from qemuProcessStop() which
> + * is meant to do cleanup after qemu process died. The
> + * domain's namespace is gone as qemu was the only process
> + * running there. We would not succeed in entering the
> + * namespace then. */
> + if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
> + return;
> +
Interesting that in this instance you choose to honor the status, but
for another void function qemuSecurityCleanupTPMEmulator the choice was
to not care. IDC which way this is done, but it should be consistent.
I can also see a reason that if a TransactionStart fails, that we at
least attempt the following rather than just returning. In that case,
I'd say to use the < 0 status as a way to not call TransactionCommit and
perhaps throw a VIR_WARN to indicate that transactions weren't being used.
Probably also want to save/restore LastError around all this so that a
failure in TransactionStart or TransactionCommit doesn't overwrite an
error we already have.
> virSecurityManagerRestoreAllLabel(driver->securityManager,
> vm->def,
> migrated,
> priv->chardevStdioLogd);
> +
> + virSecurityManagerTransactionCommit(driver->securityManager, -1);
If we wrap this in ignore_value, then Coverity will be "happy". Since
the only consumer is ProcessStop, at least we don't have the fear that a
Start succeeds and a Commit fails that we'll be "stuck" the next time we
go to do a Start...
I guess to some degree I wonder if the transaction even matters -
meaning - I think you can stick with the original code so that
Start/Commit failure doesn't mean anything.
I also think this function should be separated from the others since its
adding transaction semantics where they weren't present previously. That
way we can debate need or decide later to revert if necessary...
> }
>
>
[...]
> @@ -454,10 +472,21 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
> int *cmdret)
I almost want to say this is "different" than any of the other functions
which you are converting the existing transaction model. In this
instance you are introducing a transaction model. I think this should be
a separate patch to make it easier for a later decision to revert just
in case that decision needs to be made.
The only current consumer is virSecuritySELinuxSetTPMLabels and it
already has the multiple operation type semantics, so I'm wondering if
transaction semantics are actually necessary.
> {
> int ret = -1;
> + bool transactionStarted = false;
> +
> + if (virSecurityManagerTransactionStart(driver->securityManager) < 0)
> + return -1;
> + transactionStarted = true;
>
> if (virSecurityManagerSetTPMLabels(driver->securityManager,
> - def) < 0)
> + def) < 0) {
> + virSecurityManagerTransactionAbort(driver->securityManager);
> + return -1;
> + }
> +
> + if (virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0)
> goto cleanup;
> + transactionStarted = false;
>
> if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
> def, cmd) < 0)
> @@ -481,8 +510,13 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
> return 0;
>
> cleanup:
We should change this to "error:" because it's not cleanup in normal path...
> + if (!transactionStarted)
> + virSecurityManagerTransactionStart(driver->securityManager);
I think this should be:
if (!transactionStarted) {
if (virSecurityManagerTransactionStart(...) == 0)
transactionStarted = true;
}
> +
> virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
>
Making the subsequent be:
if (transactionStarted) {
ignore_value(virSecurityManagerTransactionCommit(...);
virSecurityManagerTransactionAbort(...);
}
Because if the "second" Start fails, then next ones aren't necessary.
I'd also be inclined to say, why bother with a transaction. If something
in the Set processing failed, I'd assume we're going to get a similar
failure in Restore processing - so let the TPM specific code handle
that. We can document the crap out of it^w^w^w^w, err, ah decision made.
> + virSecurityManagerTransactionCommit(driver->securityManager, -1);
> + virSecurityManagerTransactionAbort(driver->securityManager);
> return ret;
> }
>
> @@ -491,7 +525,12 @@ void
> qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver,
> virDomainDefPtr def)
> {
> + virSecurityManagerTransactionStart(driver->securityManager);
So this is different than qemuSecurityRestoreAllLabel...
Similar to above - we should know if Start fails, because if it does,
then commit and abort won't be happy either...
This should be a separate patch with the StartTPMEmulator...
> +
> virSecurityManagerRestoreTPMLabels(driver->securityManager, def);
> +
> + virSecurityManagerTransactionCommit(driver->securityManager, -1);
> + virSecurityManagerTransactionAbort(driver->securityManager);
> }
>
>
[...]
So, let's do this for any function that is not adding transaction
semantics, but merely modifying existing semantics,
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
But for qemuSecurityRestoreAllLabel, qemuSecurityStartTPMEmulator, and
qemuSecurityCleanupTPMEmulator - we need to extract those and consider
them separately.
Whether you alter the commit message to my suggestion above is your choice.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
