Change in vdsm[ovirt-3.5]: lvm: Do not use udev to set permissions on vdsm images

nsoffer at redhat.com nsoffer at redhat.com
Wed Oct 8 13:01:15 UTC 2014


Hello Federico Simoncelli,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/33931

to review the following change.

Change subject: lvm: Do not use udev to set permissions on vdsm images
......................................................................

lvm: Do not use udev to set permissions on vdsm images

udev has changed the rules recently, so setting USER, GROUP, or MODE
will change also the selinux label behind your back. This issue caused
vms to pause after extend, rendering thin provisioning disk useless.

We had a temporary fix, applying a static selinux label on vdsm images,
but this breaks libvirt security. This patch avoids this issue by not
using udev to set any permissions, thus preserving the secure selinux
labels set by libvirt.

This patch replaces the following commits with a simpler and hopefully
longer lasting fix which does not need any platform specific code.

d8d6c17 gitignore: Ingore vdsm-lvm.rule.tpl
00fbc83 lvm: Modify lv selinux label only if not labablled as libvirt image
b2268e4 spec: Enable lvm selinux fix for Fedora
75fc495 lvm: Set libvirt image selinux label on block devices backing vdsm images

Change-Id: I57d9987bf0be19e6e233baaeea10877918eb849b
Bug-Url: https://bugzilla.redhat.com/1149883
Relates-To: https://bugzilla.redhat.com/1147910
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/33875
Reviewed-by: Federico Simoncelli <fsimonce at redhat.com>
---
M .gitignore
M configure.ac
M vdsm.spec.in
M vdsm/storage/Makefile.am
D vdsm/storage/vdsm-chcon.in
R vdsm/storage/vdsm-lvm.rules.in
6 files changed, 9 insertions(+), 78 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/31/33931/1

diff --git a/.gitignore b/.gitignore
index da260eb..efb99aa 100644
--- a/.gitignore
+++ b/.gitignore
@@ -57,9 +57,7 @@
 vdsm/sos/vdsm.py
 vdsm/storage/protect/safelease
 vdsm/storage/lvm.env
-vdsm/storage/vdsm-chcon
 vdsm/storage/vdsm-lvm.rules
-vdsm/storage/vdsm-lvm.rules.tpl
 vdsm/sudoers.vdsm
 vdsm/svdsm.logger.conf
 vdsm/vdscli.py
diff --git a/configure.ac b/configure.ac
index 9938ba1..afe6d32 100644
--- a/configure.ac
+++ b/configure.ac
@@ -56,17 +56,6 @@
 AM_CONDITIONAL([HOOKS], [test "${enable_hooks}" = "yes"])
 
 AC_ARG_ENABLE(
-    [chcon_hack],
-    [AS_HELP_STRING(
-        [--enable-chcon-hack],
-        [enable chcon hack for block devices @<:@default=no@:>@]
-    )],
-    ,
-    [enable_chcon_hack="no"]
-)
-AM_CONDITIONAL([CHCON_HACK], [test "${enable_chcon_hack}" = "yes"])
-
-AC_ARG_ENABLE(
     [libvirt-sanlock],
     [AS_HELP_STRING(
         [--disable-libvirt-sanlock],
@@ -121,11 +110,6 @@
     [with_libvirt_service_default="${sysconfdir}/sysconfig/libvirtd"]
 )
 AC_SUBST([LIBVIRT_SERVICE_DEFAULT], ["${with_libvirt_service_default}"])
-
-
-# Selinux image label
-AC_SUBST([SVIRT_IMAGE_LABEL], ['svirt_image_t'])
-AC_SUBST([SVIRT_CONTENT_LABEL], ['svirt_content_t'])
 
 
 # Users and groups
@@ -208,7 +192,6 @@
 AC_PATH_PROG([BLKID_PATH], [blkid], [/sbin/blkid])
 AC_PATH_PROG([BRCTL_PATH], [brctl], [/usr/sbin/brctl])
 AC_PATH_PROG([CAT_PATH], [cat], [/bin/cat])
-AC_PATH_PROG([CHCON_PATH], [chcon], [/bin/chcon])
 AC_PATH_PROG([CHKCONFIG_PATH], [chkconfig], [/sbin/chkconfig])
 AC_PATH_PROG([CHMOD_PATH], [chmod], [/bin/chmod])
 AC_PATH_PROG([CHOWN_PATH], [chown], [/bin/chown])
@@ -231,7 +214,6 @@
 AC_PATH_PROG([ISCSIADM_PATH], [iscsiadm], [/sbin/iscsiadm])
 AC_PATH_PROG([KILL_PATH], [kill], [/bin/kill])
 AC_PATH_PROG([LSBLK_PATH], [lsblk], [/bin/lsblk])
-AC_PATH_PROG([LS_PATH], [ls], [/bin/ls])
 AC_PATH_PROG([LVM_PATH], [lvm], [/sbin/lvm])
 AC_PATH_PROG([MKFS_MSDOS_PATH], [mkfs.msdos], [/sbin/mkfs.msdos])
 AC_PATH_PROG([MKFS_PATH], [mkfs], [/sbin/mkfs])
@@ -300,8 +282,7 @@
 	vdsm/storage/Makefile
 	vdsm/storage/imageRepository/Makefile
 	vdsm/storage/protect/Makefile
-	vdsm/storage/vdsm-chcon
-	vdsm/storage/vdsm-lvm.rules.tpl
+	vdsm/storage/vdsm-lvm.rules
 	vdsm/virt/Makefile
 	vdsm_hooks/Makefile
 	vdsm_hooks/checkimages/Makefile
diff --git a/vdsm.spec.in b/vdsm.spec.in
index 3e42f37..fd8c709 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -35,10 +35,6 @@
 %global with_vhostmd 1
 %endif
 
-%if 0%{?fedora} >= 19 || 0%{?rhel} >= 7
-%global with_chcon_hack 1
-%endif
-
 %if 0%{?fedora} >= 15 || 0%{?rhel} >= 7
 %global with_systemd 1
 %endif
@@ -643,7 +639,7 @@
 %if 0%{?enable_autotools}
 autoreconf -if
 %endif
-%configure %{?with_hooks:--enable-hooks} %{?with_chcon_hack:--enable-chcon-hack}
+%configure %{?with_hooks:--enable-hooks}
 make
 # Setting software_version and software_revision in dsaversion.py
 baserelease=`echo "%{release}" | sed 's/\([0-9]\+\(\.[0-9]\+\)\?\).*/\1/'`
@@ -669,11 +665,6 @@
 # Install the lvm rules
 install -Dm 0644 vdsm/storage/vdsm-lvm.rules \
                  %{buildroot}%{_udevrulesdir}/12-vdsm-lvm.rules
-
-%if 0%{?with_chcon_hack}
-install -Dm 0755 vdsm/storage/vdsm-chcon \
-                 %{buildroot}%{_udevexecdir}/vdsm-chcon
-%endif
 
 install -Dm 0644 vdsm/limits.conf \
                  %{buildroot}/etc/security/limits.d/99-vdsm.conf
@@ -1150,9 +1141,6 @@
 %endif
 %{python_sitelib}/sos/plugins/vdsm.py*
 %{_udevrulesdir}/12-vdsm-lvm.rules
-%if 0%{?with_chcon_hack}
-%{_udevexecdir}/vdsm-chcon
-%endif
 /etc/security/limits.d/99-vdsm.conf
 %{_mandir}/man8/vdsmd.8*
 %if 0%{?rhel}
diff --git a/vdsm/storage/Makefile.am b/vdsm/storage/Makefile.am
index 89fa1e5..99b1460 100644
--- a/vdsm/storage/Makefile.am
+++ b/vdsm/storage/Makefile.am
@@ -81,22 +81,3 @@
 EXTRA_DIST = \
 	lvm.env.in \
 	$(NULL)
-
-all-local: vdsm-lvm.rules
-
-vdsm-lvm.rules: vdsm-lvm.rules.tpl
-if CHCON_HACK
-	python -c '\
-		import sys, re; \
-		s = open(sys.argv[1]).read(); \
-		pat = re.compile(r"{{.+?}}\n?", re.S); \
-		s = pat.sub("", s); \
-		sys.stdout.write(s)' $< > $@;
-else
-	python -c '\
-		import sys, re; \
-		s = open(sys.argv[1]).read(); \
-		pat = re.compile(r"{{if chcon_hack}}\n?.+?{{endif}}\n?", re.S); \
-		s = pat.sub("", s); \
-		sys.stdout.write(s)' $< > $@;
-endif
diff --git a/vdsm/storage/vdsm-chcon.in b/vdsm/storage/vdsm-chcon.in
deleted file mode 100644
index 6f1eb6e..0000000
--- a/vdsm/storage/vdsm-chcon.in
+++ /dev/null
@@ -1,14 +0,0 @@
-#!/bin/sh
-
-# This script must be called from a udev rule and assumes the udev environment
-# variables.
-
-# Do not touch the device if it is already labelled is libvirt image. It will
-# probably be a fixed_disk_t or it may have no selinux label.
-if @LS_PATH@ -Z "$DEVNAME" | \
-    @GREP_PATH@ -q -E ":@SVIRT_CONTENT_LABEL@:|:@SVIRT_IMAGE_LABEL@:"; then
-    exit 0
-fi
-
-echo "Changing selinux type to @SVIRT_IMAGE_LABEL@ on $DEVNAME" >&2
- at CHCON_PATH@ -t @SVIRT_IMAGE_LABEL@ "$DEVNAME"
diff --git a/vdsm/storage/vdsm-lvm.rules.tpl.in b/vdsm/storage/vdsm-lvm.rules.in
similarity index 89%
rename from vdsm/storage/vdsm-lvm.rules.tpl.in
rename to vdsm/storage/vdsm-lvm.rules.in
index fb6c87a..7a1a1b2 100644
--- a/vdsm/storage/vdsm-lvm.rules.tpl.in
+++ b/vdsm/storage/vdsm-lvm.rules.in
@@ -16,20 +16,17 @@
 #   DM_LV_NAME - logical volume name
 #   DM_VG_NAME - volume group name
 #   DM_LV_LAYER - logical volume layer (blank if not set)
-{{if chcon_hack}}
-#
-# The libvirt image label is required to allow qemu to access volumes. Libvirt
-# sets this label on volumes when starting a vm, but on EL7 and Fedora the
-# label is lost after refreshing a logical volume, and vm get paused. This rule
-# ensures that the label exist after device changes. See
-# https://bugzilla.redhat.com/1147910
-{{endif}}
 
 # "add" event is processed on coldplug only, so we need "change", too.
 ACTION!="add|change", GOTO="lvm_end"
 
-# Fix ownership for RHEV volumes
-ENV{DM_VG_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", ENV{DM_LV_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", OWNER:="@VDSMUSER@", GROUP:="@QEMUGROUP@"{{if chcon_hack}}, RUN+="vdsm-chcon"{{endif}}, GOTO="lvm_end"
+# Volumes used as vdsm images
+# WARNING: we cannot use USER, GROUP and MODE since using any of them will
+# change the selinux label to the default, causing vms to pause after extending
+# disks. https://bugzilla.redhat.com/1147910
+ENV{DM_VG_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", ENV{DM_LV_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", RUN+="@CHOWN_PATH@ @VDSMUSER@:@QEMUGROUP@ $env{DEVNAME}", GOTO="lvm_end"
+
+# Other volumes used by vdsm
 ENV{DM_VG_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", ENV{DM_LV_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]_MERGE", OWNER:="@VDSMUSER@", GROUP:="@QEMUGROUP@", GOTO="lvm_end"
 ENV{DM_VG_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", ENV{DM_LV_NAME}=="_remove_me_[a-zA-Z0-9][a-zA-Z0-9][a-zA-Z0-9][a-zA-Z0-9][a-zA-Z0-9][a-zA-Z0-9][a-zA-Z0-9][a-zA-Z0-9]_[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", OWNER:="@VDSMUSER@", GROUP:="@QEMUGROUP@", GOTO="lvm_end"
 ENV{DM_VG_NAME}=="[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9]-[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]", ENV{DM_LV_NAME}=="metadata", MODE:="0600", OWNER:="@VDSMUSER@", GROUP:="@QEMUGROUP@", GOTO="lvm_end"


-- 
To view, visit http://gerrit.ovirt.org/33931
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I57d9987bf0be19e6e233baaeea10877918eb849b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>


More information about the vdsm-patches mailing list