Bug 1189275

+++ This bug was initially created as a clone of Bug #1147910 +++

Description of problem:
When a dm is resumed systemd-udevd changes its selinux context:

# dmsetup suspend 539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6

# ls -Z /dev/dm-4
brw-rw----. vdsm qemu system_u:object_r:svirt_image_t:s0 /dev/dm-4

# dmsetup resume 539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6

# ls -Z /dev/dm-4
brw-rw----. vdsm qemu system_u:object_r:fixed_disk_device_t:s0 /dev/dm-4

Version-Release number of selected component (if applicable):
systemd-208-22.fc20.x86_64

How reproducible:
100%

Steps to Reproduce:
1. start a VM on a dm device using libvirt
2. refresh the dm (needed for example after a resize, anyway dmsetup suspend/resume are enough to trigger the issue)

Actual results:
The VM is paused (or its IO fails, depending on the VM config) since it can't write to the dm anymore.

Expected results:
The VM should keep running (no IO failures) even after a refresh of the dm.

Additional info:

Debug + strace of /usr/lib/systemd/systemd-udevd:

RUN '/usr/sbin/dmsetup udevcomplete $env{DM_COOKIE}' /usr/lib/udev/rules.d/95-dm-notify.rules:12
handling device node '/dev/dm-4', devnum=b253:4, mode=0660, uid=36, gid=107
preserve permissions /dev/dm-4, 060660, uid=36, gid=107
[pid 23728] lsetxattr("/dev/dm-4", "security.selinux", "system_u:object_r:fixed_disk_device_t:s0", 41, 0) = 0
preserve already existing symlink '/dev/block/253:4' to '../dm-4'
[pid 23728] lsetxattr("/dev/block/253:4", "security.selinux", "system_u:object_r:device_t:s0", 30, 0) = 0
found 'b253:4' claiming '/run/udev/links/\x2f539cfcda-bc30-4e35-845e-888a58229e52\x2f1d84a603-1a4a-461a-af1e-c1541f5f12b6'
creating link '/dev/539cfcda-bc30-4e35-845e-888a58229e52/1d84a603-1a4a-461a-af1e-c1541f5f12b6' to '/dev/dm-4'
preserve already existing symlink '/dev/539cfcda-bc30-4e35-845e-888a58229e52/1d84a603-1a4a-461a-af1e-c1541f5f12b6' to '../dm-4'
[pid 23728] lsetxattr("/dev/539cfcda-bc30-4e35-845e-888a58229e52/1d84a603-1a4a-461a-af1e-c1541f5f12b6", "security.selinux", "system_u:object_r:device_t:s0", 30, 0) = 0
found 'b253:4' claiming '/run/udev/links/\x2fdisk\x2fby-id\x2fdm-name-539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6'
creating link '/dev/disk/by-id/dm-name-539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6' to '/dev/dm-4'
preserve already existing symlink '/dev/disk/by-id/dm-name-539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6' to '../../dm-4'
[pid 23728] lsetxattr("/dev/disk/by-id/dm-name-539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6", "security.selinux", "system_u:object_r:device_t:s0", 30, 0) = 0
found 'b253:4' claiming '/run/udev/links/\x2fdisk\x2fby-id\x2fdm-uuid-LVM-RBGwiMghcHZfY95VC5SeHFT20xaKJp1EIOBOK3Ejxq1FTfDKI4HngfgKoihn8G3p'
creating link '/dev/disk/by-id/dm-uuid-LVM-RBGwiMghcHZfY95VC5SeHFT20xaKJp1EIOBOK3Ejxq1FTfDKI4HngfgKoihn8G3p' to '/dev/dm-4'
preserve already existing symlink '/dev/disk/by-id/dm-uuid-LVM-RBGwiMghcHZfY95VC5SeHFT20xaKJp1EIOBOK3Ejxq1FTfDKI4HngfgKoihn8G3p' to '../../dm-4'
[pid 23728] lsetxattr("/dev/disk/by-id/dm-uuid-LVM-RBGwiMghcHZfY95VC5SeHFT20xaKJp1EIOBOK3Ejxq1FTfDKI4HngfgKoihn8G3p", "security.selinux", "system_u:object_r:device_t:s0", 30, 0) = 0
found 'b253:4' claiming '/run/udev/links/\x2fmapper\x2f539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6'
creating link '/dev/mapper/539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6' to '/dev/dm-4'
preserve already existing symlink '/dev/mapper/539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6' to '../dm-4'
[pid 23728] lsetxattr("/dev/mapper/539cfcda--bc30--4e35--845e--888a58229e52-1d84a603--1a4a--461a--af1e--c1541f5f12b6", "security.selinux", "system_u:object_r:device_t:s0", 30, 0) = 0
created db file '/run/udev/data/b253:4' for '/devices/virtual/block/dm-4'

--- Additional comment from Federico Simoncelli on 2014-09-30 05:54:14 EDT ---

It seems that as systemd-udevd is preserving the permissions:

 ...
 preserve permissions /dev/dm-4, 060660, uid=36, gid=107
 ...

it should also preserve the selinux context instead of resetting it with:

 ...
 lsetxattr("/dev/dm-4", "security.selinux", "system_u:object_r:fixed_disk_device_t:s0", 41, 0) = 0
 ...

--- Additional comment from Sandro Bonazzola on 2014-09-30 07:05:20 EDT ---

Not sure it's the same issue but for bug #1142709 I opened bugs on selinux (bug #1146529, bug #1146531) can you see if they're relevant?

--- Additional comment from Federico Simoncelli on 2014-09-30 07:21:28 EDT ---

It seems that the relevant change is:

 22582bb udev: set default rules permissions only at "add" events

http://cgit.freedesktop.org/systemd/systemd/commit/?id=22582bb2cbe85b40de5f561589e0468dac769515

-        if (strcmp(udev_device_get_action(dev), "add") == 0) {
+        if (apply) {
...
                 label_fix(devnode, true, false);
         }

--- Additional comment from Federico Simoncelli on 2014-09-30 09:51:35 EDT ---



--- Additional comment from Federico Simoncelli on 2014-09-30 09:52:06 EDT ---



--- Additional comment from Federico Simoncelli on 2014-09-30 09:53:08 EDT ---

Attached patches for master and fedora 20 are solving the issue.

--- Additional comment from Federico Simoncelli on 2014-09-30 10:48:53 EDT ---

(In reply to Sandro Bonazzola from comment #2)
> Not sure it's the same issue but for bug #1142709 I opened bugs on selinux
> (bug #1146529, bug #1146531) can you see if they're relevant?

It's not related because in that case you use a fully preallocated disk (at least from what I saw from the vdsm.log attached).

Anyway only who's familiar with bug 1142709 can tell if there are other unknown implications.

--- Additional comment from Milos Malik on 2014-10-01 05:44:10 EDT ---

Which version of selinux-policy packages do you have on your system ?

I don't know anything about dmsetup, but selinux-policy-3.12.1-153.el7_0.11 says:

# matchpathcon `which dmsetup`
/usr/sbin/dmsetup	system_u:object_r:lvm_exec_t:s0
# sesearch -s lvm_t -t device_t -T
Found 1 semantic te rules:
   type_transition lvm_t device_t : blk_file fixed_disk_device_t; 

#

--- Additional comment from Kay Sievers on 2014-10-01 05:58:35 EDT ---

Looks like the rules need to be changed to apply MODE= and GROUP= only
at "change" events, then the additional conditional is not needed:
  http://cgit.freedesktop.org/systemd/systemd/commit/?id=22582bb2cbe85b40de5f561589e0468dac769515

--- Additional comment from Federico Simoncelli on 2014-10-01 06:57:53 EDT ---

(In reply to Kay Sievers from comment #9)
> Looks like the rules need to be changed to apply MODE= and GROUP= only
> at "change" events, then the additional conditional is not needed:
>  
> http://cgit.freedesktop.org/systemd/systemd/commit/
> ?id=22582bb2cbe85b40de5f561589e0468dac769515

Yes this is something mentioned in comment 3.

In my patches I didn't know if it was possible to skip both permissions change and selinux relabel so I limited it to selinux.

Also because there weren't many information in:

 9a8ae49 udev: always set selinux label at "add" events
 22582bb udev: set default rules permissions only at "add" events

if they were just refactoring then the apply rule should have been just:

 apply = streq(udev_device_get_action(dev), "add");

(IIUC this is what is also suggested in comment 9).

--- Additional comment from Lukáš Nykrýn on 2014-10-01 08:48:09 EDT ---

Set of koji builds:

http://koji.fedoraproject.org/koji/taskinfo?taskID=7740296 http://koji.fedoraproject.org/koji/taskinfo?taskID=7740304 http://koji.fedoraproject.org/koji/taskinfo?taskID=7740309

--- Additional comment from Lukáš Nykrýn on 2014-10-01 08:48:57 EDT ---

But please note that this solution is short term workaround and we should find a better solution.

--- Additional comment from Kay Sievers on 2014-10-01 09:03:48 EDT ---

(In reply to Federico Simoncelli from comment #10)

> if they were just refactoring then the apply rule should have been just:
> 
>  apply = streq(udev_device_get_action(dev), "add");

No, the intendend behavior is that if rules explicitely set something,
also during "change" it should be fully applied.

--- Additional comment from Kay Sievers on 2014-10-01 10:31:09 EDT ---

These patches are not acceptable for Fedora.

I will remove them before the next release. This needs to be solved properly
and not by dirty hacks adding condition and complex implicit rules to udev
code.

Udev is not supposed to be patched like that and we do not add such special
handling.

The disconnect of libvirt mangling device node permissions and triggering
udev rules to do the same is not and will not supported by udev.

Please apply, if possible, your rules on "add" only, so udev will not
reset any metadata on "change" events. Or have libvirt or whatever tool
needs it, write-out the temporary udev rules which also set the selinux
label along with the other properties.

--- Additional comment from Federico Simoncelli on 2014-10-01 10:53:22 EDT ---

(In reply to Kay Sievers from comment #14)
> Please apply, if possible, your rules on "add" only, so udev will not
> reset any metadata on "change" events. Or have libvirt or whatever tool
> needs it, write-out the temporary udev rules which also set the selinux
> label along with the other properties.

Thanks, so it seems that we could solve this by simply updating 12-vdsm-lvm.rules with:

-ACTION!="add", GOTO="lvm_end"
+ACTION!="add|change", GOTO="lvm_end"

I briefly checked it and it seems to work, Nir can you verify it as well? (With a real VM running possibly) Thanks.
At the moment I can't think of any harmful side-effect.

--- Additional comment from Nir Soffer on 2014-10-01 11:24:26 EDT ---

Kay, our rules are basically:

ACTION=="add|change", ENV{DM_VG_NAME}=="...", OWNER:="vdsm", GROUP:="qemu"

Can you explain why modifying the owner and group change the selinux label, and where it is documented that changing these will change the selinux label?

This used to work on RHEL 6.5 for years - why it stopped working now?

--- Additional comment from Nir Soffer on 2014-10-01 11:26:32 EDT ---

(In reply to Federico Simoncelli from comment #15)
> (In reply to Kay Sievers from comment #14)
> > Please apply, if possible, your rules on "add" only, so udev will not
> > reset any metadata on "change" events. Or have libvirt or whatever tool
> > needs it, write-out the temporary udev rules which also set the selinux
> > label along with the other properties.
> 
> Thanks, so it seems that we could solve this by simply updating
> 12-vdsm-lvm.rules with:
> 
> -ACTION!="add", GOTO="lvm_end"
> +ACTION!="add|change", GOTO="lvm_end"
> 
> I briefly checked it and it seems to work, Nir can you verify it as well?
> (With a real VM running possibly) Thanks.
> At the moment I can't think of any harmful side-effect.

We need to understand first, why rules are using "add|change" - maybe it was a hack to avoid a some problem in older rhel?

--- Additional comment from Kay Sievers on 2014-10-01 11:32:51 EDT ---

Device nodes are "owned" by udev if udev is instructed to apply any changes to
them, that includes the selinux label or any other of the metadata.

The split of mangling of permissions/metadata of device nodes between
different tools is not acceptable and not long-term maintainable. Clear
ownership rules are needed, there can only be one tool that manages
them.

The option here is to not tell udev to manage the permissions, meaning
no rules that set them, or tell udev all the needed data in runtime rules.

We must not split them or delegate parts of the permission set to multiple
tools at the same time.

--- Additional comment from Kay Sievers on 2014-10-01 11:57:22 EDT ---

(In reply to Nir Soffer from comment #17)
> (In reply to Federico Simoncelli from comment #15)

> > I briefly checked it and it seems to work, Nir can you verify it as well?
> > (With a real VM running possibly) Thanks.
> > At the moment I can't think of any harmful side-effect.
> 
> We need to understand first, why rules are using "add|change" - maybe it was
> a hack to avoid a some problem in older rhel?

It could be that at the time the "add" is generated, that the device is not
fully set up, and the rules do match at all, hence the "change" is needed.

But as mentioned above, we either manage the permissions by udev or by
libvirt and not by both at the same time. That means that the udev rule
should be deleted and libvirt should do the chown/chmod along with the
selinux label mangling, or libvirt should write out temporary rules that
allow udev to also apply the proper selinux label.

--- Additional comment from Kay Sievers on 2014-10-01 14:26:41 EDT ---

I've reverted the patch in the Fedora package now.

Please find another solution which does not split up permission maintenance between udev and other tools.

Only one of them can own the device node, the current udev logic does and
will not support any special logic or shared permission management.

--- Additional comment from Nir Soffer on 2014-10-02 02:58:57 EDT ---

(In reply to Kay Sievers from comment #20)
> I've reverted the patch in the Fedora package now.
What is "the patch"?

Do you mean that you reverted the fix provided by the builds in comment 11?
If true, then this bug is not MODIFIED any more.

--- Additional comment from Federico Simoncelli on 2014-10-02 04:23:17 EDT ---

(In reply to Kay Sievers from comment #19)
> (In reply to Nir Soffer from comment #17)
> > (In reply to Federico Simoncelli from comment #15)
> 
> > > I briefly checked it and it seems to work, Nir can you verify it as well?
> > > (With a real VM running possibly) Thanks.
> > > At the moment I can't think of any harmful side-effect.
> > 
> > We need to understand first, why rules are using "add|change" - maybe it was
> > a hack to avoid a some problem in older rhel?
> 
> It could be that at the time the "add" is generated, that the device is not
> fully set up, and the rules do match at all, hence the "change" is needed.
> 
> But as mentioned above, we either manage the permissions by udev or by
> libvirt and not by both at the same time.

Agreed, we'll try not to use the udev permissions if possible because there are some assumptions that are not clear.

For example by default the dm device has these permissions:

# ls -l /dev/dm-4 
brw-rw----. 1 root disk 253, 4 Oct  2 07:54 /dev/dm-4

but as soon as we add the rule:

ENV{DM_LV_NAME}=="?*", OWNER:="vdsm"

it becomes:

# ls -l /dev/dm-4 
brw-------. 1 vdsm root 253, 4 Oct  2 07:56 /dev/dm-4

Why? Who asked to change the group?

I am sure the same happens with OWNER and MODE since the apply logic is:

 apply = streq(udev_device_get_action(dev), "add") || event->owner_set || event->group_set || event->mode_set;

And on master even if you specify:

ENV{DM_LV_NAME}=="?*", SECLABEL{selinux}:="system_u:object_r:svirt_image_t:s0"

the label is not applied, unless you specify also one of OWNER|GROUP|MODE (for the same apply logic above).

Why? How should I guess that that's the case?


(In reply to Kay Sievers from comment #14)
> Udev is not supposed to be patched like that and we do not add such special
> handling.

Actually the same approach was already used for permissions, in fact the old:

 streq(udev_device_get_action(dev), "add")

came from:

commit 48a849ee17fb25e0001bfcc0f28a4aa633d016a1
Author: Kay Sievers <kay>
Date:   Fri Jan 4 16:15:46 2013 +0100

    udev: set device node permissions only at "add" events

because of bug 903716

It seems that the same logic was not considered for the security context.

If a rule specifies an OWNER why udev should change also the GROUP and the SECLABEL?

All these permissions/labels should be independent and not set to any magic default if just one of them is defined.

--- Additional comment from Nir Soffer on 2014-10-02 06:20:23 EDT ---

This looks like a reply of bug 903716.

Kay, you did not answer my question about documentation - is this the old, new and the change in behavior documented somewhere?

--- Additional comment from Kay Sievers on 2014-10-03 06:04:21 EDT ---

There is no explicit documentation. It is just the simple rule: udev
manages all or none of the permissions at "change" events.

Means, either udev manages the permissions or something else, never
multiple tools at the same time

--- Additional comment from Nir Soffer on 2014-10-06 18:17:11 EDT ---

(In reply to Federico Simoncelli from comment #22)
> (In reply to Kay Sievers from comment #19)
> > (In reply to Nir Soffer from comment #17)
> > > (In reply to Federico Simoncelli from comment #15)
>
> And on master even if you specify:
> 
> ENV{DM_LV_NAME}=="?*",
> SECLABEL{selinux}:="system_u:object_r:svirt_image_t:s0"
> 
> the label is not applied, unless you specify also one of OWNER|GROUP|MODE
> (for the same apply logic above).
> 
> Why? How should I guess that that's the case?

Looks like another bug, open another bug for this?

--- Additional comment from Nir Soffer on 2014-10-06 18:22:03 EDT ---

(In reply to Kay Sievers from comment #24)
> There is no explicit documentation. It is just the simple rule: udev
> manages all or none of the permissions at "change" events.
> 
> Means, either udev manages the permissions or something else, never
> multiple tools at the same time

Kay, just to make it clear - would this rule apply any user/group/mode/seclabel?

ACTION=="change", ENV{DM_LV_NAME}=="foo", RUN+="my-program"

According to your reply, this should not cause change in
the the device user, group, mode or selinux label.

--- Additional comment from Nir Soffer on 2014-10-06 18:52:51 EDT ---

Related to bug 1127460 and bug 1149883.

--- Additional comment from Peter Rajnoha on 2014-11-24 07:34:13 EST ---

(In reply to Kay Sievers from comment #19)
> (In reply to Nir Soffer from comment #17)
> > (In reply to Federico Simoncelli from comment #15)
> But as mentioned above, we either manage the permissions by udev or by
> libvirt and not by both at the same time.

I don't understand this. It's udev who manages /dev content, isn't it? And just like we can't create the nodes outside udev and just like we have OWNER/GROUP rules to override defaults for perms, we should have a rule for the selinux context to override defaults if needed.

So what's the problem here? Is it just about backporting the upstream patch? Or is there anything else?

--- Additional comment from Jan Synacek on 2015-01-06 03:59:54 EST ---

The solution was reverted, I'm putting this bugzilla back to assigned.

--- Additional comment from Kay Sievers on 2015-01-08 05:24:03 EST ---

(In reply to Nir Soffer from comment #26)
> Kay, just to make it clear - would this rule apply any
> user/group/mode/seclabel?
> 
> ACTION=="change", ENV{DM_LV_NAME}=="foo", RUN+="my-program"

"add" events will always reset everything. Any use of OWNER, GROUP, MODE will reset everything to the defaults or current values specified by rules.

RUN in a "change" event will not change anything if OWNER, GROUP, MODE are not applied by any rule.

--- Additional comment from Lennart Poettering on 2015-02-04 15:34:59 EST ---

REassigning to lvm, as this appears to be a bug in lvm's usage of udev, but not in udev.