6:52 a.m.
While reviewing patch:
http://www.redhat.com/archives/libvir-list/2016-March/msg00194.html
I noted that the order of operations during the code path was slightly
different and could cause issues if the reattachment code was called
first. It would seem that resetting the hostdev label and cgroup prior
to reattaching the device to the host would be a better corollary to
the attachment code path which detaches the devices from the host, then
sets the cgroup, then sets the hostdev label.
John Ferlan (2):
qemu: Restore label before reattach device to host
qemu: Tear down the cgroup before reattach device to host
src/qemu/qemu_hotplug.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
--
2.5.5
6:52 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Restore label before reattach device to host
When a hostdev is attached to the guest (and removed from the host),
the order of operations is call qemuHostdevPreparePCIDevices to remove
the device from the host, call qemuSetupHostdevCgroup to setup the cgroups,
and virSecurityManagerSetHostdevLabel to set the labels.
When the device is removed from the guest, the code didn't use the
reverse order leading to possible issues (especially if the path to
the device no longer exists). This patch will move the call to
virSecurityManagerRestoreHostdevLabel to prior to reattaching the
device to the host.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f580906..fcb86e4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3029,8 +3029,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
char *drivestr = NULL;
- int backend;
- bool is_vfio = false;
VIR_DEBUG("Removing host device %s from domain %p %s",
hostdev->info->alias, vm, vm->def->name);
@@ -3072,10 +3070,16 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
virDomainAuditHostdev(vm, hostdev, "detach", true);
+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ hostdev->source.subsys.u.pci.backend !=
+ VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
+ if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
+ vm->def, hostdev, NULL) < 0)
+ VIR_WARN("Failed to restore host device labelling");
+ }
+
switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- backend = hostdev->source.subsys.u.pci.backend;
- is_vfio = backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
qemuDomainRemovePCIHostDevice(driver, vm, hostdev);
/* QEMU might no longer need to lock as much memory, eg. we just
* detached the last VFIO device, so adjust the limit here */
@@ -3095,12 +3099,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
if (qemuTeardownHostdevCgroup(vm, hostdev) < 0)
VIR_WARN("Failed to remove host device cgroup ACL");
- if (!is_vfio &&
- virSecurityManagerRestoreHostdevLabel(driver->securityManager,
- vm->def, hostdev, NULL) < 0) {
- VIR_WARN("Failed to restore host device labelling");
- }
-
virDomainHostdevDefFree(hostdev);
if (net) {
--
2.5.5
6:52 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Tear down the cgroup before reattach device to host
When a hostdev is attached to the guest (and removed from the host),
the order of operations is call qemuHostdevPreparePCIDevices to remove
the device from the host, call qemuSetupHostdevCgroup to setup the cgroups,
and virSecurityManagerSetHostdevLabel to set the labels.
When the device is removed from the guest, the code didn't use the
reverse order leading to possible issues (especially if the path to
the device no longer exists). This patch will move the call to
qemuTeardownHostdevCgroup to prior to reattaching the device to
the host.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fcb86e4..dead080 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3078,6 +3078,9 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
VIR_WARN("Failed to restore host device labelling");
}
+ if (qemuTeardownHostdevCgroup(vm, hostdev) < 0)
+ VIR_WARN("Failed to remove host device cgroup ACL");
+
switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
qemuDomainRemovePCIHostDevice(driver, vm, hostdev);
@@ -3096,9 +3099,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
break;
}
- if (qemuTeardownHostdevCgroup(vm, hostdev) < 0)
- VIR_WARN("Failed to remove host device cgroup ACL");
-
virDomainHostdevDefFree(hostdev);
if (net) {
--
2.5.5
4:17 p.m.
On 03/28/2016 07:52 AM, John Ferlan wrote:
While reviewing patch:
http://www.redhat.com/archives/libvir-list/2016-March/msg00194.html
I noted that the order of operations during the code path was slightly
different and could cause issues if the reattachment code was called
first. It would seem that resetting the hostdev label and cgroup prior
to reattaching the device to the host would be a better corollary to
the attachment code path which detaches the devices from the host, then
sets the cgroup, then sets the hostdev label.
I've noticed similar issues at other places in the code (teardown not
being the exact mirror image of setup) but didn't touch it because I
didn't want to accidentally introduce regressions.
I wanted to test these patches to verify that the sequence change
doesn't uncover some odd regression, but now I find that hotplug of
type='hostdev' network devices is completely broken (even before
applying your patches), so I'll have to figure out the cause of that
first :-/
9:43 a.m.
On 03/28/2016 05:17 PM, Laine Stump wrote:
On 03/28/2016 07:52 AM, John Ferlan wrote:
> While reviewing patch:
>
> http://www.redhat.com/archives/libvir-list/2016-March/msg00194.html
>
> I noted that the order of operations during the code path was slightly
> different and could cause issues if the reattachment code was called
> first. It would seem that resetting the hostdev label and cgroup prior
> to reattaching the device to the host would be a better corollary to
> the attachment code path which detaches the devices from the host, then
> sets the cgroup, then sets the hostdev label.
I've noticed similar issues at other places in the code (teardown not
being the exact mirror image of setup) but didn't touch it because I
didn't want to accidentally introduce regressions.
I wanted to test these patches to verify that the sequence change
doesn't uncover some odd regression, but now I find that hotplug of
type='hostdev' network devices is completely broken (even before
applying your patches), so I'll have to figure out the cause of that
first :-/
I was finally able to try this out (extensively, because I've been
testing the fixes for the other problems I found on top of your
patches). Everything checks out and the logic makes sense, so ACK.
3184
days inactive
3191
days old
4 comments
2 participants
participants (2)
-
John Ferlan -
Laine Stump