8:56 a.m.
As discussed here [1], it's unsafe to allow /dev/vfio/vfio to all the domains
(even those not doing PCI assignemnt). The same goes for /dev/dri/*.
1: https://www.redhat.com/archives/libvir-list/2017-February/msg00267.html
Michal Privoznik (7):
qemu_cgroup: Kill qemuSetupHostUSBDeviceCgroup
qemu_cgroup: Kill qemuSetupHostSCSIDeviceCgroup
qemu_cgroup: Kill qemuSetupHostSCSIVHostDeviceCgroup
qemuSetupHostdevCgroup: Use qemuDomainGetHostdevPath
qemuDomainGetHostdevPath: Create /dev/vfio/vfio iff needed
qemuDomainGetHostdevPath: Report /dev/vfio/vfio less frequently
qemu: Allow /dev/dri/render* for virgl domains
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_cgroup.c | 311 +++++++++++--------------------------
src/qemu/qemu_domain.c | 207 ++++++++++++++++++++----
src/qemu/qemu_domain.h | 7 +
src/qemu/test_libvirtd_qemu.aug.in | 1 -
5 files changed, 274 insertions(+), 254 deletions(-)
--
2.11.0
8:56 a.m.
New subject: [libvirt] [PATCH 1/7] qemu_cgroup: Kill qemuSetupHostUSBDeviceCgroup
There's no need for this function. Currently it is passed as a
callback to virUSBDeviceFileIterate(). However, USB devices have
just one file path. Therefore we can mimic approach used in
qemuDomainGetHostdevPath() to get path and call
virCgroupAllowDevicePath() directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 6c90d46d1..7302c43ee 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -260,23 +260,6 @@ qemuSetupInputCgroup(virDomainObjPtr vm,
}
-static int
-qemuSetupHostUSBDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
- const char *path,
- void *opaque)
-{
- virDomainObjPtr vm = opaque;
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret;
-
- VIR_DEBUG("Process path '%s' for USB device", path);
- ret = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", ret == 0);
-
- return ret;
-}
-
static int
qemuSetupHostSCSIDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
const char *path,
@@ -333,6 +316,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
virSCSIDevicePtr scsi = NULL;
virSCSIVHostDevicePtr host = NULL;
char *path = NULL;
+ int rv;
/* currently this only does something for PCI devices using vfio
* for device assignment, but it is called for *all* hostdev
@@ -347,8 +331,6 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- int rv;
-
pci = virPCIDeviceNew(pcisrc->addr.domain,
pcisrc->addr.bus,
pcisrc->addr.slot,
@@ -381,13 +363,15 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
goto cleanup;
}
- /* oddly, qemuSetupHostUSBDeviceCgroup doesn't ever
- * reference the usb object we just created
- */
- if (virUSBDeviceFileIterate(usb, qemuSetupHostUSBDeviceCgroup,
- vm) < 0) {
+ if (VIR_STRDUP(path, virUSBDeviceGetPath(usb)) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Process path '%s' for USB device", path);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path,
+ VIR_CGROUP_DEVICE_RW, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", rv == 0);
+ if (rv < 0)
goto cleanup;
- }
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
--
2.11.0
6:10 a.m.
New subject: [libvirt] [PATCH 1/7] qemu_cgroup: Kill qemuSetupHostUSBDeviceCgroup
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
There's no need for this function. Currently it is passed as a
callback to virUSBDeviceFileIterate(). However, USB devices have
just one file path. Therefore we can mimic approach used in
qemuDomainGetHostdevPath() to get path and call
virCgroupAllowDevicePath() directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 6c90d46d1..7302c43ee 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -260,23 +260,6 @@ qemuSetupInputCgroup(virDomainObjPtr vm,
}
-static int
-qemuSetupHostUSBDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
- const char *path,
- void *opaque)
-{
- virDomainObjPtr vm = opaque;
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret;
-
- VIR_DEBUG("Process path '%s' for USB device", path);
- ret = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", ret
== 0);
-
- return ret;
-}
-
static int
qemuSetupHostSCSIDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
const char *path,
@@ -333,6 +316,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
virSCSIDevicePtr scsi = NULL;
virSCSIVHostDevicePtr host = NULL;
char *path = NULL;
+ int rv;
/* currently this only does something for PCI devices using vfio
* for device assignment, but it is called for *all* hostdev
@@ -347,8 +331,6 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- int rv;
-
pci = virPCIDeviceNew(pcisrc->addr.domain,
pcisrc->addr.bus,
pcisrc->addr.slot,
@@ -381,13 +363,15 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
goto cleanup;
}
- /* oddly, qemuSetupHostUSBDeviceCgroup doesn't ever
- * reference the usb object we just created
- */
- if (virUSBDeviceFileIterate(usb, qemuSetupHostUSBDeviceCgroup,
- vm) < 0) {
+ if (VIR_STRDUP(path, virUSBDeviceGetPath(usb)) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Process path '%s' for USB device", path);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path,
+ VIR_CGROUP_DEVICE_RW, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", rv == 0);
+ if (rv < 0)
goto cleanup;
- }
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
8:56 a.m.
New subject: [libvirt] [PATCH 2/7] qemu_cgroup: Kill qemuSetupHostSCSIDeviceCgroup
There's no need for this function. Currently it is passed as a
callback to virSCSIDeviceFileIterate(). However, SCSI devices
have just one file path. Therefore we can mimic approach used in
qemuDomainGetHostdevPath() to get path and call
virCgroupAllowDevicePath() directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 38 +++++++++++++-------------------------
1 file changed, 13 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 7302c43ee..6017da662 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -260,28 +260,6 @@ qemuSetupInputCgroup(virDomainObjPtr vm,
}
-static int
-qemuSetupHostSCSIDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
- const char *path,
- void *opaque)
-{
- virDomainObjPtr vm = opaque;
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret;
-
- VIR_DEBUG("Process path '%s' for SCSI device", path);
-
- ret = virCgroupAllowDevicePath(priv->cgroup, path,
- virSCSIDeviceGetReadonly(dev) ?
- VIR_CGROUP_DEVICE_READ :
- VIR_CGROUP_DEVICE_RW, false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
- virSCSIDeviceGetReadonly(dev) ? "r" :
"rw", ret == 0);
-
- return ret;
-}
-
static int
qemuSetupHostSCSIVHostDeviceCgroup(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED,
const char *path,
@@ -395,9 +373,19 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
dev->shareable)) == NULL)
goto cleanup;
- if (virSCSIDeviceFileIterate(scsi,
- qemuSetupHostSCSIDeviceCgroup,
- vm) < 0)
+ if (VIR_STRDUP(path, virSCSIDeviceGetPath(scsi)) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Process path '%s' for SCSI device", path);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path,
+ virSCSIDeviceGetReadonly(scsi) ?
+ VIR_CGROUP_DEVICE_READ :
+ VIR_CGROUP_DEVICE_RW, false);
+
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
+ virSCSIDeviceGetReadonly(scsi) ? "r" :
"rw",
+ rv == 0);
+ if (rv < 0)
goto cleanup;
}
break;
--
2.11.0
6:11 a.m.
New subject: [libvirt] [PATCH 2/7] qemu_cgroup: Kill qemuSetupHostSCSIDeviceCgroup
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
There's no need for this function. Currently it is passed as a
callback to virSCSIDeviceFileIterate(). However, SCSI devices
have just one file path. Therefore we can mimic approach used in
qemuDomainGetHostdevPath() to get path and call
virCgroupAllowDevicePath() directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 38 +++++++++++++-------------------------
1 file changed, 13 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 7302c43ee..6017da662 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -260,28 +260,6 @@ qemuSetupInputCgroup(virDomainObjPtr vm,
}
-static int
-qemuSetupHostSCSIDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
- const char *path,
- void *opaque)
-{
- virDomainObjPtr vm = opaque;
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret;
-
- VIR_DEBUG("Process path '%s' for SCSI device", path);
-
- ret = virCgroupAllowDevicePath(priv->cgroup, path,
- virSCSIDeviceGetReadonly(dev) ?
- VIR_CGROUP_DEVICE_READ :
- VIR_CGROUP_DEVICE_RW, false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
- virSCSIDeviceGetReadonly(dev) ? "r" :
"rw",
ret == 0);
-
- return ret;
-}
-
static int
qemuSetupHostSCSIVHostDeviceCgroup(virSCSIVHostDevicePtr dev
ATTRIBUTE_UNUSED,
const char *path,
@@ -395,9 +373,19 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
dev->shareable)) == NULL)
goto cleanup;
- if (virSCSIDeviceFileIterate(scsi,
-
qemuSetupHostSCSIDeviceCgroup,
- vm) < 0)
+ if (VIR_STRDUP(path, virSCSIDeviceGetPath(scsi)) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Process path '%s' for SCSI device", path);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path,
+
virSCSIDeviceGetReadonly(scsi) ?
+ VIR_CGROUP_DEVICE_READ :
+ VIR_CGROUP_DEVICE_RW,
false);
+
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
+ virSCSIDeviceGetReadonly(scsi) ?
"r" : "rw",
+ rv == 0);
+ if (rv < 0)
goto cleanup;
}
break;
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
8:56 a.m.
New subject: [libvirt] [PATCH 3/7] qemu_cgroup: Kill qemuSetupHostSCSIVHostDeviceCgroup
There's no need for this function. Currently it is passed as a
callback to virSCSIVHostDeviceFileIterate(). However, SCSI host
devices have just one file path. Therefore we can mimic approach
used in qemuDomainGetHostdevPath() to get path and call
virCgroupAllowDevicePath() directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 6017da662..89854b5bd 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -260,25 +260,6 @@ qemuSetupInputCgroup(virDomainObjPtr vm,
}
-static int
-qemuSetupHostSCSIVHostDeviceCgroup(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED,
- const char *path,
- void *opaque)
-{
- virDomainObjPtr vm = opaque;
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret;
-
- VIR_DEBUG("Process path '%s' for scsi_host device", path);
-
- ret = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", ret == 0);
-
- return ret;
-}
-
int
qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
@@ -397,9 +378,17 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn)))
goto cleanup;
- if (virSCSIVHostDeviceFileIterate(host,
- qemuSetupHostSCSIVHostDeviceCgroup,
- vm) < 0)
+ if (VIR_STRDUP(path, virSCSIVHostDeviceGetPath(host)) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Process path '%s' for scsi_host device",
path);
+
+ rv = virCgroupAllowDevicePath(priv->cgroup, path,
+ VIR_CGROUP_DEVICE_RW, false);
+
+ virDomainAuditCgroupPath(vm, priv->cgroup,
+ "allow", path, "rw", rv ==
0);
+ if (rv < 0)
goto cleanup;
}
break;
--
2.11.0
6:12 a.m.
New subject: [libvirt] [PATCH 3/7] qemu_cgroup: Kill qemuSetupHostSCSIVHostDeviceCgroup
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
There's no need for this function. Currently it is passed as a
callback to virSCSIVHostDeviceFileIterate(). However, SCSI host
devices have just one file path. Therefore we can mimic approach
used in qemuDomainGetHostdevPath() to get path and call
virCgroupAllowDevicePath() directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 6017da662..89854b5bd 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -260,25 +260,6 @@ qemuSetupInputCgroup(virDomainObjPtr vm,
}
-static int
-qemuSetupHostSCSIVHostDeviceCgroup(virSCSIVHostDevicePtr dev
ATTRIBUTE_UNUSED,
- const char *path,
- void *opaque)
-{
- virDomainObjPtr vm = opaque;
- qemuDomainObjPrivatePtr priv = vm->privateData;
- int ret;
-
- VIR_DEBUG("Process path '%s' for scsi_host device", path);
-
- ret = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", ret
== 0);
-
- return ret;
-}
-
int
qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
@@ -397,9 +378,17 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn)))
goto cleanup;
- if (virSCSIVHostDeviceFileIterate(host,
-
qemuSetupHostSCSIVHostDeviceCgroup,
- vm) < 0)
+ if (VIR_STRDUP(path, virSCSIVHostDeviceGetPath(host)) < 0)
+ goto cleanup;
+
+ VIR_DEBUG("Process path '%s' for scsi_host device",
path);
+
+ rv = virCgroupAllowDevicePath(priv->cgroup, path,
+ VIR_CGROUP_DEVICE_RW,
false);
+
+ virDomainAuditCgroupPath(vm, priv->cgroup,
+ "allow", path, "rw", rv ==
0);
+ if (rv < 0)
goto cleanup;
}
break;
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
8:56 a.m.
New subject: [libvirt] [PATCH 4/7] qemuSetupHostdevCgroup: Use qemuDomainGetHostdevPath
Since these two functions are nearly identical (with
qemuSetupHostdevCgroup actually calling virCgroupAllowDevicePath)
we can have one function call the other and thus de-duplicate
some code.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 147 +++++--------------------------------------------
src/qemu/qemu_domain.c | 31 +++++++++--
src/qemu/qemu_domain.h | 4 ++
3 files changed, 43 insertions(+), 139 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 89854b5bd..19832c209 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -264,147 +264,26 @@ int
qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
- int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
- virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
- virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
- virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
- virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
- virPCIDevicePtr pci = NULL;
- virUSBDevicePtr usb = NULL;
- virSCSIDevicePtr scsi = NULL;
- virSCSIVHostDevicePtr host = NULL;
char *path = NULL;
- int rv;
+ int perms;
+ int ret = -1;
- /* currently this only does something for PCI devices using vfio
- * for device assignment, but it is called for *all* hostdev
- * devices.
- */
+ if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
+ goto cleanup;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
- return 0;
-
- if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
-
- switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- pci = virPCIDeviceNew(pcisrc->addr.domain,
- pcisrc->addr.bus,
- pcisrc->addr.slot,
- pcisrc->addr.function);
- if (!pci)
- goto cleanup;
-
- if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
- goto cleanup;
-
- VIR_DEBUG("Cgroup allow %s for PCI device assignment", path);
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "allow", path, "rw", rv ==
0);
- if (rv < 0)
- goto cleanup;
- }
- break;
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- /* NB: hostdev->missing wasn't previously checked in the
- * case of hotplug, only when starting a domain. Now it is
- * always checked, and the cgroup setup skipped if true.
- */
- if (dev->missing)
- break;
- if ((usb = virUSBDeviceNew(usbsrc->bus, usbsrc->device,
- NULL)) == NULL) {
- goto cleanup;
- }
-
- if (VIR_STRDUP(path, virUSBDeviceGetPath(usb)) < 0)
- goto cleanup;
-
- VIR_DEBUG("Process path '%s' for USB device", path);
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", rv == 0);
- if (rv < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
- if (scsisrc->protocol ==
- VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
- virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
- /* Follow qemuSetupDiskCgroup() and qemuSetImageCgroupInternal()
- * which does nothing for non local storage
- */
- VIR_DEBUG("Not updating cgroups for hostdev iSCSI path
'%s'",
- iscsisrc->path);
- } else {
- virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
- &scsisrc->u.host;
- if ((scsi = virSCSIDeviceNew(NULL,
- scsihostsrc->adapter,
- scsihostsrc->bus,
- scsihostsrc->target,
- scsihostsrc->unit,
- dev->readonly,
- dev->shareable)) == NULL)
- goto cleanup;
-
- if (VIR_STRDUP(path, virSCSIDeviceGetPath(scsi)) < 0)
- goto cleanup;
-
- VIR_DEBUG("Process path '%s' for SCSI device", path);
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
- virSCSIDeviceGetReadonly(scsi) ?
- VIR_CGROUP_DEVICE_READ :
- VIR_CGROUP_DEVICE_RW, false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
- virSCSIDeviceGetReadonly(scsi) ? "r" :
"rw",
- rv == 0);
- if (rv < 0)
- goto cleanup;
- }
- break;
- }
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: {
- if (hostsrc->protocol ==
- VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) {
- if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn)))
- goto cleanup;
-
- if (VIR_STRDUP(path, virSCSIVHostDeviceGetPath(host)) < 0)
- goto cleanup;
-
- VIR_DEBUG("Process path '%s' for scsi_host device",
path);
-
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "allow", path, "rw", rv ==
0);
- if (rv < 0)
- goto cleanup;
- }
- break;
- }
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
- break;
- }
+ if (!path) {
+ /* There's no path that we need to allow. Claim success. */
+ ret = 0;
+ goto cleanup;
}
- ret = 0;
+ VIR_DEBUG("Cgroup allow %s perms=%d", path, perms);
+ ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
+ virCgroupGetDevicePermsString(perms), ret == 0);
+
cleanup:
- virPCIDeviceFree(pci);
- virUSBDeviceFree(usb);
- virSCSIDeviceFree(scsi);
- virSCSIVHostDeviceFree(host);
VIR_FREE(path);
return ret;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7c696963e..c6d32525f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6831,9 +6831,21 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
}
-static int
+/**
+ * qemuDomainGetHostdevPath:
+ * @dev: host device definition
+ * @path: resulting path to @dev
+ * @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
+ *
+ * For given device @dev fetch its host path and store it at @path. Optionally,
+ * caller can get @perms on the path (e.g. rw/ro).
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
- char **path)
+ char **path,
+ int *perms)
{
int ret = -1;
virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
@@ -6864,6 +6876,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci)))
goto cleanup;
freeTmpPath = true;
+ if (perms)
+ *perms = VIR_CGROUP_DEVICE_RW;
}
break;
@@ -6878,6 +6892,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virUSBDeviceGetPath(usb)))
goto cleanup;
+ if (perms)
+ *perms = VIR_CGROUP_DEVICE_RW;
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
@@ -6902,6 +6918,9 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIDeviceGetPath(scsi)))
goto cleanup;
+ if (perms)
+ *perms = virSCSIDeviceGetReadonly(scsi) ?
+ VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW;
}
break;
@@ -6913,6 +6932,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIVHostDeviceGetPath(host)))
goto cleanup;
+ if (perms)
+ *perms = VIR_CGROUP_DEVICE_RW;
}
break;
}
@@ -7328,7 +7349,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
int ret = -1;
char *path = NULL;
- if (qemuDomainGetHostdevPath(dev, &path) < 0)
+ if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0)
goto cleanup;
if (!path) {
@@ -7964,7 +7985,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
goto cleanup;
if (!path) {
@@ -7995,7 +8016,7 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
goto cleanup;
if (!path) {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 5cfa3e114..f81550e2f 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -802,6 +802,10 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
virQEMUCapsPtr qemuCaps);
+int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+ char **path,
+ int *perms);
+
int qemuDomainBuildNamespace(virQEMUDriverPtr driver,
virDomainObjPtr vm);
--
2.11.0
6:20 a.m.
New subject: [libvirt] [PATCH 4/7] qemuSetupHostdevCgroup: Use qemuDomainGetHostdevPath
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
Since these two functions are nearly identical (with
qemuSetupHostdevCgroup actually calling virCgroupAllowDevicePath)
we can have one function call the other and thus de-duplicate
some code.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 147
+++++--------------------------------------------
src/qemu/qemu_domain.c | 31 +++++++++--
src/qemu/qemu_domain.h | 4 ++
3 files changed, 43 insertions(+), 139 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 89854b5bd..19832c209 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -264,147 +264,26 @@ int
qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
- int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
- virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
- virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
- virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
- virDomainHostdevSubsysSCSIVHostPtr hostsrc =
&dev->source.subsys.u.scsi_host;
- virPCIDevicePtr pci = NULL;
- virUSBDevicePtr usb = NULL;
- virSCSIDevicePtr scsi = NULL;
- virSCSIVHostDevicePtr host = NULL;
char *path = NULL;
- int rv;
+ int perms;
+ int ret = -1;
- /* currently this only does something for PCI devices using vfio
- * for device assignment, but it is called for *all* hostdev
- * devices.
- */
+ if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
+ goto cleanup;
- if (!virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_DEVICES))
- return 0;
-
- if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
-
- switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- pci = virPCIDeviceNew(pcisrc->addr.domain,
- pcisrc->addr.bus,
- pcisrc->addr.slot,
- pcisrc->addr.function);
- if (!pci)
- goto cleanup;
-
- if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
- goto cleanup;
-
- VIR_DEBUG("Cgroup allow %s for PCI device assignment",
path);
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW,
false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "allow", path, "rw", rv ==
0);
- if (rv < 0)
- goto cleanup;
- }
- break;
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- /* NB: hostdev->missing wasn't previously checked in the
- * case of hotplug, only when starting a domain. Now it is
- * always checked, and the cgroup setup skipped if true.
- */
- if (dev->missing)
- break;
- if ((usb = virUSBDeviceNew(usbsrc->bus, usbsrc->device,
- NULL)) == NULL) {
- goto cleanup;
- }
-
- if (VIR_STRDUP(path, virUSBDeviceGetPath(usb)) < 0)
- goto cleanup;
-
- VIR_DEBUG("Process path '%s' for USB device", path);
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW, false);
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
"rw", rv == 0);
- if (rv < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
- if (scsisrc->protocol ==
- VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
- virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc =
&scsisrc->u.iscsi;
- /* Follow qemuSetupDiskCgroup() and
qemuSetImageCgroupInternal()
- * which does nothing for non local storage
- */
- VIR_DEBUG("Not updating cgroups for hostdev iSCSI path
'%s'",
- iscsisrc->path);
- } else {
- virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
- &scsisrc->u.host;
- if ((scsi = virSCSIDeviceNew(NULL,
- scsihostsrc->adapter,
- scsihostsrc->bus,
- scsihostsrc->target,
- scsihostsrc->unit,
- dev->readonly,
- dev->shareable)) == NULL)
- goto cleanup;
-
- if (VIR_STRDUP(path, virSCSIDeviceGetPath(scsi)) < 0)
- goto cleanup;
-
- VIR_DEBUG("Process path '%s' for SCSI device", path);
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
-
virSCSIDeviceGetReadonly(scsi) ?
- VIR_CGROUP_DEVICE_READ :
- VIR_CGROUP_DEVICE_RW,
false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
- virSCSIDeviceGetReadonly(scsi) ?
"r" : "rw",
- rv == 0);
- if (rv < 0)
- goto cleanup;
- }
- break;
- }
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: {
- if (hostsrc->protocol ==
- VIR_DOMAIN_HOSTDEV_SUBSYS_SCSI_HOST_PROTOCOL_TYPE_VHOST) {
- if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn)))
- goto cleanup;
-
- if (VIR_STRDUP(path, virSCSIVHostDeviceGetPath(host)) < 0)
- goto cleanup;
-
- VIR_DEBUG("Process path '%s' for scsi_host device",
path);
-
- rv = virCgroupAllowDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RW,
false);
-
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "allow", path, "rw", rv ==
0);
- if (rv < 0)
- goto cleanup;
- }
- break;
- }
-
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
- break;
- }
+ if (!path) {
+ /* There's no path that we need to allow. Claim success. */
+ ret = 0;
+ goto cleanup;
}
- ret = 0;
+ VIR_DEBUG("Cgroup allow %s perms=%d", path, perms);
+ ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
+ virCgroupGetDevicePermsString(perms), ret ==
0);
+
cleanup:
- virPCIDeviceFree(pci);
- virUSBDeviceFree(usb);
- virSCSIDeviceFree(scsi);
- virSCSIVHostDeviceFree(host);
VIR_FREE(path);
return ret;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7c696963e..c6d32525f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6831,9 +6831,21 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
video,
}
-static int
+/**
+ * qemuDomainGetHostdevPath:
+ * @dev: host device definition
+ * @path: resulting path to @dev
+ * @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
+ *
+ * For given device @dev fetch its host path and store it at @path.
Optionally,
+ * caller can get @perms on the path (e.g. rw/ro).
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
- char **path)
+ char **path,
+ int *perms)
{
int ret = -1;
virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
@@ -6864,6 +6876,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci)))
goto cleanup;
freeTmpPath = true;
+ if (perms)
+ *perms = VIR_CGROUP_DEVICE_RW;
}
break;
@@ -6878,6 +6892,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virUSBDeviceGetPath(usb)))
goto cleanup;
+ if (perms)
+ *perms = VIR_CGROUP_DEVICE_RW;
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
@@ -6902,6 +6918,9 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIDeviceGetPath(scsi)))
goto cleanup;
+ if (perms)
+ *perms = virSCSIDeviceGetReadonly(scsi) ?
+ VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW;
missing a space after :
}
break;
@@ -6913,6 +6932,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIVHostDeviceGetPath(host)))
goto cleanup;
+ if (perms)
+ *perms = VIR_CGROUP_DEVICE_RW;
}
break;
}
@@ -7328,7 +7349,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
int ret = -1;
char *path = NULL;
- if (qemuDomainGetHostdevPath(dev, &path) < 0)
+ if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0)
goto cleanup;
if (!path) {
@@ -7964,7 +7985,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr
driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
goto cleanup;
if (!path) {
@@ -7995,7 +8016,7 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr
driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
goto cleanup;
if (!path) {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 5cfa3e114..f81550e2f 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -802,6 +802,10 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
virQEMUCapsPtr qemuCaps);
+int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+ char **path,
+ int *perms);
+
int qemuDomainBuildNamespace(virQEMUDriverPtr driver,
virDomainObjPtr vm);
--
otherwise,
Reviewed-by: Marc-André Lureau <marcandre.lureau(a)redhat.com>
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
8:56 a.m.
New subject: [libvirt] [PATCH 5/7] qemuDomainGetHostdevPath: Create /dev/vfio/vfio iff needed
So far, we are allowing /dev/vfio/vfio in the devices cgroup
unconditionally (and creating it in the namespace too). Even if
domain has no hostdev assignment configured. This is potential
security hole. Therefore, when starting the domain (or
hotplugging a hostdev) create & allow /dev/vfio/vfio too (if
needed).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_cgroup.c | 53 ++++++++++++----
src/qemu/qemu_domain.c | 124 ++++++++++++++++++++++++-------------
src/qemu/qemu_domain.h | 5 +-
src/qemu/test_libvirtd_qemu.aug.in | 1 -
5 files changed, 125 insertions(+), 60 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 97d769d42..9f990c20d 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -354,7 +354,7 @@
# "/dev/null", "/dev/full", "/dev/zero",
# "/dev/random", "/dev/urandom",
# "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
-# "/dev/rtc","/dev/hpet", "/dev/vfio/vfio"
+# "/dev/rtc","/dev/hpet"
#]
#
# RDMA migration requires the following extra files to be added to the list:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 19832c209..944e8dc87 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -46,12 +46,13 @@ const char *const defaultDeviceACL[] = {
"/dev/null", "/dev/full", "/dev/zero",
"/dev/random", "/dev/urandom",
"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
- "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
+ "/dev/rtc", "/dev/hpet",
NULL,
};
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
+#define DEV_VFIO "/dev/vfio/vfio"
static int
qemuSetupImagePathCgroup(virDomainObjPtr vm,
@@ -265,26 +266,31 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- char *path = NULL;
- int perms;
- int ret = -1;
+ char **path = NULL;
+ int *perms = NULL;
+ size_t i, npaths = 0;
+ int rv, ret = -1;
- if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
+ if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
goto cleanup;
- if (!path) {
- /* There's no path that we need to allow. Claim success. */
- ret = 0;
- goto cleanup;
+ for (i = 0; i < npaths; i++) {
+ VIR_DEBUG("Cgroup allow %s perms=%d", path[i], perms[i]);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path[i], perms[i], false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path[i],
+ virCgroupGetDevicePermsString(perms[i]),
+ ret == 0);
+ if (rv < 0)
+ goto cleanup;
}
- VIR_DEBUG("Cgroup allow %s perms=%d", path, perms);
- ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, false);
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
- virCgroupGetDevicePermsString(perms), ret == 0);
+ ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
+ VIR_FREE(perms);
return ret;
}
@@ -312,6 +318,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
int rv;
+ size_t i, vfios = 0;
pci = virPCIDeviceNew(pcisrc->addr.domain,
pcisrc->addr.bus,
@@ -330,6 +337,26 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
"deny", path, "rwm", rv ==
0);
if (rv < 0)
goto cleanup;
+
+ /* If this is the last hostdev with VFIO backend deny
+ * /dev/vfio/vfio too. */
+ for (i = 0; i < vm->def->nhostdevs; i++) {
+ virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
+ if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ tmp->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI
&&
+ tmp->source.subsys.u.pci.backend ==
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
+ vfios++;
+ }
+
+ if (vfios == 0) {
+ VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device
assignment");
+ rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
+ VIR_CGROUP_DEVICE_RWM, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup,
+ "deny", DEV_VFIO, "rwm",
rv == 0);
+ if (rv < 0)
+ goto cleanup;
+ }
}
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c6d32525f..530eced33 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -107,6 +107,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
#define PROC_MOUNTS "/proc/mounts"
#define DEVPREFIX "/dev/"
+#define DEV_VFIO "/dev/vfio/vfio"
struct _qemuDomainLogContext {
@@ -6834,18 +6835,24 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
/**
* qemuDomainGetHostdevPath:
* @dev: host device definition
+ * @npaths: number of items in @path and @perms arrays
* @path: resulting path to @dev
* @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
*
- * For given device @dev fetch its host path and store it at @path. Optionally,
- * caller can get @perms on the path (e.g. rw/ro).
+ * For given device @dev fetch its host path and store it at
+ * @path. If a device requires other paths to be present/allowed
+ * they are stored in the @path array after the actual path.
+ * Optionally, caller can get @perms on the path (e.g. rw/ro).
+ *
+ * The caller is responsible for freeing the memory.
*
* Returns 0 on success, -1 otherwise.
*/
int
qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
- char **path,
- int *perms)
+ size_t *npaths,
+ char ***path,
+ int **perms)
{
int ret = -1;
virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
@@ -6858,8 +6865,13 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
virSCSIVHostDevicePtr host = NULL;
char *tmpPath = NULL;
bool freeTmpPath = false;
+ bool includeVFIO = false;
+ char **tmpPaths = NULL;
+ int *tmpPerms = NULL;
+ size_t i, tmpNpaths = 0;
+ int perm = 0;
- *path = NULL;
+ *npaths = 0;
switch ((virDomainHostdevMode) dev->mode) {
case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
@@ -6876,8 +6888,9 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci)))
goto cleanup;
freeTmpPath = true;
- if (perms)
- *perms = VIR_CGROUP_DEVICE_RW;
+
+ perm = VIR_CGROUP_DEVICE_RW;
+ includeVFIO = true;
}
break;
@@ -6892,8 +6905,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virUSBDeviceGetPath(usb)))
goto cleanup;
- if (perms)
- *perms = VIR_CGROUP_DEVICE_RW;
+ perm = VIR_CGROUP_DEVICE_RW;
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
@@ -6918,9 +6930,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIDeviceGetPath(scsi)))
goto cleanup;
- if (perms)
- *perms = virSCSIDeviceGetReadonly(scsi) ?
- VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW;
+ perm = virSCSIDeviceGetReadonly(scsi) ?
+ VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW;
}
break;
@@ -6932,8 +6943,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIVHostDeviceGetPath(host)))
goto cleanup;
- if (perms)
- *perms = VIR_CGROUP_DEVICE_RW;
+ perm = VIR_CGROUP_DEVICE_RW;
}
break;
}
@@ -6949,11 +6959,40 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
break;
}
- if (VIR_STRDUP(*path, tmpPath) < 0)
- goto cleanup;
+ if (tmpPath) {
+ size_t toAlloc = 1;
+ if (includeVFIO)
+ toAlloc = 2;
+
+ if (VIR_ALLOC_N(tmpPaths, toAlloc) < 0 ||
+ VIR_ALLOC_N(tmpPerms, toAlloc) < 0 ||
+ VIR_STRDUP(tmpPaths[0], tmpPath) < 0)
+ goto cleanup;
+ tmpNpaths = toAlloc;
+ tmpPerms[0] = perm;
+
+ if (includeVFIO) {
+ if (VIR_STRDUP(tmpPaths[1], DEV_VFIO) < 0)
+ goto cleanup;
+ tmpPerms[1] = VIR_CGROUP_DEVICE_RW;
+ }
+ }
+
+ *npaths = tmpNpaths;
+ tmpNpaths = 0;
+ *path = tmpPaths;
+ tmpPaths = NULL;
+ if (perms) {
+ *perms = tmpPerms;
+ tmpPerms = NULL;
+ }
ret = 0;
cleanup:
+ for (i = 0; i < tmpNpaths; i++)
+ VIR_FREE(tmpPaths[i]);
+ VIR_FREE(tmpPaths);
+ VIR_FREE(tmpPerms);
virPCIDeviceFree(pci);
virUSBDeviceFree(usb);
virSCSIDeviceFree(scsi);
@@ -7347,22 +7386,21 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
const char *devPath)
{
int ret = -1;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
- if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
goto cleanup;
- if (!path) {
- /* There's no /dev device that we need to create. Claim success. */
- ret = 0;
- goto cleanup;
+ for (i = 0; i < npaths; i++) {
+ if (qemuDomainCreateDevice(path[i], devPath, false) < 0)
+ goto cleanup;
}
- if (qemuDomainCreateDevice(path, devPath, false) < 0)
- goto cleanup;
-
ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
@@ -7980,26 +8018,26 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev)
{
int ret = -1;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
goto cleanup;
- if (!path) {
- /* There's no /dev device that we need to create. Claim success. */
- ret = 0;
+ for (i = 0; i < npaths; i++) {
+ if (qemuDomainAttachDeviceMknod(driver,
+ vm,
+ path[i]) < 0)
goto cleanup;
}
- if (qemuDomainAttachDeviceMknod(driver,
- vm,
- path) < 0)
- goto cleanup;
ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
@@ -8011,25 +8049,25 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev)
{
int ret = -1;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
goto cleanup;
- if (!path) {
- /* There's no /dev device that we need to create. Claim success. */
- ret = 0;
- goto cleanup;
- }
-
- if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0)
+ /* Don't remove other paths than for the @hostdev itself.
+ * They might be still in use by other devices. */
+ if (npaths > 0 &&
+ qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
goto cleanup;
ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f81550e2f..e64aa25ba 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -803,8 +803,9 @@ bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
virQEMUCapsPtr qemuCaps);
int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
- char **path,
- int *perms);
+ size_t *npaths,
+ char ***path,
+ int **perms);
int qemuDomainBuildNamespace(virQEMUDriverPtr driver,
virDomainObjPtr vm);
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index bd25235d3..6f03898c0 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -55,7 +55,6 @@ module Test_libvirtd_qemu =
{ "8" = "/dev/kqemu" }
{ "9" = "/dev/rtc" }
{ "10" = "/dev/hpet" }
- { "11" = "/dev/vfio/vfio" }
}
{ "save_image_format" = "raw" }
{ "dump_image_format" = "raw" }
--
2.11.0
6:23 a.m.
New subject: [libvirt] [PATCH 5/7] qemuDomainGetHostdevPath: Create /dev/vfio/vfio iff needed
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
So far, we are allowing /dev/vfio/vfio in the devices cgroup
unconditionally (and creating it in the namespace too). Even if
domain has no hostdev assignment configured. This is potential
security hole. Therefore, when starting the domain (or
hotplugging a hostdev) create & allow /dev/vfio/vfio too (if
needed).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
looks good to me,
Reviewed-by: Marc-André Lureau <marcandre.lureau(a)redhat.com>
---
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_cgroup.c | 53 ++++++++++++----
src/qemu/qemu_domain.c | 124
++++++++++++++++++++++++-------------
src/qemu/qemu_domain.h | 5 +-
src/qemu/test_libvirtd_qemu.aug.in | 1 -
5 files changed, 125 insertions(+), 60 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 97d769d42..9f990c20d 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -354,7 +354,7 @@
# "/dev/null", "/dev/full", "/dev/zero",
# "/dev/random", "/dev/urandom",
# "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
-# "/dev/rtc","/dev/hpet", "/dev/vfio/vfio"
+# "/dev/rtc","/dev/hpet"
#]
#
# RDMA migration requires the following extra files to be added to the
list:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 19832c209..944e8dc87 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -46,12 +46,13 @@ const char *const defaultDeviceACL[] = {
"/dev/null", "/dev/full", "/dev/zero",
"/dev/random", "/dev/urandom",
"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
- "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
+ "/dev/rtc", "/dev/hpet",
NULL,
};
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
+#define DEV_VFIO "/dev/vfio/vfio"
static int
qemuSetupImagePathCgroup(virDomainObjPtr vm,
@@ -265,26 +266,31 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- char *path = NULL;
- int perms;
- int ret = -1;
+ char **path = NULL;
+ int *perms = NULL;
+ size_t i, npaths = 0;
+ int rv, ret = -1;
- if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
+ if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
goto cleanup;
- if (!path) {
- /* There's no path that we need to allow. Claim success. */
- ret = 0;
- goto cleanup;
+ for (i = 0; i < npaths; i++) {
+ VIR_DEBUG("Cgroup allow %s perms=%d", path[i], perms[i]);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path[i], perms[i],
false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path[i],
+ virCgroupGetDevicePermsString(perms[i]),
+ ret == 0);
+ if (rv < 0)
+ goto cleanup;
}
- VIR_DEBUG("Cgroup allow %s perms=%d", path, perms);
- ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, false);
- virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
- virCgroupGetDevicePermsString(perms), ret ==
0);
+ ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
+ VIR_FREE(perms);
return ret;
}
@@ -312,6 +318,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
int rv;
+ size_t i, vfios = 0;
pci = virPCIDeviceNew(pcisrc->addr.domain,
pcisrc->addr.bus,
@@ -330,6 +337,26 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
"deny", path, "rwm", rv ==
0);
if (rv < 0)
goto cleanup;
+
+ /* If this is the last hostdev with VFIO backend deny
+ * /dev/vfio/vfio too. */
+ for (i = 0; i < vm->def->nhostdevs; i++) {
+ virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
+ if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ tmp->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ tmp->source.subsys.u.pci.backend ==
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
+ vfios++;
+ }
+
+ if (vfios == 0) {
+ VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device
assignment");
+ rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
+ VIR_CGROUP_DEVICE_RWM,
false);
+ virDomainAuditCgroupPath(vm, priv->cgroup,
+ "deny", DEV_VFIO,
"rwm", rv
== 0);
+ if (rv < 0)
+ goto cleanup;
+ }
}
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c6d32525f..530eced33 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -107,6 +107,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
#define PROC_MOUNTS "/proc/mounts"
#define DEVPREFIX "/dev/"
+#define DEV_VFIO "/dev/vfio/vfio"
struct _qemuDomainLogContext {
@@ -6834,18 +6835,24 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
video,
/**
* qemuDomainGetHostdevPath:
* @dev: host device definition
+ * @npaths: number of items in @path and @perms arrays
* @path: resulting path to @dev
* @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
*
- * For given device @dev fetch its host path and store it at @path.
Optionally,
- * caller can get @perms on the path (e.g. rw/ro).
+ * For given device @dev fetch its host path and store it at
+ * @path. If a device requires other paths to be present/allowed
+ * they are stored in the @path array after the actual path.
+ * Optionally, caller can get @perms on the path (e.g. rw/ro).
+ *
+ * The caller is responsible for freeing the memory.
*
* Returns 0 on success, -1 otherwise.
*/
int
qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
- char **path,
- int *perms)
+ size_t *npaths,
+ char ***path,
+ int **perms)
{
int ret = -1;
virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
@@ -6858,8 +6865,13 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
virSCSIVHostDevicePtr host = NULL;
char *tmpPath = NULL;
bool freeTmpPath = false;
+ bool includeVFIO = false;
+ char **tmpPaths = NULL;
+ int *tmpPerms = NULL;
+ size_t i, tmpNpaths = 0;
+ int perm = 0;
- *path = NULL;
+ *npaths = 0;
switch ((virDomainHostdevMode) dev->mode) {
case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
@@ -6876,8 +6888,9 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci)))
goto cleanup;
freeTmpPath = true;
- if (perms)
- *perms = VIR_CGROUP_DEVICE_RW;
+
+ perm = VIR_CGROUP_DEVICE_RW;
+ includeVFIO = true;
}
break;
@@ -6892,8 +6905,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virUSBDeviceGetPath(usb)))
goto cleanup;
- if (perms)
- *perms = VIR_CGROUP_DEVICE_RW;
+ perm = VIR_CGROUP_DEVICE_RW;
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
@@ -6918,9 +6930,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIDeviceGetPath(scsi)))
goto cleanup;
- if (perms)
- *perms = virSCSIDeviceGetReadonly(scsi) ?
- VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW;
+ perm = virSCSIDeviceGetReadonly(scsi) ?
+ VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW;
}
break;
@@ -6932,8 +6943,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
if (!(tmpPath = (char *) virSCSIVHostDeviceGetPath(host)))
goto cleanup;
- if (perms)
- *perms = VIR_CGROUP_DEVICE_RW;
+ perm = VIR_CGROUP_DEVICE_RW;
}
break;
}
@@ -6949,11 +6959,40 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr
dev,
break;
}
- if (VIR_STRDUP(*path, tmpPath) < 0)
- goto cleanup;
+ if (tmpPath) {
+ size_t toAlloc = 1;
+ if (includeVFIO)
+ toAlloc = 2;
+
+ if (VIR_ALLOC_N(tmpPaths, toAlloc) < 0 ||
+ VIR_ALLOC_N(tmpPerms, toAlloc) < 0 ||
+ VIR_STRDUP(tmpPaths[0], tmpPath) < 0)
+ goto cleanup;
+ tmpNpaths = toAlloc;
+ tmpPerms[0] = perm;
+
+ if (includeVFIO) {
+ if (VIR_STRDUP(tmpPaths[1], DEV_VFIO) < 0)
+ goto cleanup;
+ tmpPerms[1] = VIR_CGROUP_DEVICE_RW;
+ }
+ }
+
+ *npaths = tmpNpaths;
+ tmpNpaths = 0;
+ *path = tmpPaths;
+ tmpPaths = NULL;
+ if (perms) {
+ *perms = tmpPerms;
+ tmpPerms = NULL;
+ }
ret = 0;
cleanup:
+ for (i = 0; i < tmpNpaths; i++)
+ VIR_FREE(tmpPaths[i]);
+ VIR_FREE(tmpPaths);
+ VIR_FREE(tmpPerms);
virPCIDeviceFree(pci);
virUSBDeviceFree(usb);
virSCSIDeviceFree(scsi);
@@ -7347,22 +7386,21 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
const char *devPath)
{
int ret = -1;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
- if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
goto cleanup;
- if (!path) {
- /* There's no /dev device that we need to create. Claim success.
*/
- ret = 0;
- goto cleanup;
+ for (i = 0; i < npaths; i++) {
+ if (qemuDomainCreateDevice(path[i], devPath, false) < 0)
+ goto cleanup;
}
- if (qemuDomainCreateDevice(path, devPath, false) < 0)
- goto cleanup;
-
ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
@@ -7980,26 +8018,26 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr
driver,
virDomainHostdevDefPtr hostdev)
{
int ret = -1;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
goto cleanup;
- if (!path) {
- /* There's no /dev device that we need to create. Claim success.
*/
- ret = 0;
+ for (i = 0; i < npaths; i++) {
+ if (qemuDomainAttachDeviceMknod(driver,
+ vm,
+ path[i]) < 0)
goto cleanup;
}
- if (qemuDomainAttachDeviceMknod(driver,
- vm,
- path) < 0)
- goto cleanup;
ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
@@ -8011,25 +8049,25 @@
qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev)
{
int ret = -1;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
goto cleanup;
- if (!path) {
- /* There's no /dev device that we need to create. Claim success.
*/
- ret = 0;
- goto cleanup;
- }
-
- if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0)
+ /* Don't remove other paths than for the @hostdev itself.
+ * They might be still in use by other devices. */
+ if (npaths > 0 &&
+ qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
goto cleanup;
ret = 0;
cleanup:
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f81550e2f..e64aa25ba 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -803,8 +803,9 @@ bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr
video,
virQEMUCapsPtr qemuCaps);
int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
- char **path,
- int *perms);
+ size_t *npaths,
+ char ***path,
+ int **perms);
int qemuDomainBuildNamespace(virQEMUDriverPtr driver,
virDomainObjPtr vm);
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/
test_libvirtd_qemu.aug.in
index bd25235d3..6f03898c0 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -55,7 +55,6 @@ module Test_libvirtd_qemu =
{ "8" = "/dev/kqemu" }
{ "9" = "/dev/rtc" }
{ "10" = "/dev/hpet" }
- { "11" = "/dev/vfio/vfio" }
}
{ "save_image_format" = "raw" }
{ "dump_image_format" = "raw" }
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
8:56 a.m.
New subject: [libvirt] [PATCH 6/7] qemuDomainGetHostdevPath: Report /dev/vfio/vfio less frequently
So far, qemuDomainGetHostdevPath has no knowledge of the reasong
it is called and thus reports /dev/vfio/vfio for every VFIO
backed device. This is suboptimal, as we want it to:
a) report /dev/vfio/vfio on every addition or domain startup
b) report /dev/vfio/vfio only on last VFIO device being unplugged
If a domain is being stopped then namespace and CGroup die with
it so no need to worry about that. I mean, even when a domain
that's exiting has more than one VFIO devices assigned to it,
this function does not clean /dev/vfio/vfio in CGroup nor in the
namespace. But that doesn't matter.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 87 ++++++++++++--------------------------------------
src/qemu/qemu_domain.c | 38 ++++++++++++++++------
src/qemu/qemu_domain.h | 4 ++-
3 files changed, 52 insertions(+), 77 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 944e8dc87..209cbc275 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -52,7 +52,6 @@ const char *const defaultDeviceACL[] = {
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
-#define DEV_VFIO "/dev/vfio/vfio"
static int
qemuSetupImagePathCgroup(virDomainObjPtr vm,
@@ -271,7 +270,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
size_t i, npaths = 0;
int rv, ret = -1;
- if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
+ if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, &perms)
< 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -298,11 +297,10 @@ int
qemuTeardownHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
- int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
- virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
- virPCIDevicePtr pci = NULL;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
+ int rv, ret = -1;
/* currently this only does something for PCI devices using vfio
* for device assignment, but it is called for *all* hostdev
@@ -312,70 +310,27 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
return 0;
- if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
-
- switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- int rv;
- size_t i, vfios = 0;
-
- pci = virPCIDeviceNew(pcisrc->addr.domain,
- pcisrc->addr.bus,
- pcisrc->addr.slot,
- pcisrc->addr.function);
- if (!pci)
- goto cleanup;
-
- if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
- goto cleanup;
-
- VIR_DEBUG("Cgroup deny %s for PCI device assignment", path);
- rv = virCgroupDenyDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RWM, false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "deny", path, "rwm", rv ==
0);
- if (rv < 0)
- goto cleanup;
-
- /* If this is the last hostdev with VFIO backend deny
- * /dev/vfio/vfio too. */
- for (i = 0; i < vm->def->nhostdevs; i++) {
- virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
- if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
- tmp->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI
&&
- tmp->source.subsys.u.pci.backend ==
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
- vfios++;
- }
-
- if (vfios == 0) {
- VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device
assignment");
- rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
- VIR_CGROUP_DEVICE_RWM, false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "deny", DEV_VFIO, "rwm",
rv == 0);
- if (rv < 0)
- goto cleanup;
- }
- }
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- /* nothing to tear down for USB */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
- /* nothing to tear down for SCSI */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
- /* nothing to tear down for scsi_host */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
- break;
- }
+ if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ dev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ dev->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO
&&
+ qemuDomainGetHostdevPath(vm->def, dev, true,
+ &npaths, &path, NULL) < 0)
+ goto cleanup;
+
+ for (i = 0; i < npaths; i++) {
+ VIR_DEBUG("Cgroup deny %s", path[i]);
+ rv = virCgroupDenyDevicePath(priv->cgroup, path[i],
+ VIR_CGROUP_DEVICE_RWM, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup,
+ "deny", path[i], "rwm", rv == 0);
+ if (rv < 0)
+ goto cleanup;
}
ret = 0;
cleanup:
- virPCIDeviceFree(pci);
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 530eced33..515e0052e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6834,7 +6834,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
/**
* qemuDomainGetHostdevPath:
+ * @def: domain definition
* @dev: host device definition
+ * @teardown: true if device will be removed
* @npaths: number of items in @path and @perms arrays
* @path: resulting path to @dev
* @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
@@ -6849,7 +6851,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
* Returns 0 on success, -1 otherwise.
*/
int
-qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+qemuDomainGetHostdevPath(virDomainDefPtr def,
+ virDomainHostdevDefPtr dev,
+ bool teardown,
size_t *npaths,
char ***path,
int **perms)
@@ -6890,7 +6894,21 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
freeTmpPath = true;
perm = VIR_CGROUP_DEVICE_RW;
- includeVFIO = true;
+ if (teardown) {
+ size_t nvfios = 0;
+ for (i = 0; i < def->nhostdevs; i++) {
+ virDomainHostdevDefPtr tmp = def->hostdevs[i];
+ if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ tmp->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ tmp->source.subsys.u.pci.backend ==
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
+ nvfios++;
+ }
+
+ if (nvfios == 0)
+ includeVFIO = true;
+ } else {
+ includeVFIO = true;
+ }
}
break;
@@ -7389,7 +7407,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
char **path = NULL;
size_t i, npaths = 0;
- if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, NULL) < 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -8024,7 +8042,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(NULL, hostdev, false, &npaths, &path, NULL) <
0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -8055,14 +8073,14 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(vm->def, hostdev, true,
+ &npaths, &path, NULL) < 0)
goto cleanup;
- /* Don't remove other paths than for the @hostdev itself.
- * They might be still in use by other devices. */
- if (npaths > 0 &&
- qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
- goto cleanup;
+ for (i = 0; i < npaths; i++) {
+ if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0)
+ goto cleanup;
+ }
ret = 0;
cleanup:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index e64aa25ba..80de50fbe 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -802,7 +802,9 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
virQEMUCapsPtr qemuCaps);
-int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+int qemuDomainGetHostdevPath(virDomainDefPtr def,
+ virDomainHostdevDefPtr dev,
+ bool teardown,
size_t *npaths,
char ***path,
int **perms);
--
2.11.0
6:32 a.m.
New subject: [libvirt] [PATCH 6/7] qemuDomainGetHostdevPath: Report /dev/vfio/vfio less frequently
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
So far, qemuDomainGetHostdevPath has no knowledge of the reasong
reasong/reason
it is called and thus reports /dev/vfio/vfio for every VFIO
backed device. This is suboptimal, as we want it to:
a) report /dev/vfio/vfio on every addition or domain startup
b) report /dev/vfio/vfio only on last VFIO device being unplugged
If a domain is being stopped then namespace and CGroup die with
it so no need to worry about that. I mean, even when a domain
that's exiting has more than one VFIO devices assigned to it,
this function does not clean /dev/vfio/vfio in CGroup nor in the
namespace. But that doesn't matter.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Fine approach to me,
Reviewed-by: Marc-André Lureau <marcandre.lureau(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 87
++++++++++++--------------------------------------
src/qemu/qemu_domain.c | 38 ++++++++++++++++------
src/qemu/qemu_domain.h | 4 ++-
3 files changed, 52 insertions(+), 77 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 944e8dc87..209cbc275 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -52,7 +52,6 @@ const char *const defaultDeviceACL[] = {
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
-#define DEV_VFIO "/dev/vfio/vfio"
static int
qemuSetupImagePathCgroup(virDomainObjPtr vm,
@@ -271,7 +270,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
size_t i, npaths = 0;
int rv, ret = -1;
- if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
+ if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path,
&perms) < 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -298,11 +297,10 @@ int
qemuTeardownHostdevCgroup(virDomainObjPtr vm,
virDomainHostdevDefPtr dev)
{
- int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
- virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
- virPCIDevicePtr pci = NULL;
- char *path = NULL;
+ char **path = NULL;
+ size_t i, npaths = 0;
+ int rv, ret = -1;
/* currently this only does something for PCI devices using vfio
* for device assignment, but it is called for *all* hostdev
@@ -312,70 +310,27 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
if (!virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_DEVICES))
return 0;
- if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
-
- switch ((virDomainHostdevSubsysType) dev->source.subsys.type) {
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
- if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
- int rv;
- size_t i, vfios = 0;
-
- pci = virPCIDeviceNew(pcisrc->addr.domain,
- pcisrc->addr.bus,
- pcisrc->addr.slot,
- pcisrc->addr.function);
- if (!pci)
- goto cleanup;
-
- if (!(path = virPCIDeviceGetIOMMUGroupDev(pci)))
- goto cleanup;
-
- VIR_DEBUG("Cgroup deny %s for PCI device assignment",
path);
- rv = virCgroupDenyDevicePath(priv->cgroup, path,
- VIR_CGROUP_DEVICE_RWM,
false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "deny", path, "rwm", rv ==
0);
- if (rv < 0)
- goto cleanup;
-
- /* If this is the last hostdev with VFIO backend deny
- * /dev/vfio/vfio too. */
- for (i = 0; i < vm->def->nhostdevs; i++) {
- virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
- if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
- tmp->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
- tmp->source.subsys.u.pci.backend ==
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
- vfios++;
- }
-
- if (vfios == 0) {
- VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device
assignment");
- rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
- VIR_CGROUP_DEVICE_RWM,
false);
- virDomainAuditCgroupPath(vm, priv->cgroup,
- "deny", DEV_VFIO,
"rwm", rv
== 0);
- if (rv < 0)
- goto cleanup;
- }
- }
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
- /* nothing to tear down for USB */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
- /* nothing to tear down for SCSI */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
- /* nothing to tear down for scsi_host */
- break;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
- break;
- }
+ if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ dev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ dev->source.subsys.u.pci.backend ==
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO &&
+ qemuDomainGetHostdevPath(vm->def, dev, true,
+ &npaths, &path, NULL) < 0)
+ goto cleanup;
+
+ for (i = 0; i < npaths; i++) {
+ VIR_DEBUG("Cgroup deny %s", path[i]);
+ rv = virCgroupDenyDevicePath(priv->cgroup, path[i],
+ VIR_CGROUP_DEVICE_RWM, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup,
+ "deny", path[i], "rwm", rv == 0);
+ if (rv < 0)
+ goto cleanup;
}
ret = 0;
cleanup:
- virPCIDeviceFree(pci);
+ for (i = 0; i < npaths; i++)
+ VIR_FREE(path[i]);
VIR_FREE(path);
return ret;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 530eced33..515e0052e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6834,7 +6834,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
video,
/**
* qemuDomainGetHostdevPath:
+ * @def: domain definition
* @dev: host device definition
+ * @teardown: true if device will be removed
* @npaths: number of items in @path and @perms arrays
* @path: resulting path to @dev
* @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
@@ -6849,7 +6851,9 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr
video,
* Returns 0 on success, -1 otherwise.
*/
int
-qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+qemuDomainGetHostdevPath(virDomainDefPtr def,
+ virDomainHostdevDefPtr dev,
+ bool teardown,
size_t *npaths,
char ***path,
int **perms)
@@ -6890,7 +6894,21 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
freeTmpPath = true;
perm = VIR_CGROUP_DEVICE_RW;
- includeVFIO = true;
+ if (teardown) {
+ size_t nvfios = 0;
+ for (i = 0; i < def->nhostdevs; i++) {
+ virDomainHostdevDefPtr tmp = def->hostdevs[i];
+ if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+ tmp->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+ tmp->source.subsys.u.pci.backend ==
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
+ nvfios++;
+ }
+
+ if (nvfios == 0)
+ includeVFIO = true;
+ } else {
+ includeVFIO = true;
+ }
}
break;
@@ -7389,7 +7407,7 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
char **path = NULL;
size_t i, npaths = 0;
- if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(NULL, dev, false, &npaths, &path, NULL)
< 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -8024,7 +8042,7 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr
driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(NULL, hostdev, false, &npaths, &path,
NULL) < 0)
goto cleanup;
for (i = 0; i < npaths; i++) {
@@ -8055,14 +8073,14 @@
qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
return 0;
- if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
+ if (qemuDomainGetHostdevPath(vm->def, hostdev, true,
+ &npaths, &path, NULL) < 0)
goto cleanup;
- /* Don't remove other paths than for the @hostdev itself.
- * They might be still in use by other devices. */
- if (npaths > 0 &&
- qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
- goto cleanup;
+ for (i = 0; i < npaths; i++) {
+ if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0)
+ goto cleanup;
+ }
ret = 0;
cleanup:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index e64aa25ba..80de50fbe 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -802,7 +802,9 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver,
bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
virQEMUCapsPtr qemuCaps);
-int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
+int qemuDomainGetHostdevPath(virDomainDefPtr def,
+ virDomainHostdevDefPtr dev,
+ bool teardown,
size_t *npaths,
char ***path,
int **perms);
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
8:56 a.m.
New subject: [libvirt] [PATCH 7/7] qemu: Allow /dev/dri/render* for virgl domains
When enabling virgl, qemu opens /dev/dri/render*. So far, we are
not allowing that in devices cgroup nor creating the file in
domain's namespace and thus requiring users to set the paths in
qemu.conf. This, however, is suboptimal as it allows access to
ALL qemu processes even those which don't have virgl configured.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_cgroup.c | 51 ++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 115 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 209cbc275..c667dc12b 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -335,6 +335,52 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
return ret;
}
+
+static int
+qemuSetupVideoCgroup(virDomainObjPtr vm,
+ virDomainVideoDefPtr video)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ const char *dripath = "/dev/dri";
+ char *devpath = NULL;
+ struct dirent *ent;
+ DIR *dir;
+ int rv, rc, ret = -1;
+
+ if (!video->accel ||
+ !video->accel->accel3d)
+ return 0;
+
+ if (virDirOpen(&dir, dripath) < 0)
+ return ret;
+
+ while ((rv = virDirRead(dir, &ent, dripath)) > 0) {
+ if (!STRPREFIX(ent->d_name, "render"))
+ continue;
+
+ VIR_FREE(devpath);
+ if (virAsprintf(&devpath, "%s/%s", dripath, ent->d_name) <
0)
+ goto cleanup;
+
+ rc = virCgroupAllowDevicePath(priv->cgroup, devpath,
+ VIR_CGROUP_DEVICE_RW, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devpath,
+ "rw", rc == 0);
+ if (rv < 0)
+ goto cleanup;
+ }
+
+ if (rv < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(devpath);
+ VIR_DIR_CLOSE(dir);
+ return ret;
+}
+
+
static int
qemuSetupBlkioCgroup(virDomainObjPtr vm)
{
@@ -604,6 +650,11 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
goto cleanup;
}
+ for (i = 0; i < vm->def->nvideos; i++) {
+ if (qemuSetupVideoCgroup(vm, vm->def->videos[i]) < 0)
+ goto cleanup;
+ }
+
for (i = 0; i < vm->def->ninputs; i++) {
if (qemuSetupInputCgroup(vm, vm->def->inputs[i]) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 515e0052e..06ba1cf00 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7504,6 +7504,67 @@ qemuDomainSetupTPM(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
}
+static int
+qemuDomainSetupVideo(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+ virDomainVideoDefPtr video,
+ const char *devPath)
+{
+ const char *dripath = "/dev/dri";
+ char *dridevpath = NULL;
+ struct dirent *ent;
+ DIR *dir;
+ int rv, ret = -1;
+
+ if (!video->accel ||
+ !video->accel->accel3d)
+ return 0;
+
+ if (virDirOpen(&dir, dripath) < 0)
+ return ret;
+
+ while ((rv = virDirRead(dir, &ent, dripath)) > 0) {
+ if (!STRPREFIX(ent->d_name, "render"))
+ continue;
+
+ VIR_FREE(dridevpath);
+ if (virAsprintf(&dridevpath, "%s/%s", dripath, ent->d_name) <
0)
+ goto cleanup;
+
+ if (qemuDomainCreateDevice(dridevpath, devPath, false) < 0)
+ goto cleanup;
+ }
+
+ if (rv < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(dridevpath);
+ VIR_DIR_CLOSE(dir);
+ return ret;
+}
+
+
+static int
+qemuDomainSetupAllVideos(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ const char *devPath)
+{
+ size_t i;
+
+ VIR_DEBUG("Setting up videos");
+ for (i = 0; i < vm->def->nvideos; i++) {
+ if (qemuDomainSetupVideo(driver,
+ vm->def->videos[i],
+ devPath) < 0)
+ return -1;
+ }
+
+ VIR_DEBUG("Setup all videos");
+ return 0;
+}
+
+
static int
qemuDomainSetupInput(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
virDomainInputDefPtr input,
@@ -7657,6 +7718,9 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver,
if (qemuDomainSetupTPM(driver, vm, devPath) < 0)
goto cleanup;
+ if (qemuDomainSetupAllVideos(driver, vm, devPath) < 0)
+ goto cleanup;
+
if (qemuDomainSetupAllInputs(driver, vm, devPath) < 0)
goto cleanup;
--
2.11.0
6:47 a.m.
New subject: [libvirt] [PATCH 7/7] qemu: Allow /dev/dri/render* for virgl domains
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
When enabling virgl, qemu opens /dev/dri/render*. So far, we are
not allowing that in devices cgroup nor creating the file in
domain's namespace and thus requiring users to set the paths in
qemu.conf. This, however, is suboptimal as it allows access to
ALL qemu processes even those which don't have virgl configured.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Thanks, but that doesn't work :)
You should loop over the spice/gl graphics nodes (virtio accel3d is not
actually using 3d, as of today, if the graphics configuration/layer doesn't
provide it)
See also Ján Tomko "qemu_cgroup: allow access to /dev/dri/render*" patch,
which use to work.
After my series "[PATCH 0/5] Add rendernode selection support", it will
further have to narrow the path allowed to the specified rendernode. This
can be done in my series or yours, depending on applied order.
thanks
---
src/qemu/qemu_cgroup.c | 51 ++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.c | 64
++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 115 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 209cbc275..c667dc12b 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -335,6 +335,52 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
return ret;
}
+
+static int
+qemuSetupVideoCgroup(virDomainObjPtr vm,
+ virDomainVideoDefPtr video)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ const char *dripath = "/dev/dri";
+ char *devpath = NULL;
+ struct dirent *ent;
+ DIR *dir;
+ int rv, rc, ret = -1;
+
+ if (!video->accel ||
+ !video->accel->accel3d)
+ return 0;
+
+ if (virDirOpen(&dir, dripath) < 0)
+ return ret;
+
+ while ((rv = virDirRead(dir, &ent, dripath)) > 0) {
+ if (!STRPREFIX(ent->d_name, "render"))
+ continue;
+
+ VIR_FREE(devpath);
+ if (virAsprintf(&devpath, "%s/%s", dripath, ent->d_name) <
0)
+ goto cleanup;
+
+ rc = virCgroupAllowDevicePath(priv->cgroup, devpath,
+ VIR_CGROUP_DEVICE_RW, false);
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devpath,
+ "rw", rc == 0);
+ if (rv < 0)
+ goto cleanup;
+ }
+
+ if (rv < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(devpath);
+ VIR_DIR_CLOSE(dir);
+ return ret;
+}
+
+
static int
qemuSetupBlkioCgroup(virDomainObjPtr vm)
{
@@ -604,6 +650,11 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
goto cleanup;
}
+ for (i = 0; i < vm->def->nvideos; i++) {
+ if (qemuSetupVideoCgroup(vm, vm->def->videos[i]) < 0)
+ goto cleanup;
+ }
+
for (i = 0; i < vm->def->ninputs; i++) {
if (qemuSetupInputCgroup(vm, vm->def->inputs[i]) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 515e0052e..06ba1cf00 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7504,6 +7504,67 @@ qemuDomainSetupTPM(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
}
+static int
+qemuDomainSetupVideo(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+ virDomainVideoDefPtr video,
+ const char *devPath)
+{
+ const char *dripath = "/dev/dri";
+ char *dridevpath = NULL;
+ struct dirent *ent;
+ DIR *dir;
+ int rv, ret = -1;
+
+ if (!video->accel ||
+ !video->accel->accel3d)
+ return 0;
+
+ if (virDirOpen(&dir, dripath) < 0)
+ return ret;
+
+ while ((rv = virDirRead(dir, &ent, dripath)) > 0) {
+ if (!STRPREFIX(ent->d_name, "render"))
+ continue;
+
+ VIR_FREE(dridevpath);
+ if (virAsprintf(&dridevpath, "%s/%s", dripath, ent->d_name)
< 0)
+ goto cleanup;
+
+ if (qemuDomainCreateDevice(dridevpath, devPath, false) < 0)
+ goto cleanup;
+ }
+
+ if (rv < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(dridevpath);
+ VIR_DIR_CLOSE(dir);
+ return ret;
+}
+
+
+static int
+qemuDomainSetupAllVideos(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ const char *devPath)
+{
+ size_t i;
+
+ VIR_DEBUG("Setting up videos");
+ for (i = 0; i < vm->def->nvideos; i++) {
+ if (qemuDomainSetupVideo(driver,
+ vm->def->videos[i],
+ devPath) < 0)
+ return -1;
+ }
+
+ VIR_DEBUG("Setup all videos");
+ return 0;
+}
+
+
static int
qemuDomainSetupInput(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
virDomainInputDefPtr input,
@@ -7657,6 +7718,9 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver,
if (qemuDomainSetupTPM(driver, vm, devPath) < 0)
goto cleanup;
+ if (qemuDomainSetupAllVideos(driver, vm, devPath) < 0)
+ goto cleanup;
+
if (qemuDomainSetupAllInputs(driver, vm, devPath) < 0)
goto cleanup;
--
2.11.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Marc-André Lureau
12:42 a.m.
New subject: [libvirt] [PATCH 7/7] qemu: Allow /dev/dri/render* for virgl domains
On 16.02.2017 13:47, Marc-André Lureau wrote:
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
> When enabling virgl, qemu opens /dev/dri/render*. So far, we are
> not allowing that in devices cgroup nor creating the file in
> domain's namespace and thus requiring users to set the paths in
> qemu.conf. This, however, is suboptimal as it allows access to
> ALL qemu processes even those which don't have virgl configured.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>
Thanks, but that doesn't work :)
You should loop over the spice/gl graphics nodes (virtio accel3d is not
actually using 3d, as of today, if the graphics configuration/layer doesn't
provide it)
See also Ján Tomko "qemu_cgroup: allow access to /dev/dri/render*" patch,
which use to work.
After my series "[PATCH 0/5] Add rendernode selection support", it will
further have to narrow the path allowed to the specified rendernode. This
can be done in my series or yours, depending on applied order.
Correct, I've pushed your patches on Friday so now I'll work on allowing
selected render node in cgroup. BTW: what about /dev/dri/card0 and
/dev/dri/controlD4 - do they need to be allowed in devices CGroup too?
BTW: I've merged patches 1-6/7 since you reviewed them.
Thanks!
Michal
2832
days inactive
2842
days old
15 comments
2 participants
participants (2)
-
Marc-André Lureau -
Michal Privoznik