70-udev-acl.rules needs to put g+rw on /dev/kvm

Bug #1103022 reported by Serge Hallyn on 2013-01-22
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
qemu (Ubuntu)
High
Serge Hallyn
qemu-kvm (Ubuntu)
Undecided
Unassigned
Quantal
High
Unassigned

Bug Description

When qemu-system gets installed, the newly installed udev rule causes /dev/kvm to gets chgrpd to kvm and its mode to get set to g+rw. However, because /dev/kvm was tagged with ACL previously, there is a group:: acl on /dev/kvm which does not get removed. Therefore /dev/kvm is g+rw in the file mode, but the acl denies group read/write access. After a reboot all is fine.

I have not seen a clean way to have udev remove that acl, and there is no reason for it. So please update the 70-udev-acl.rules file to set MODE=0660 on /dev/kvm

================
SRU Justification
1. Impact: when qemu-kvm is first installed, /dev/kvm is not owned by group kvm (until subsequent reboot). This prevents libvirt from using kvm until a reboot.
2. Development fix: add group kvm during preinst. (this was done in precise, but accidentally dropped in a merge from debian in quantal)
3. Stable fix: same as development fix
4. Test case: Create a new quantal vm. sudo apt-get install qemu-kvm. ls -l /dev/kvm, check that it is owned by group kvm. Note if you've install qemu-kvm previously, purging it is not enough before retesting - you must also remove the kvm group entry.
5. Regression potential: none

Related branches

Changed in udev (Ubuntu):
importance: Undecided → High
status: New → Confirmed
Serge Hallyn (serge-hallyn) wrote :
tags: added: patch
Martin Pitt (pitti) wrote :

As discussed on IRC: As this is an one-time upgrade fix for cleaning up after the currently wrong /lib/udev/rules.d/40-qemu-system.rules, I'd much rather like to see this as an one-time action in postinst (which is the standard way of doing such operations on upgrade) than this patch, which would confuse matters even more and also potentially breaks custom rules which change the permissions of /dev/kvm.

Serge Hallyn (serge-hallyn) wrote :

I'm assuming (hoping) comment #2 as entered before we proceeded with our irc conversation?

The 40-qemu-system.rules is not the problem and we're not trying to recover from it.
As mentioned in irc, the steps are:

 1. set up a new ubuntu system, it modprobes kvm_intel, /dev/kvm gets created,
 2. 70-udev-acl.rules sets /dev/kvm to root:root rwx------, and tags it with acl
 3. user logs in, something (consolekit?) adds a group::--- acl
 4. admin logs in remotely, installs qemu-system and libvirt-bin, which triggers udev with new rules,
 5. udev chowns /dev/kvm to root:kvm, and sets it to rwxrw----, but the group::--- acl remains
 6. libvirt tries to start a vm as group kvm, but the group:--- acl refuses it

The patch I proposed here simply sets GROUP=0660 in the 70-udev-acl.rules. That way the group:: acl still gets added on login, but becomes group::rw-. So it's a workaround for whatever is adding that acl in the first place.

As discussed on irc, I'll see if I can figure out what exactly is causing that
group acl to be (needlessly) written.

Serge Hallyn (serge-hallyn) wrote :

In fact udev-acl appears to be doing it.

When logging into a simple ubuntu desktop with qemu-system not installed, I log in and find /dev/kvm is root:root rwm------ with a group::--- acl installed. I verify the acl by doing 'chmod g+rw /dev/kvm' followed by ls -l /dev/kvm and getfacl /dev/kvm showng that /dev/kvm is now rwxrw---- but the group::--- acl is still there.

Next I moved /usr/lib/ConsoleKit/run-seat.d/udev-acl.ck to /root/ and rebooted, and ssh'd in. Now /dev/kvm had no acls and was

serge@ubuntu:~$ ls -l /dev/kvm
crw-rw---- 1 root root 10, 232 Jan 29 01:04 /dev/kvm
serge@ubuntu:~$ getfacl /dev/kvm
getfacl: Removing leading '/' from absolute path names
# file: dev/kvm
# owner: root
# group: root
user::rw-
group::rw-
other::---

(I next undid this by doing 'chmod g-rw /dev/kvm')

Next I manually ran:

/lib/udev/udev-acl -a change --device=/dev/kvm

after this, I got:

root@ubuntu:~# getfacl /dev/kvm
getfacl: Removing leading '/' from absolute path names
# file: dev/kvm
# owner: root
# group: root
user::rw-
user:lightdm:rw-
group::---
mask::rw-
other::---

root@ubuntu:~# chmod g+rw /dev/kvm
root@ubuntu:~# getfacl /dev/kvm
getfacl: Removing leading '/' from absolute path names
# file: dev/kvm
# owner: root
# group: root
user::rw-
user:lightdm:rw-
group::---
mask::rw-
other::---

showing that running udev-acl.ck is what is causing the group acl to be
created, even though it's not obvious, looking at the udev-acl.c code,
how it would do so.

Download full text (4.3 KiB)

Serge Hallyn [2013-01-29 6:22 -0000]:
> I'm assuming (hoping) comment #2 as entered before we proceeded with our
> irc conversation?

Yes, I did after the first part when it seemed to me that we agreed
that this was only an upgrade fix for the wrong
40-qemu-system.rules.

> The 40-qemu-system.rules is not the problem and we're not trying to recover from it.

Well, it is a problem in the sense that the setfacl call there should
not be necessary and also should be redundant, as it is essentially
equivalent to "chmod g+rw"?

> 1. set up a new ubuntu system, it modprobes kvm_intel, /dev/kvm gets created,
> 2. 70-udev-acl.rules sets /dev/kvm to root:root rwx------, and tags it with acl

No, 70-udev-acl.rules does not change the ownership or permissions of
/dev/kvm, the kernel default is 600 root:root. 70-udev-acl.rules
merely tags it with UDEV_ACL, which causes udev-acl.ck to add
additional user ACLs for the user currently owning the foreground
console.

> 3. user logs in, something (consolekit?) adds a group::--- acl

That's the bit which I don't understand. I seriously doubt that this
is udev-acl, as that only adds additional user ACLs. What makes you
sure that this is actually an ACL, as opposed to just being the
ACL representation of the original "600" permissions that /dev/kvm
gets without 40-qemu-system.rules?

 - The udev-acl.ck code does not look like it would be able to do it
 - As far as I know, ACLs are additive to Unix file permissions, i. e.
   they can never remove access.
 - I don't see any call to *facl in any installed postinst script.

> 4. admin logs in remotely, installs qemu-system and libvirt-bin, which triggers udev with new rules,
> 5. udev chowns /dev/kvm to root:kvm, and sets it to rwxrw----, but the group::--- acl remains
> [...]
> As discussed on irc, I'll see if I can figure out what exactly is causing that
> group acl to be (needlessly) written.

Right, that seems to be the crux of the problem.

I tried to reproduce this:

  $ sudo mv /lib/udev/rules.d/40-qemu-system.rules{,.disabled}
  $ sudo rmmod kvm_intel kvm
  $ sudo passwd root
  # log into VT1 as root, to avoid having a CK session
  # modprobe kvm_intel
  # ls -l /dev/kvm; getfacl /dev/kvm
  crw------- 1 root root 10, 232 Jan 29 08:36 /dev/kvm
  # file: dev/kvm
  # owner: root
  # group: root
  user::rw-
  group::---
  other::---

So far so good. We should be after step 2 now. Now I switch back to my
desktop session, so that 70-udev-acl.rules kicks in:

  $ ls -l /dev/kvm; getfacl /dev/kvm
  crw-rw----+ 1 root root 10, 232 Jan 29 08:36 /dev/kvm
  getfacl: Entferne führende '/' von absoluten Pfadnamen
  # file: dev/kvm
  # owner: root
  # group: root
  user::rw-
  user:martin:rw-
  group::---
  mask::rw-
  other::---

We see that udev added the dynamic ACL. That's step 3. Now let's "install" the new rules:

  $ sudo mv /lib/udev/rules.d/40-qemu-system.rules{.disabled,}
  $ udevadm trigger --subsystem-match=misc --action=change --verbose
  [...]
  /sys/devices/virtual/misc/kvm

  $ ls -l /dev/kvm; getfacl /dev/kvm
  crw-rw----+ 1 root root 10, 232 Jan 29 08:36 /dev/kvm
  # file: dev/kvm
  # owner: root
  # group: root
  user::rw-
  user:martin:rw-
  group...

Read more...

Serge Hallyn (serge-hallyn) wrote :
Download full text (3.5 KiB)

> > The 40-qemu-system.rules is not the problem and we're not trying to recover from it.
>
> Well, it is a problem in the sense that the setfacl call there should
> not be necessary and also should be redundant, as it is essentially
> equivalent to "chmod g+rw"?

I'd love to remove it (I only very recently added it), but it is not
redundant with chmod g+rw, as chmod does not remove acls.

> > 1. set up a new ubuntu system, it modprobes kvm_intel, /dev/kvm gets created,
> > 2. 70-udev-acl.rules sets /dev/kvm to root:root rwx------, and tags it with acl
>
> No, 70-udev-acl.rules does not change the ownership or permissions of

Ok, fine, I'll change that to "leaves it root:root rwx------ when it
could otherwise change it to root:root rwxrw----".

> > 3. user logs in, something (consolekit?) adds a group::--- acl
>
> That's the bit which I don't understand. I seriously doubt that this
> is udev-acl, as that only adds additional user ACLs. What makes you
> sure that this is actually an ACL, as opposed to just being the
> ACL representation of the original "600" permissions that /dev/kvm
> gets without 40-qemu-system.rules?
>
> - The udev-acl.ck code does not look like it would be able to do it
> - As far as I know, ACLs are additive to Unix file permissions, i. e.
> they can never remove access.
> - I don't see any call to *facl in any installed postinst script.
>
> > 4. admin logs in remotely, installs qemu-system and libvirt-bin, which triggers udev with new rules,
> > 5. udev chowns /dev/kvm to root:kvm, and sets it to rwxrw----, but the group::--- acl remains
> > [...]
> > As discussed on irc, I'll see if I can figure out what exactly is causing that
> > group acl to be (needlessly) written.
>
> Right, that seems to be the crux of the problem.
>
> I tried to reproduce this:
>
> $ sudo mv /lib/udev/rules.d/40-qemu-system.rules{,.disabled}
> $ sudo rmmod kvm_intel kvm
> $ sudo passwd root
> # log into VT1 as root, to avoid having a CK session
> # modprobe kvm_intel
> # ls -l /dev/kvm; getfacl /dev/kvm
> crw------- 1 root root 10, 232 Jan 29 08:36 /dev/kvm
> # file: dev/kvm
> # owner: root
> # group: root
> user::rw-
> group::---
> other::---
>
> So far so good. We should be after step 2 now. Now I switch back to my
> desktop session, so that 70-udev-acl.rules kicks in:
>
> $ ls -l /dev/kvm; getfacl /dev/kvm
> crw-rw----+ 1 root root 10, 232 Jan 29 08:36 /dev/kvm
> getfacl: Entferne führende '/' von absoluten Pfadnamen
> # file: dev/kvm
> # owner: root
> # group: root
> user::rw-
> user:martin:rw-
> group::---
> mask::rw-
> other::---
>
> We see that udev added the dynamic ACL. That's step 3. Now let's
> "install" the new rules:
>
> $ sudo mv /lib/udev/rules.d/40-qemu-system.rules{.disabled,}

[...]

> $ udevadm trigger --subsystem-match=misc --action=change --verbose
> [...]
> /sys/devices/virtual/misc/kvm
>
> $ ls -l /dev/kvm; getfacl /dev/kvm
> crw-rw----+ 1 root root 10, 232 Jan 29 08:36 /dev/kvm
> # file: dev/kvm
> # owner: root
> # group: root
> user::rw-
> user:martin:rw-
> group::---
> mask::rw-
> other::---
>
> And that's indeed where th...

Read more...

Martin Pitt (pitti) wrote :

> This is where you've gone wrong in your experiment. The /dev/kvm perms will be correct because you've reloaded kvm_intel
after a (any) 40-qemu-system.rule existed.

Ah, thanks for pointing out!

I just noticed another problem which we might have overlooked here:

$ sudo rmmod kvm_intel kvm
$ sudo mv /lib/udev/rules.d/40-qemu-system.rules{,.disabled}
$ sudo mv /lib/udev/udev-acl{,.disabled}
$ sudo modprobe kvm_intel
$ sudo cp /lib/udev/rules.d/40-qemu-system.rules{.disabled,}
$ udevadm trigger --sysname-match=kvm --verbose
/sys/devices/virtual/misc/kvm
$ ls -l /dev/kvm
crw------- 1 root root 10, 232 Jan 30 07:21 /dev/kvm

i. e. triggering a change (or add) event does not actually apply the GROUP and MODE settings to /dev/kvm, it remains as root. This is without any udev-acl magic, and happens both with the current package rules as well as the simpler rules I tried above (without the := and RUN).

So that part of the postinst doesn't currently work as expected.

When I work around this by setting the permissions manually (as the postinst intends to do)

   sudo chgrp kvm /dev/kvm
   sudo chmod g+rw /dev/kvm

then running

  sudo /lib/udev/udev-acl.disabled -a change --device=/dev/kvm

seems to have the desired effect:

$ getfacl /dev/kvm
# file: dev/kvm
# owner: root
# group: kvm
user::rw-
user:martin:rw-
group::rw-
mask::rw-
other::---

So may it be that this was a red herring, caused by the unexpected/wrong group and permissions of /dev/kvm after triggering the rule?

Serge Hallyn (serge-hallyn) wrote :

Can you please try that a few more times? If I script that, I find that
I get your results if I don't put a sleep before the udevadm trigger,
but the right results if I do. I.e.:

cat > kvmudevtest << EOF
mv /lib/udev/rules.d/40-qemu-system.rules{,.disabled}
mv /lib/udev/udev-acl{,.disabled}
rmmod kvm_intel
modprobe kvm_intel
cp /lib/udev/rules.d/40-qemu-system.rules{.disabled,}
sleep 2
udevadm trigger --sysname-match=kvm --verbose
ls -l /dev/kvm
EOF

bash kvmudevtest

with the sleep, it works. Without the sleep, /dev/kvm does not get
chowned. (obviously just a race with inotify and udev's handling
thereof)

So disabling udev-acl, on my raring test system, gives just the
right result.

Serge Hallyn (serge-hallyn) wrote :

This test case shows the bug in udev-acl.c.

When udev-acl is called to add a user acl, it

1. gets the current acl
2. removes any acl for non-current user
3. adds acl for user
4. writes the result.

Any existing group acl is kept.

What this test case shows, is that in step 1 udev will get what looks like a group acl (for zero perms) - even though that was not an acl, it was just the group perms on the inode. Then in step 4, that acl gets written as an explicit acl.

To test, compile the program, touch a file, look at the perms, run this program specifying the file as argument, and re-check the perms:

1. gcc -o acltest acltest.c -lacl
2. echo ab > ab
3. chmod 700 ab
3. ls -l ab; getfacl ab
4. ./acltest ab
5. ls -l ab; getfacl ab

An acl has been added to ab (indicated by '+' in ls output), which was not there before

Serge Hallyn (serge-hallyn) wrote :

So really, the above is what triggers the bug, but I think the real bug is that when udev-acl later gets called to update the permissions, it simply keeps the existing group acl instead of updating it for the new MODE=0660 in the new udev rule.

Martin Pitt (pitti) wrote :

Just for the record, udev-acl will hopefully go away RSN (either in raring, or immediately after), see https://blueprints.launchpad.net/ubuntu/+spec/foundations-1303-consolekit-logind-migration and bug 1153224.

Serge Hallyn (serge-hallyn) wrote :

The bug really is in udev-acl. However, as udev-acl will soon go away, and I can work around it in qemu, I am marking it against qemu and implementing the workaround until we switch to logind.

Changed in udev (Ubuntu):
status: Confirmed → In Progress
assignee: nobody → Serge Hallyn (serge-hallyn)
affects: udev (Ubuntu) → qemu (Ubuntu)
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package qemu - 1.4.0+dfsg-1expubuntu4

---------------
qemu (1.4.0+dfsg-1expubuntu4) raring; urgency=low

  * re-add qemu-system-x86.modprobe to set nesting=1 (LP: #1155177)
  * qemu-system-x86.qemu-kvm.upstart:
    - remove NESTED workarounds from upstart file.
    - remove loading of modules which is now always done
    - remove TAPR define which is no longer used
  * move customizable defines back to qemu-kvm.default
  * copy creation of group kvm to preinst - the group must exist when the
    kvm udev rule is installed (LP: #1103022) (LP: #1092715)
  * add adduser to qemu-system-common Pre-Depends for use by preinst.
 -- Serge Hallyn <email address hidden> Thu, 14 Mar 2013 14:21:53 -0500

Changed in qemu (Ubuntu):
status: In Progress → Fix Released
Serge Hallyn (serge-hallyn) wrote :

qemu-kvm in quantal was replaced by qemu in raring. Hence this bug is invalid in raring qemu-kvm, fix released in raring qemu, and exists in quantal qemu-kvm.

Changed in qemu-kvm (Ubuntu):
status: New → Invalid
Changed in qemu-kvm (Ubuntu Quantal):
assignee: nobody → Serge Hallyn (serge-hallyn)
importance: Undecided → High
status: New → In Progress
no longer affects: qemu (Ubuntu Quantal)
description: updated
tags: removed: patch
Changed in qemu-kvm (Ubuntu Quantal):
assignee: Serge Hallyn (serge-hallyn) → nobody

Hello Serge, or anyone else affected,

Accepted qemu-kvm into quantal-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/qemu-kvm/1.2.0+noroms-0ubuntu2.12.10.4 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in qemu-kvm (Ubuntu Quantal):
status: In Progress → Fix Committed
tags: added: verification-needed
Martin Pitt (pitti) wrote :

Just a note, udev-acl is gone in saucy. It was replaced with "uaccess", which is similar in spirit, but a different implementation.

Serge Hallyn (serge-hallyn) wrote :

Thanks Daviey. confirmed, with quantal-proposed I get

crw-rw---- 1 root kvm 10, 232 May 17 15:43 /dev/kvm

description: updated
tags: added: verification-done
removed: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package qemu-kvm - 1.2.0+noroms-0ubuntu2.12.10.4

---------------
qemu-kvm (1.2.0+noroms-0ubuntu2.12.10.4) quantal-proposed; urgency=low

  * qemu-kvm.preinst: add kvm group if not present. (LP: #1103022)
 -- Serge Hallyn <email address hidden> Fri, 22 Mar 2013 10:14:18 -0500

Changed in qemu-kvm (Ubuntu Quantal):
status: Fix Committed → Fix Released

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments