On 26.11.2014 19:11, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1082521 > > Support for shared hostdev's was added in a number of commits, initially > starting with 'f2c1d9a80' and most recently commit id 'fd243fc4' to fix > issues with the initial implementation. Missed in all those changes was > the need to mimic the virSELinux{Set|Restore}SecurityDiskLabel code to > handle the "shared" (or shareable) and readonly options when Setting > or Restoring the SELinux labels. > > This patch will adjust the virSecuritySELinuxSetSecuritySCSILabel to not > use the virSecuritySELinuxSetSecurityHostdevLabelHelper in order to set > the label. Rather follow what the Disk code does by setting the label > differently based on whether shareable/readonly is set. This patch will > also modify the virSecuritySELinuxRestoreSecuritySCSILabel to follow > the same logic as virSecuritySELinuxRestoreSecurityImageLabelInt and not > restore the label if shared/readonly > > Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > --- > src/security/security_selinux.c | 58 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 49 insertions(+), 9 deletions(-) > > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c > index f96be50..3a794b8 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -71,6 +71,12 @@ struct _virSecuritySELinuxData { > #define SECURITY_SELINUX_VOID_DOI "0" > #define SECURITY_SELINUX_NAME "selinux" > > +/* Data structure to pass to *FileIterate so we have everything we need */ > +struct SELinuxSCSICallbackData { > + virSecurityManagerPtr mgr; > + virDomainDefPtr def; > +}; > + > static int > virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr, > virDomainDefPtr def, > @@ -1143,7 +1149,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, > !src->backingStore) > return 0; > > - /* Don't restore labels on readoly/shared disks, because other VMs may > + /* Don't restore labels on readonly/shared disks, because other VMs may > * still be accessing these. Alternatively we could iterate over all > * running domains and try to figure out if it is in use, but this would > * not work for clustered filesystems, since we can't see running VMs using > @@ -1309,14 +1315,31 @@ virSecuritySELinuxSetSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, > } > > static int > -virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, > +virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev, > const char *file, void *opaque) > { > - return virSecuritySELinuxSetSecurityHostdevLabelHelper(file, opaque); > + virSecurityLabelDefPtr secdef; > + struct SELinuxSCSICallbackData *ptr = opaque; > + virSecurityManagerPtr mgr = ptr->mgr; > + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); > + > + secdef = virDomainDefGetSecurityLabelDef(ptr->def, SECURITY_SELINUX_NAME); > + if (secdef == NULL) > + return 0; > + > + if (virSCSIDeviceGetShareable(dev)) > + return virSecuritySELinuxSetFileconOptional(file, > + data->file_context); > + else if (virSCSIDeviceGetReadonly(dev)) > + return virSecuritySELinuxSetFileconOptional(file, > + data->content_context); > + else > + return virSecuritySELinuxSetFileconOptional(file, secdef->imagelabel); > } > > static int > -virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def, > +virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, > + virDomainDefPtr def, > virDomainHostdevDefPtr dev, > const char *vroot) > > @@ -1377,17 +1400,27 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def, > > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { > virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; > + struct SELinuxSCSICallbackData *ptr; I think this can be a static variable. I mean, variable allocated on stack, not on the heap. > + > + if (VIR_ALLOC(ptr) < 0) > + goto done; Subsequently, VIR_ALLOC can just go away. > + ptr->mgr = mgr; > + ptr->def = def; > + > virSCSIDevicePtr scsi = > virSCSIDeviceNew(NULL, > scsihostsrc->adapter, scsihostsrc->bus, > scsihostsrc->target, scsihostsrc->unit, > dev->readonly, dev->shareable); > > - if (!scsi) > + if (!scsi) { > + VIR_FREE(ptr); > goto done; > + } > > - ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, def); > + ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, ptr); > virSCSIDeviceFree(scsi); > + VIR_FREE(ptr); And so can VIR_FREE(). ACK with this squashed in: diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 3a794b8..8942f5f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1400,12 +1400,7 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; - struct SELinuxSCSICallbackData *ptr; - - if (VIR_ALLOC(ptr) < 0) - goto done; - ptr->mgr = mgr; - ptr->def = def; + struct SELinuxSCSICallbackData data = {.mgr = mgr, .def = def}; virSCSIDevicePtr scsi = virSCSIDeviceNew(NULL, @@ -1413,14 +1408,11 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, scsihostsrc->target, scsihostsrc->unit, dev->readonly, dev->shareable); - if (!scsi) { - VIR_FREE(ptr); + if (!scsi) goto done; - } - ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, ptr); + ret = virSCSIDeviceFileIterate(scsi, virSecuritySELinuxSetSecuritySCSILabel, &data); virSCSIDeviceFree(scsi); - VIR_FREE(ptr); break; } Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list