[libvirt] [PATCH] qemu: Correctly label migration TCP socket passed to qemu
--- src/qemu/qemu_migration.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a84faf6..d787a09 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1484,9 +1484,12 @@ qemuMigrationRun(struct qemud_driver *driver, case MIGRATION_DEST_FD: if (spec->fwdType != MIGRATION_FWD_DIRECT) fd = spec->dest.fd.local; - ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, - spec->dest.fd.qemu); - VIR_FORCE_CLOSE(spec->dest.fd.qemu); + if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec->dest.fd.qemu) == 0) { + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu); + } break; } qemuDomainObjExitMonitorWithDriver(driver, vm); -- 1.7.6.1
On Thu, Aug 25, 2011 at 12:35:57PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_migration.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a84faf6..d787a09 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1484,9 +1484,12 @@ qemuMigrationRun(struct qemud_driver *driver, case MIGRATION_DEST_FD: if (spec->fwdType != MIGRATION_FWD_DIRECT) fd = spec->dest.fd.local; - ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, - spec->dest.fd.qemu); - VIR_FORCE_CLOSE(spec->dest.fd.qemu); + if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec->dest.fd.qemu) == 0) { + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu); + } break; } qemuDomainObjExitMonitorWithDriver(driver, vm);
Okay, checking in the code source, ret is initialized to -1 to that looks correct to me, but a second review would be welcome, it's a tricky part, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/25/2011 04:35 AM, Jiri Denemark wrote:
--- src/qemu/qemu_migration.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a84faf6..d787a09 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1484,9 +1484,12 @@ qemuMigrationRun(struct qemud_driver *driver, case MIGRATION_DEST_FD: if (spec->fwdType != MIGRATION_FWD_DIRECT) fd = spec->dest.fd.local; - ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, - spec->dest.fd.qemu); - VIR_FORCE_CLOSE(spec->dest.fd.qemu);
Unconditional, changed to...
+ if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec->dest.fd.qemu) == 0) { + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu);
...conditional. Oops - you leak the fd if changing the label fails. Needs a v2. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 25, 2011 at 08:16:58 -0600, Eric Blake wrote:
On 08/25/2011 04:35 AM, Jiri Denemark wrote:
--- src/qemu/qemu_migration.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a84faf6..d787a09 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1484,9 +1484,12 @@ qemuMigrationRun(struct qemud_driver *driver, case MIGRATION_DEST_FD: if (spec->fwdType != MIGRATION_FWD_DIRECT) fd = spec->dest.fd.local; - ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, - spec->dest.fd.qemu); - VIR_FORCE_CLOSE(spec->dest.fd.qemu);
Unconditional, changed to...
+ if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec->dest.fd.qemu) == 0) { + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu);
...conditional. Oops - you leak the fd if changing the label fails.
No, there's no leak here. The fd is closed by the caller in any case. Note, we don't close it here if we fail before we ever get to qemuMonitorMigrateToFd. This additional close after qemuMonitorMigrateToFd() is just for better feeling in case migration started to avoid having an fd open which doesn't technically belong to us anymore for the whole time of migration. Jirka
On 08/25/2011 08:30 AM, Jiri Denemark wrote:
- VIR_FORCE_CLOSE(spec->dest.fd.qemu);
Unconditional, changed to...
+ if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec->dest.fd.qemu) == 0) { + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu);
...conditional. Oops - you leak the fd if changing the label fails.
No, there's no leak here. The fd is closed by the caller in any case. Note, we don't close it here if we fail before we ever get to qemuMonitorMigrateToFd. This additional close after qemuMonitorMigrateToFd() is just for better feeling in case migration started to avoid having an fd open which doesn't technically belong to us anymore for the whole time of migration.
Ah, makes sense. Until a child process is using the fd, it doesn't matter if we close early, and you've convinced me that the caller mops up in the cases where we don't close early. Meanwhile, once a child is using the fd, then we don't want to get in the way (especially important on pipes, where having multiple processes open as writers on the same pipe can prevent readers from seeing EOF), so only closing early after spawning the child makes sense. So, ACK to this patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 25, 2011 at 12:35:57PM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_migration.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a84faf6..d787a09 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1484,9 +1484,12 @@ qemuMigrationRun(struct qemud_driver *driver, case MIGRATION_DEST_FD: if (spec->fwdType != MIGRATION_FWD_DIRECT) fd = spec->dest.fd.local; - ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, - spec->dest.fd.qemu); - VIR_FORCE_CLOSE(spec->dest.fd.qemu); + if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec->dest.fd.qemu) == 0) { + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu); + } break; } qemuDomainObjExitMonitorWithDriver(driver, vm);
Based on more discussions with Dan Walsh this needs to be instead calling virSecurityManagerSetProcessFDLabel Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--- Notes: Version 2: - use virSecurityManagerSetProcessFDLabel instead of virSecurityManagerSetImageFDLabel since the correct label for TCP sockets appears to be svirt_t and not svirt_image_t src/qemu/qemu_migration.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a84faf6..8e24199 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1484,9 +1484,12 @@ qemuMigrationRun(struct qemud_driver *driver, case MIGRATION_DEST_FD: if (spec->fwdType != MIGRATION_FWD_DIRECT) fd = spec->dest.fd.local; - ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, - spec->dest.fd.qemu); - VIR_FORCE_CLOSE(spec->dest.fd.qemu); + if (virSecurityManagerSetProcessFDLabel(driver->securityManager, vm, + spec->dest.fd.qemu) == 0) { + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu); + } break; } qemuDomainObjExitMonitorWithDriver(driver, vm); -- 1.7.6.1
On Thu, Aug 25, 2011 at 18:16:17 +0200, Jiri Denemark wrote:
--- Notes: Version 2: - use virSecurityManagerSetProcessFDLabel instead of virSecurityManagerSetImageFDLabel since the correct label for TCP sockets appears to be svirt_t and not svirt_image_t
Apparently this isn't right either. Relabeling a socket of an already established connection is not a good idea since it may even break the connection. I'll post a v3 that labels this in a different way. Jirka
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jiri Denemark