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

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.

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.

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.

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

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

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

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.

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

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.

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.

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.

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

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.

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!

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

Thanks Daviey. confirmed, with quantal-proposed I get

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

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

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.