Bug 1147910

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
 ...

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 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);
         }

Created attachment 942743 [details]
0001-udev-set-default-selinux-label-only-at-add-events.patch-fedora

Created attachment 942744 [details]
0001-udev-set-default-selinux-label-only-at-add-events.patch-master

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

(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.

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; 

#

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

(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).

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

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

(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.

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.

(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.

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?

(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?

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.

(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.

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.

(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.

(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.

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?

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

(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?

(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.

Related to bug 1127460 and bug 1149883.

(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?

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

(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.

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

(In reply to Kay Sievers from comment #30)
> RUN in a "change" event will not change anything if OWNER, GROUP, MODE are
> not applied by any rule.

The problem here is about resetting the selinux context for nodes/symlinks on CHANGE event which should be preserved by udev - LVM does not touch any of the nodes/symlinks in dev at all these days. Or am I missing something? What's LVM supposed to do here in this case?

Well what originally set this in the specific case we are discussing?  

system_u:object_r:svirt_image_t:s0

Was it a udev rule or something external?

(In reply to Alasdair Kergon from comment #33)
> Well what originally set this in the specific case we are discussing?  
> 
> system_u:object_r:svirt_image_t:s0
> 
> Was it a udev rule or something external?

The selinux context is set by libvirt when you start a vm using a block-based disk.

Well I think that's your answer: from the comments above, it seems the udev maintainers want udev to control the selinux context as well as the user/group/mode, and so libvirt should not be changing this outside udev.

The problem with this approach in the general case is that it means udev has to work out which devices belong to libvirt and which don't when the devices concerned are probably indistinguishable from other devices.
In the case of vdsm though, perhaps there is something (name format?) satisfactory to match upon.

12-dm-permissions.rules  needs examples with selinux adding

(In reply to Kay Sievers from comment #30)
> (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.

Simply, OWNER,GROUP,MODE and SECLABEL{selinux} must be all set together when DM device is active and there are events incoming, that means:

  - CHANGE event that is activating the device (just after the dm table load+resume)
  - synthesized ADD event for active device
  - any CHANGE event for active device

The seclabel is not applied when there's no OWNER,GROUP,MODE set at the same time when the event is processed (I think this is the part that confused us all).

So the fix is simple? Just set OWNER/GROUP/MODE/SECLABEL{selinux} all the time? And yes, for that you need to hook onto something - DM_NAME, DM_UUID, DM_LV_NAME,DM_VG_NAME or whatever else you can call in the rules to identify the devices that need to be marked that way. Never set owner/group/mode/seclabel out of udev with any external tool.

(...also, is SECLABEL{selinux} available in F20? I think it's quite recent addition - it was not recognized before, but I don't know exactly in which systemd/udev version it appeared.)

(In reply to Peter Rajnoha from comment #36)
>   - CHANGE event that is activating the device (just after the dm table
> load+resume)
>   - synthesized ADD event for active device
>   - any CHANGE event for active device
> 

N.B.: there's no need to distringuish these when you're using 12-dm-permissions.rules since if the device is inactive/not usable yet, all the DM_NAME,DM_UUID... variables are not set.

I thought the only problem here is that the SECLABEL{selinux} is not yet available as it's quite new (my comment #28 which got unanswered).

(In reply to Peter Rajnoha from comment #37)
> I thought the only problem here is that the SECLABEL{selinux} is not yet
> available as it's quite new (my comment #28 which got unanswered).

Not only SECLABEL{selinux} is not available, the selinux label is not 
a static label but a dynamic unique label for each vm (this is part of
svirt vm isolation). So the only way to apply the label though udev is
by a temporary udev rule.

When SECLABEL is available, either vdsm or libvirt can use temporary udev
rules. In vdsm, this is a large change effecting many flows. I don't know
about libvirt side.

Eric, can libvirt use temporary udev rules for applying svirt selinux
labels?

(In reply to Nir Soffer from comment #38)
> (In reply to Peter Rajnoha from comment #37)
> > I thought the only problem here is that the SECLABEL{selinux} is not yet
> > available as it's quite new (my comment #28 which got unanswered).
> 
> Not only SECLABEL{selinux} is not available, the selinux label is not 
> a static label but a dynamic unique label for each vm (this is part of
> svirt vm isolation). So the only way to apply the label though udev is
> by a temporary udev rule.
> 
> When SECLABEL is available, either vdsm or libvirt can use temporary udev
> rules. In vdsm, this is a large change effecting many flows. I don't know
> about libvirt side.
> 
> Eric, can libvirt use temporary udev rules for applying svirt selinux
> labels?

Dan, what do you think?  Right now, libvirt is setting labels on block devices by directly calling into libselinux; what would it look like to instead generate a udev rule to write the label?

I certainly would not want to rely on invoking udev in order to set the labels initially. Working with udev is a real pain because of its asynchronous nature where by you have to write the rule, then trigger udev to process rules in the background and then wait some indeterminate amount of time for udev to finish. If it is sufficient for libvirt to write the rule, and then set label explicitly itself that could work. eg libvirt still does what it does today, but we just install a rule to stop udev from undoing our work.

(In reply to Daniel Berrange from comment #40)
> If it is sufficient for libvirt to write the
> rule, and then set label explicitly itself that could work. eg libvirt still
> does what it does today, but we just install a rule to stop udev from
> undoing our work.

That should work fine I think.  In principle, udev may be triggered at any time and jump in and reset things to match its view of the world.

(Or we might try to come up with a mechanism for lvm to pass the required selinux context through to udev and then specify it with lvcreate/lvchange.)

(In reply to Daniel Berrange from comment #40)
> I certainly would not want to rely on invoking udev in order to set the
> labels initially. Working with udev is a real pain because of its
> asynchronous nature where by you have to write the rule, then trigger udev
> to process rules in the background and then wait some indeterminate amount
> of time for udev to finish. If it is sufficient for libvirt to write the
> rule, and then set label explicitly itself that could work. eg libvirt still
> does what it does today, but we just install a rule to stop udev from
> undoing our work.

This will be racy:
1. Libvirt set selinux label
2. Libvirt write udev rule
3. Udev event triggered before udev detect the new rule, using old rules
   and removing svirt selinux lable
4. Libvirt starts vm, vm fail to access storage
5. Udev detect new rule and apply correct label

(In reply to Alasdair Kergon from comment #41)
> (Or we might try to come up with a mechanism for lvm to pass the required
> selinux context through to udev and then specify it with lvcreate/lvchange.)

This will not solve libvirt problem, since it does not create the lvs
when starting vms for vdsm. vdsm creates these lvs, but since libvirt
set the selinux label, vdsm know the label only after the vm was
started.

According to libvirt docs, vdsm can create the lv selinux context,
and pass the context to libvirt, but this require changes touching
many vdsm flows, and of course will not help other libvirt users.

(In reply to Nir Soffer from comment #42)
> This will be racy:
> 1. Libvirt set selinux label
> 2. Libvirt write udev rule
> 3. Udev event triggered before udev detect the new rule, using old rules
>    and removing svirt selinux lable
> 4. Libvirt starts vm, vm fail to access storage
> 5. Udev detect new rule and apply correct label

That's why my suggestion was that libvirt would write the udev rule & tell udev to reload, *before* setting the selinux label. ie your steps 1 & 2 are the wrong way around.

(In reply to Daniel Berrange from comment #44)
> (In reply to Nir Soffer from comment #42)
> > This will be racy:
> > 1. Libvirt set selinux label
> > 2. Libvirt write udev rule
> > 3. Udev event triggered before udev detect the new rule, using old rules
> >    and removing svirt selinux lable
> > 4. Libvirt starts vm, vm fail to access storage
> > 5. Udev detect new rule and apply correct label
> 
> That's why my suggestion was that libvirt would write the udev rule & tell
> udev to reload, *before* setting the selinux label. ie your steps 1 & 2 are
> the wrong way around.

Right, my example was bad. But we may have this race:

1. libvirt write new rule
2. libvirt tells udev to reload rules (without waiting for completion)
3. libvirt set selinux label
4. udev handle event on the device using old cached rule, removing
   the selinux label set by libvirt.
5. udev reload new rule

Kay, is this possible?

This message is a reminder that Fedora 20 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 20. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '20'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 20 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Fedora 20 changed to end-of-life (EOL) status on 2015-06-23. Fedora 20 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.

Moving this to libvirt to make use of the new SECLABEL=value variable (see man ude) that can be set in udev rules and then interpreted by udev to set the exact selinux label needed.

If the "value" is configure outside, it needs to be imported to udev with PROGRAM=... rule first.

(In reply to Peter Rajnoha from comment #48)
> If the "value" is configure outside, it needs to be imported to udev with
> PROGRAM=... rule first.

I meant IMPORT{program}=...

Clearing stale needinfo

Switching libvirt to install udev rules is significant upstream work so it doesn't really make sense for the fedora tracker. Moving upstream.

But if this is affecting VDSM in RHEL I'd suggest cloning this bug there so it's properly prioritized.

The resolution that was implemented upstream is to run qemu in a namespace with separate /dev mount managed by libvirtd. Although the initial feature was implemented in 3.1.0 it was very buggy and it wasn't really usable until 3.2.0 release.