Re: [libvirt] [PATCH 2/2] qemu: Don't overwrite security labels
On 11.06.2012 18:54, Eric Blake wrote:
On 06/11/2012 08:29 AM, Michal Privoznik wrote:
> Currently, if qemuProcessStart fail at some point, e.g. because
> domain being started wants a PCI/USB device already assigned to
> a different domain, we jump to cleanup label where qemuProcessStop
> is performed. This unconditionally calls virSecurityManagerRestoreAllLabel
> which is wrong because the other domain is still using those devices.
>
> However, once we successfully label all devices/paths in
> qemuProcessStart() from that point on, we have to perform a rollback
> on failure - that is - we have to virSecurityManagerRestoreAllLabel.
> ---
> src/qemu/qemu_process.c | 12 ++++++++----
> src/qemu/qemu_process.h | 3 ++-
> 2 files changed, 10 insertions(+), 5 deletions(-)
Double-negative logic. But I guess we're stuck with it, as the default
of 'flags==0' must imply the relabel.
> @@ -3984,9 +3987,10 @@ void qemuProcessStop(struct qemud_driver *driver,
> }
>
> /* Reset Security Labels */
> - virSecurityManagerRestoreAllLabel(driver->securityManager,
> - vm->def,
> - flags & VIR_QEMU_PROCESS_STOP_MIGRATED);
> + if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL))
Took me a couple reads to convince myself that I couldn't come up with
any nicer wording of this condition without breaking defaults.
ACK.
Yeah, it is quite confusing, therefore I am squashing in some comments
before pushing:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0309bfb..5f3aa99 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3281,7 +3281,7 @@ int qemuProcessStart(virConnectPtr conn,
int i;
char *nodeset = NULL;
char *nodemask = NULL;
- unsigned int stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+ unsigned int stop_flags;
/* Okay, these are just internal flags,
* but doesn't hurt to check */
@@ -3289,6 +3289,12 @@ int qemuProcessStart(virConnectPtr conn,
VIR_QEMU_PROCESS_START_PAUSED |
VIR_QEMU_PROCESS_START_AUTODESROY, -1);
+ /* From now on until domain security labeling is done:
+ * if any operation fails and we goto cleanup, we must not
+ * restore any security label as we would overwrite labels
+ * we did not set. */
+ stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+
hookData.conn = conn;
hookData.vm = vm;
hookData.driver = driver;
@@ -3632,6 +3638,10 @@ int qemuProcessStart(virConnectPtr conn,
vm->def, stdin_path) < 0)
goto cleanup;
+ /* Security manager labeled all devices, therefore
+ * if any operation from now on fails and we goto cleanup,
+ * where virSecurityManagerRestoreAllLabel() is called
+ * (hidden under qemuProcessStop) we need to restore labels. */
stop_flags &= ~VIR_QEMU_PROCESS_STOP_NO_RELABEL;
if (stdin_fd != -1) {
@@ -3986,7 +3996,7 @@ void qemuProcessStop(struct qemud_driver *driver,
VIR_FREE(xml);
}
- /* Reset Security Labels */
+ /* Reset Security Labels unless caller don't want us to */
if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL))
virSecurityManagerRestoreAllLabel(driver->securityManager,
vm->def,
---
And pushed now. Thanks for review!
Michal