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 <[email protected]>
> ---
> 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
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
