Re: [libvirt] [PATCH v1 27/31] virSecuritySELinuxRestoreImageLabelInt: Don't skip non-local storage

This function is currently not called for any type of storage source that is not considered 'local' (as defined by virStorageSourceIsLocalStorage()). Well, NVMe disks are not 'local' from that point of view and therefore we will need to call this function more frequently. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 38 ++++++++++++++++++++++ src/security/security_selinux.c | 56 ++++++++++++++++++++++++++++----- 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 137daf5d28..d0b84c99b0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -912,6 +912,23 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain,

+ nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + return virSecurityDACSetOwnership(mgr, NULL, vfioGroupDev, user, group, false); + } + /* We can't do restore on shared resources safely. Not even * with refcounting implemented in XATTRs because if there * was a domain running with the feature turned off the @@ -1001,6 +1018,27 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, } } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain,

+ nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + /* Ideally, we would check if there is not another PCI + * device within domain def that is in the same IOMMU + * group. But we're not doing that for hostdevs yet. */ + + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, vfioGroupDev, false); + } + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 99cef3f212..a2e4dcb6da 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1751,9 +1751,8 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, { virSecurityLabelDefPtr seclabel; virSecurityDeviceLabelDefPtr disk_seclabel; - - if (!src->path || !virStorageSourceIsLocalStorage(src)) - return 0; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; + const char *path = src->path; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) @@ -1785,9 +1784,16 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, * ownership, because that kills access on the destination host which is * sub-optimal for the guest VM's I/O attempts :-) */ if (migrated) { - int rc = virFileIsSharedFS(src->path); - if (rc < 0) - return -1; + int rc = 1; + + if (virStorageSourceIsLocalStorage(src)) { + if (!src->path) + return 0; + + if ((rc = virFileIsSharedFS(src->path)) < 0) + return -1; + } + if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared", src->path); @@ -1795,7 +1801,26 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecuritySELinuxRestoreFileLabel(mgr, src->path, true); + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain,

+ nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + /* Ideally, we would check if there is not another PCI + * device within domain def that is in the same IOMMU + * group. But we're not doing that for hostdevs yet. */ + path = vfioGroupDev; + } + + return virSecuritySELinuxRestoreFileLabel(mgr, path, true); } @@ -1820,6 +1845,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; bool remember; + VIR_AUTOFREE(char *) vfioGroupDev = NULL; const char *path = src->path; const char *tcon = NULL; bool optional = false; @@ -1880,6 +1906,22 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, tcon = data->content_context; } + /* This is not very clean. But so far we don't have NVMe + * storage pool backend so that its chownCallback would be + * called. And this place looks least offensive. */ + if (src->type == VIR_STORAGE_TYPE_NVME) { + const virStorageSourceNVMeDef *nvme = src->nvme; + VIR_AUTOUNREF(virPCIDevicePtr) pci = virPCIDeviceNew(nvme->pciAddr.domain,

+ nvme->pciAddr.bus, + nvme->pciAddr.slot, + nvme->pciAddr.function); + if (!pci || + !(vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci))) + return -1; + + path = vfioGroupDev; + } + ret = virSecuritySELinuxSetFileconHelper(mgr, path, tcon, optional, remember); if (ret == 1 && !disk_seclabel) { -- 2.21.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list