The summary is misleading. Mention NVNe and since you are fixing two
functions don't mention the name.
On Thu, Jul 11, 2019 at 17:54:14 +0200, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
> ---
> 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,
This is not a virObject.
> + 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,
Same as above.
> + 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,
same as above.
> + 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,
same as above.
> + 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
