[libvirt] [PATCH v4 06/14] security: Enable labeling of vfio mediated devices

Erik Skultety posted 14 patches 8 years, 3 months ago
[libvirt] [PATCH v4 06/14] security: Enable labeling of vfio mediated devices
Posted by Erik Skultety 8 years, 3 months ago
Label the VFIO IOMMU devices under /dev/vfio/ referenced by the symlinks
in the sysfs (e.g. /sys/class/mdev_bus/<uuid>/iommu_group) which what
qemu actually gets formatted on the command line. This patch updates all
of our security drivers.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/security/security_apparmor.c | 21 +++++++++++++++++-
 src/security/security_dac.c      | 45 ++++++++++++++++++++++++++++++++++++--
 src/security/security_selinux.c  | 47 ++++++++++++++++++++++++++++++++++++++--
 3 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index f5b72e1c2d..fc55815261 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -51,6 +51,7 @@
 #include "virlog.h"
 #include "virstring.h"
 #include "virscsi.h"
+#include "virmdev.h"
 
 #define VIR_FROM_THIS VIR_FROM_SECURITY
 
@@ -813,6 +814,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
     virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
     virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
     virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
+    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
 
     if (!secdef || !secdef->relabel)
         return 0;
@@ -901,8 +903,25 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
         break;
     }
 
-    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
+        char *vfiodev = NULL;
+        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
+                                                         mdevsrc->model);
+
+        if (!mdev)
+            goto done;
+
+        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
+            virMediatedDeviceFree(mdev);
+            goto done;
+        }
+
+        ret = AppArmorSetSecurityHostdevLabelHelper(vfiodev, ptr);
+
+        VIR_FREE(vfiodev);
+        virMediatedDeviceFree(mdev);
         break;
+    }
 
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
         ret = 0;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4e968f29c0..922e484942 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -33,6 +33,7 @@
 #include "virfile.h"
 #include "viralloc.h"
 #include "virlog.h"
+#include "virmdev.h"
 #include "virpci.h"
 #include "virusb.h"
 #include "virscsi.h"
@@ -867,6 +868,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
     virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
     virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
     virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
+    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
     int ret = -1;
 
     if (!priv->dynamicOwnership)
@@ -964,7 +966,26 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
         break;
     }
 
-    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
+        char *vfiodev = NULL;
+        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
+                                                         mdevsrc->model);
+
+        if (!mdev)
+            goto done;
+
+        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
+            virMediatedDeviceFree(mdev);
+            goto done;
+        }
+
+        ret = virSecurityDACSetHostdevLabelHelper(vfiodev, &cbdata);
+
+        VIR_FREE(vfiodev);
+        virMediatedDeviceFree(mdev);
+        break;
+    }
+
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
         ret = 0;
         break;
@@ -1032,6 +1053,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
     virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
     virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
     virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
+    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
     int ret = -1;
 
     secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
@@ -1120,7 +1142,26 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
         break;
     }
 
-    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
+        char *vfiodev = NULL;
+        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
+                                                         mdevsrc->model);
+
+        if (!mdev)
+            goto done;
+
+        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
+            virMediatedDeviceFree(mdev);
+            goto done;
+        }
+
+        ret = virSecurityDACRestoreFileLabel(virSecurityManagerGetPrivateData(mgr),
+                                             vfiodev);
+        VIR_FREE(vfiodev);
+        virMediatedDeviceFree(mdev);
+        break;
+    }
+
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
         ret = 0;
         break;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 7b3276dc34..df7c96833e 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -36,6 +36,7 @@
 #include "virerror.h"
 #include "viralloc.h"
 #include "virlog.h"
+#include "virmdev.h"
 #include "virpci.h"
 #include "virusb.h"
 #include "virscsi.h"
@@ -1741,6 +1742,7 @@ virSecuritySELinuxSetHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED,
     return virSecuritySELinuxSetHostdevLabelHelper(file, opaque);
 }
 
+
 static int
 virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
                                         virDomainDefPtr def,
@@ -1752,6 +1754,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
     virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
     virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
     virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
+    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
     virSecuritySELinuxCallbackData data = {.mgr = mgr, .def = def};
 
     int ret = -1;
@@ -1838,7 +1841,26 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
         break;
     }
 
-    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
+        char *vfiodev = NULL;
+        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
+                                                         mdevsrc->model);
+
+        if (!mdev)
+            goto done;
+
+        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
+            virMediatedDeviceFree(mdev);
+            goto done;
+        }
+
+        ret = virSecuritySELinuxSetHostdevLabelHelper(vfiodev, &data);
+
+        VIR_FREE(vfiodev);
+        virMediatedDeviceFree(mdev);
+        break;
+    }
+
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
         ret = 0;
         break;
@@ -1973,6 +1995,7 @@ virSecuritySELinuxRestoreHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED,
     return virSecuritySELinuxRestoreFileLabel(mgr, file);
 }
 
+
 static int
 virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
                                             virDomainHostdevDefPtr dev,
@@ -1983,6 +2006,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
     virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
     virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
     virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
+    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
     int ret = -1;
 
     /* Like virSecuritySELinuxRestoreImageLabelInt() for a networked
@@ -2066,7 +2090,26 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
         break;
     }
 
-    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
+        char *vfiodev = NULL;
+        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
+                                                         mdevsrc->model);
+
+        if (!mdev)
+            goto done;
+
+        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
+            virMediatedDeviceFree(mdev);
+            goto done;
+        }
+
+        ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev);
+
+        VIR_FREE(vfiodev);
+        virMediatedDeviceFree(mdev);
+        break;
+    }
+
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
         ret = 0;
         break;
-- 
2.12.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 06/14] security: Enable labeling of vfio mediated devices
Posted by Laine Stump 8 years, 3 months ago
On 03/22/2017 11:27 AM, Erik Skultety wrote:
> Label the VFIO IOMMU devices under /dev/vfio/ referenced by the symlinks
> in the sysfs (e.g. /sys/class/mdev_bus/<uuid>/iommu_group) which what
> qemu actually gets formatted on the command line. 

The sentence above is confused (i.e. I don't understand it), but I won't
know how to unconfuse it until I've gone through the patch.


> This patch updates all
> of our security drivers.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/security/security_apparmor.c | 21 +++++++++++++++++-
>  src/security/security_dac.c      | 45 ++++++++++++++++++++++++++++++++++++--
>  src/security/security_selinux.c  | 47 ++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 108 insertions(+), 5 deletions(-)
> 
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index f5b72e1c2d..fc55815261 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -51,6 +51,7 @@
>  #include "virlog.h"
>  #include "virstring.h"
>  #include "virscsi.h"
> +#include "virmdev.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>  
> @@ -813,6 +814,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
>      virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>      virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
> +    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
>  
>      if (!secdef || !secdef->relabel)
>          return 0;
> @@ -901,8 +903,25 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> +        char *vfiodev = NULL;
> +        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
> +                                                         mdevsrc->model);
> +
> +        if (!mdev)
> +            goto done;
> +
> +        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
> +            virMediatedDeviceFree(mdev);
> +            goto done;
> +        }

Going through the various patches and seeing this (or similar) sequences
so often makes me think it might be cleaner to have APIs that take a
uuidstr and model instead (or maybe define
virDomainHostdevSubsysMediatedDevPtr in util instead of conf, then pass
the mdevsrc directly - that would make it continue to work if/once we
add different ways to specify the device.

But things currently work exactly the same way for PCI devices, so no
sense rewriting just for that. These are all internal APIs, so we can
tweak them to our hearts' content in the future.


> +
> +        ret = AppArmorSetSecurityHostdevLabelHelper(vfiodev, ptr);
> +
> +        VIR_FREE(vfiodev);
> +        virMediatedDeviceFree(mdev);
>          break;
> +    }
>  
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 4e968f29c0..922e484942 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -33,6 +33,7 @@
>  #include "virfile.h"
>  #include "viralloc.h"
>  #include "virlog.h"
> +#include "virmdev.h"
>  #include "virpci.h"
>  #include "virusb.h"
>  #include "virscsi.h"
> @@ -867,6 +868,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
>      virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>      virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
> +    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
>      int ret = -1;
>  
>      if (!priv->dynamicOwnership)
> @@ -964,7 +966,26 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> +        char *vfiodev = NULL;
> +        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
> +                                                         mdevsrc->model);
> +
> +        if (!mdev)
> +            goto done;
> +
> +        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
> +            virMediatedDeviceFree(mdev);
> +            goto done;
> +        }

(see what I mean?)

> +
> +        ret = virSecurityDACSetHostdevLabelHelper(vfiodev, &cbdata);
> +
> +        VIR_FREE(vfiodev);
> +        virMediatedDeviceFree(mdev);
> +        break;
> +    }
> +
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
>          break;
> @@ -1032,6 +1053,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
>      virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>      virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
> +    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
>      int ret = -1;
>  
>      secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> @@ -1120,7 +1142,26 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> +        char *vfiodev = NULL;
> +        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
> +                                                         mdevsrc->model);
> +
> +        if (!mdev)
> +            goto done;
> +
> +        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
> +            virMediatedDeviceFree(mdev);
> +            goto done;
> +        }
> +
> +        ret = virSecurityDACRestoreFileLabel(virSecurityManagerGetPrivateData(mgr),
> +                                             vfiodev);
> +        VIR_FREE(vfiodev);
> +        virMediatedDeviceFree(mdev);
> +        break;
> +    }
> +
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
>          break;
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 7b3276dc34..df7c96833e 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -36,6 +36,7 @@
>  #include "virerror.h"
>  #include "viralloc.h"
>  #include "virlog.h"
> +#include "virmdev.h"
>  #include "virpci.h"
>  #include "virusb.h"
>  #include "virscsi.h"
> @@ -1741,6 +1742,7 @@ virSecuritySELinuxSetHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED,
>      return virSecuritySELinuxSetHostdevLabelHelper(file, opaque);
>  }
>  
> +
>  static int
>  virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
>                                          virDomainDefPtr def,
> @@ -1752,6 +1754,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
>      virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>      virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
> +    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
>      virSecuritySELinuxCallbackData data = {.mgr = mgr, .def = def};
>  
>      int ret = -1;
> @@ -1838,7 +1841,26 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> +        char *vfiodev = NULL;
> +        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
> +                                                         mdevsrc->model);
> +
> +        if (!mdev)
> +            goto done;
> +
> +        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
> +            virMediatedDeviceFree(mdev);
> +            goto done;
> +        }
> +
> +        ret = virSecuritySELinuxSetHostdevLabelHelper(vfiodev, &data);
> +
> +        VIR_FREE(vfiodev);
> +        virMediatedDeviceFree(mdev);
> +        break;
> +    }
> +
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
>          break;
> @@ -1973,6 +1995,7 @@ virSecuritySELinuxRestoreHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED,
>      return virSecuritySELinuxRestoreFileLabel(mgr, file);
>  }
>  
> +
>  static int
>  virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
>                                              virDomainHostdevDefPtr dev,
> @@ -1983,6 +2006,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
>      virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>      virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
> +    virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
>      int ret = -1;
>  
>      /* Like virSecuritySELinuxRestoreImageLabelInt() for a networked
> @@ -2066,7 +2090,26 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,
>          break;
>      }
>  
> -    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> +        char *vfiodev = NULL;
> +        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
> +                                                         mdevsrc->model);
> +
> +        if (!mdev)
> +            goto done;
> +
> +        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
> +            virMediatedDeviceFree(mdev);
> +            goto done;
> +        }
> +
> +        ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev);
> +
> +        VIR_FREE(vfiodev);
> +        virMediatedDeviceFree(mdev);
> +        break;
> +    }
> +
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>          ret = 0;
>          break;
> 


ACK.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 06/14] security: Enable labeling of vfio mediated devices
Posted by Erik Skultety 8 years, 3 months ago
On Sun, Mar 26, 2017 at 02:25:02PM -0400, Laine Stump wrote:
> On 03/22/2017 11:27 AM, Erik Skultety wrote:
> > Label the VFIO IOMMU devices under /dev/vfio/ referenced by the symlinks
> > in the sysfs (e.g. /sys/class/mdev_bus/<uuid>/iommu_group) which what
> > qemu actually gets formatted on the command line. 
> 
> The sentence above is confused (i.e. I don't understand it), but I won't
> know how to unconfuse it until I've gone through the patch.

Now that I'm reading it again, of course I know what I meant, but that's only
because I wrote it, but from the native speaker's perspective, I guess the
reaction must have been something like "wut?!". So I simplified it to the bare
minimum in terms of the patch just adding labeling for mdevs as well.

[...]

> >
> > -    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> > +        char *vfiodev = NULL;
> > +        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
> > +                                                         mdevsrc->model);
> > +
> > +        if (!mdev)
> > +            goto done;
> > +
> > +        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
> > +            virMediatedDeviceFree(mdev);
> > +            goto done;
> > +        }
> 
> Going through the various patches and seeing this (or similar) sequences
> so often makes me think it might be cleaner to have APIs that take a
> uuidstr and model instead (or maybe define
> virDomainHostdevSubsysMediatedDevPtr in util instead of conf, then pass
> the mdevsrc directly - that would make it continue to work if/once we
> add different ways to specify the device.
> 
> But things currently work exactly the same way for PCI devices, so no
> sense rewriting just for that. These are all internal APIs, so we can
> tweak them to our hearts' content in the future.
> 

Yeah, there's definitely some room for 'refurbishment' of the hostdev code.
I'll put that on my TODO list.

> > -    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
> > +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> > +        char *vfiodev = NULL;
> > +        virMediatedDevicePtr mdev = virMediatedDeviceNew(mdevsrc->uuidstr,
> > +                                                         mdevsrc->model);
> > +
> > +        if (!mdev)
> > +            goto done;
> > +
> > +        if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdev))) {
> > +            virMediatedDeviceFree(mdev);
> > +            goto done;
> > +        }
> 
> (see what I mean?)

Yep.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list