[libvirt] [PATCH 0/7] qemu: Be more cautious about allowed devices
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
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@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
Hi On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
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@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
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
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@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
Hi On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
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@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
Hi On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@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@redhat.com> 2.11.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
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@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
Hi On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@redhat.com>
looks good to me, Reviewed-by: Marc-André Lureau <marcandre.lureau@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
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@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
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@redhat.com>
Fine approach to me, Reviewed-by: Marc-André Lureau <marcandre.lureau@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
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@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
Hi On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
On 16.02.2017 13:47, Marc-André Lureau wrote:
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn@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@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
participants (2)
-
Marc-André Lureau -
Michal Privoznik