rules for images on attach-device not containing lock permission

Bug #1726804 reported by Christian Ehrhardt 
16
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Ubuntu Cloud Archive
Fix Released
Critical
Unassigned
Pike
Fix Released
Critical
Unassigned
Queens
Fix Released
Critical
Unassigned
libvirt (Ubuntu)
Fix Released
Critical
Unassigned
Artful
Fix Released
Critical
Unassigned

Bug Description

[Impact]

 * Qemu 2.10 started to lock image files to ensure no data corruption
   occurs. Unfurtunately that isn't covered by the apparmor rules we had
   for images so far - it need to add "k" permission.

 * This was spotted and done in Artful, but the tests for the hot-add of
   disks were hidden behind some other known not-too-bad issues. So by
   fixing those tests I realized that hot-add of disks is currently broken
   in Artful.

[Test Case]

# Get a very minimal Testguest that keeps running to attach something
$ qemu-img create /tmp/A.img 1M
cat <<EOF > testguest.xml
<domain type='kvm'>
        <name>testguest</name>
        <uuid>deadbeef-dead-beef-dead-beefdeadbeef</uuid>
        <memory unit='KiB'>1024</memory>
        <vcpu placement='static'>1</vcpu>
        <os>
                <type arch='x86_64' machine='pc-i440fx-zesty'>hvm</type>
                <boot dev='hd'/>
        </os>
        <features>
                <acpi/>
                <apic/>
                <pae/>
        </features>
        <devices>
                <emulator>/usr/bin/kvm-spice</emulator>
                <disk type='file' device='disk'>
                        <driver name='qemu'/>
                        <source file='/tmp/A.img'/>
                        <target dev='vda'/>
                </disk>
        </devices>
        <seclabel type='dynamic' model='apparmor' relabel='yes'/>
</domain>
EOF
$ virsh define testguest.xml
$ virsh start testguest

# Prepare Disk
$ qemu-img create /tmp/F.img 1M
$ cat <<EOF >diskF.xml
<disk type='file'>
  <driver name='qemu'/>
  <source file='/tmp/F.img'/>
  <target dev='sdc'/>
</disk>
EOF

# Then attach:
$ virsh attach-device testguest diskF.xml

* This should work, but fails without the fix as:
   error: internal error: unable to execute QEMU command 'device_add':
Property 'scsi-hd.drive' can't find value 'drive-scsi0-0-0-1'
  With a related apparmor denial:
   apparmor="DENIED" operation="file_lock" profile="libvirt-7d781722-69b7-8801-fe96-caf37b7a8969" name="/tmp/tmpKzZQR0/device_disk.img" pid=17582 comm="qemu" requested_mask="k" denied_mask="k" fsuid=0 ouid=0

* With the fix the file is rwk and works to be attached

[Regression Potential]

 * This is only adding apparmor lock permissions to files added after
   start. Thereby the only thing that comes to mind is if now things are
   locked that were not before, and thereby cause issues. But OTOH no one
   but qemu should lock the image files in use - and if someone else does
   he now correctly sees qemu holding the lock. Seems safe to me.

[Other Info]

 * This is an release/upgrade-regression which should be fixed
   asap. I already wrote and submitted a fix to upstream, but given that
   this can break a lot of use cases we ahve to fix fast and reroll in
   case upstream decides to modify.

---

On something like:
 $ virsh attach-device <guest> <xml>

The rule rendered is:
"/tmp/B.img" rw,

This is missing the k flag needed on qemu >=2.10.

This applies to block and file definitions:
<disk type='block'>
  <driver name='qemu'/>
  <source dev='/tmp/B.img'/>
  <target dev='sdb'/>
</disk>
<disk type='file'>
  <driver name='qemu'/>
  <source file='/tmp/F.img'/>
  <target dev='sdc'/>
</disk>

Both are rendered correctly as:
"/tmp/F.img" rwk,
If being part of the domain xml instead of being a hot-add.

tags: added: virt-aa-helper
Changed in libvirt (Ubuntu):
status: New → Confirmed
description: updated
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Tests - static:
sudo ../libvirt-upstream-git/src/virt-aa-helper --create --dryrun --uuid 'libvirt-f4239a92-f933-4bd3-b9fb-b9c260a7dc65' < test-virt-aa-helper.xml
=> but we know here all works fine

Test - hot-add:
This is more complex - actually it should be added to the overall XML and then passed to virt-aa-helper which would make no difference, but it does.
The call would be from libvirt itself actually with a merged XML IIRC so it should be no difference (other than context being initialized now but not later).

Well debugging will uncover what is going on...

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Prep: get sources with apt-source or pull-lp-source
Prep: install gdb
Prep: install libvirt dbgsym packages

# Get a very minimal Testguest that keeps running:
$ qemu-img create /tmp/A.img 1M
cat <<EOF > testguest.xml
<domain type='kvm'>
        <name>testguest</name>
        <uuid>deadbeef-dead-beef-dead-beefdeadbeef</uuid>
        <memory unit='KiB'>1024</memory>
        <vcpu placement='static'>1</vcpu>
        <os>
                <type arch='x86_64' machine='pc-i440fx-zesty'>hvm</type>
                <boot dev='hd'/>
        </os>
        <features>
                <acpi/>
                <apic/>
                <pae/>
        </features>
        <devices>
                <emulator>/usr/bin/kvm-spice</emulator>
                <disk type='file' device='disk'>
                        <driver name='qemu'/>
                        <source file='/tmp/A.img'/>
                        <target dev='vda'/>
                </disk>
        </devices>
        <seclabel type='dynamic' model='apparmor' relabel='yes'/>
</domain>
EOF
$ virsh start testguest

# Get
$ qemu-img create /tmp/B.img 1M
$ qemu-img create /tmp/F.img 1M
$ cat <<EOF >diskB.xml
<disk type='block'>
  <driver name='qemu'/>
  <source dev='/tmp/B.img'/>
  <target dev='sdb'/>
</disk>
EOF
$ cat <<EOF >diskF.xml
<disk type='file'>
  <driver name='qemu'/>
  <source file='/tmp/F.img'/>
  <target dev='sdc'/>
</disk>
EOF

Start a loop to attach (maybe there are better ways?)
$ PID=""; while [ -z "${PID}" ]; do PID=$(pgrep virt-aa-helper); done; sudo gdb -p ${PID}

Then attach soemthing:
$ virsh attach-device testguest diskF.xml

We don't care too much on the actual result, so just restart in gdb with "run" to debug, but add the same args that libvirt used (e.g. from proc).
That would be e.g.:
run -p 0 -r -u libvirt-deadbeef-dead-beef-dead-beefdeadbeef -f /tmp/F.img

Here already the alternate path becomes more clear.
It does not use the profile XML, but instead the -f flag.

And this misses the qemu >=2.10 compat to allow locking the files.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Test patch building for tests

Changed in libvirt (Ubuntu):
importance: Undecided → Critical
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

In the sense that before qemu 2.10 this worked this is an regression-release bug

tags: added: regression-release
description: updated
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Added the Cloud Archive as that certainy will be an issue in Pike as well - so keeping the Openstack team updated (I guess you pick latest libvirt anyway, but to be sure).

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Note to myself: tests in Bileto ticket 3008

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Prepped the SRU Template for Artful as it is released now.
Also passed (the now fully running, due to the fixes) regression tests - so ready for SRU review.

Note: Bionic Beaver is not yet around, so uploading to Artful with a normal version increment should still be the right thing to do - if there was a race with BB, please let me know so that I upload it there asap to fix it in Artful.

Revision history for this message
Robie Basak (racb) wrote :

SRU +1 for what is currently in the queue.

Revision history for this message
Andy Whitcroft (apw) wrote : Please test proposed package

Hello ChristianEhrhardt, or anyone else affected,

Accepted libvirt into artful-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libvirt/3.6.0-1ubuntu6 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 on 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-artful to verification-done-artful. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-artful. 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 libvirt (Ubuntu Artful):
status: Confirmed → Fix Committed
tags: added: verification-needed verification-needed-artful
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :
Download full text (7.8 KiB)

Verified in Proposed, here the full log:

root@artful-qatests:~# qemu-img create /var/lib/libvirt/A.img 1M
Formatting '/var/lib/libvirt/A.img', fmt=raw size=1048576
root@artful-qatests:~# cat <<EOF > testguest.xml
> <domain type='kvm'>
> <name>testguest</name>
> <uuid>deadbeef-dead-beef-dead-beefdeadbeef</uuid>
> <memory unit='KiB'>1024</memory>
> <vcpu placement='static'>1</vcpu>
> <os>
> <type arch='x86_64' machine='pc-i440fx-zesty'>hvm</type>
> <boot dev='hd'/>
> </os>
> <features>
> <acpi/>
> <apic/>
> <pae/>
> </features>
> <devices>
> <emulator>/usr/bin/kvm-spice</emulator>
> <disk type='file' device='disk'>
> <driver name='qemu'/>
> <source file='/var/lib/libvirt/A.img'/>
> <target dev='vda'/>
> </disk>
> </devices>
> <seclabel type='dynamic' model='apparmor' relabel='yes'/>
> </domain>
> EOF
root@artful-qatests:~# virsh define testguest.xml
Domain testguest defined from testguest.xml

root@artful-qatests:~# virsh start testguest
Domain testguest started

root@artful-qatests:~# qemu-img create /var/lib/libvirt/F.img 1M
Formatting '/var/lib/libvirt/F.img', fmt=raw size=1048576
root@artful-qatests:~# cat <<EOF >diskF.xml
> <disk type='file'>
> <driver name='qemu'/>
> <source file='/var/lib/libvirt/F.img'/>
> <target dev='sdc'/>
> </disk>
> EOF
root@artful-qatests:~# virsh attach-device testguest diskF.xml
error: Failed to attach device from diskF.xml
error: internal error: unable to execute QEMU command 'device_add': Property 'scsi-hd.drive' can't find value 'drive-scsi0-0-2'

root@artful-qatests:~# dmesg | tail -n 4
[152072.603398] audit: type=1400 audit(1508919165.522:639): apparmor="DENIED" operation="file_lock" profile="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" name="/var/lib/libvirt/F.img" pid=17985 comm="qemu-system-x86" requested_mask="k" denied_mask="k" fsuid=64055 ouid=64055
[152072.603400] audit: type=1400 audit(1508919165.523:640): apparmor="DENIED" operation="file_lock" profile="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" name="/var/lib/libvirt/F.img" pid=17985 comm="qemu-system-x86" requested_mask="k" denied_mask="k" fsuid=64055 ouid=64055
[152072.603402] audit: type=1400 audit(1508919165.523:641): apparmor="DENIED" operation="file_lock" profile="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" name="/var/lib/libvirt/F.img" pid=17985 comm="qemu-system-x86" requested_mask="k" denied_mask="k" fsuid=64055 ouid=64055
[152072.739782] audit: type=1400 audit(1508919165.659:642): apparmor="STATUS" operation="profile_replace" profile="unconfined" name="libvirt-deadbeef-dead-beef-dead-beefdeadbeef" pid=18063 comm="apparmor_parser"

# Now changing to the version in proposed

root@artful-qatests:~# # enable proposed
root@artful-qatests:~# vim /etc/apt/sources.list
root@artful-qatests:~# apt update
Hit:1 http://archive.ubuntu.com/ubuntu artful InRelease
Ign:2 http://ddebs.ubuntu.com artful InRelease
Get:3 http://arc...

Read more...

tags: added: verification-done verification-done-artful
removed: verification-needed verification-needed-artful
Changed in cloud-archive:
status: New → Triaged
importance: Undecided → Critical
Revision history for this message
Martin Pitt (pitti) wrote :

This also gets detected by the cockpit integration tests in https://github.com/cockpit-project/cockpit/pull/7939, so I spent an hour to create a minimal reproducer (attached), only to find that this is already fixed in -proposed.

I confirm that with the -proposed package this works again. Thanks!

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

No need to reroll the patch BTW got accepted as-is in https://www.redhat.com/archives/libvir-list/2017-October/msg01191.html

Revision history for this message
Corey Bryant (corey.bryant) wrote :

Hello ChristianEhrhardt, or anyone else affected,

Accepted libvirt into pike-proposed. The package will build now and be available in the Ubuntu Cloud Archive in a few hours, and then in the -proposed repository.

Please help us by testing this new package. To enable the -proposed repository:

  sudo add-apt-repository cloud-archive:pike-proposed
  sudo apt-get update

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-pike-needed to verification-pike-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-pike-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!

tags: added: verification-pike-needed
Revision history for this message
Corey Bryant (corey.bryant) wrote :

Hello ChristianEhrhardt, or anyone else affected,

Accepted libvirt into queens-proposed. The package will build now and be available in the Ubuntu Cloud Archive in a few hours, and then in the -proposed repository.

Please help us by testing this new package. To enable the -proposed repository:

  sudo add-apt-repository cloud-archive:queens-proposed
  sudo apt-get update

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-queens-needed to verification-queens-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-queens-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!

tags: added: verification-queens-needed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libvirt - 3.6.0-1ubuntu6

---------------
libvirt (3.6.0-1ubuntu6) artful; urgency=medium

  * d/p/ubuntu-aa/0037-virt-aa-helper...: grant locking permission on append
    files (LP: #1726804)
  * d/p/ubuntu-aa/0038-virt-aa-helper-fix-paths-for-usb-hostdevs.patch:
    fix path generation for USB host devices (LP: #1552241)
  * d/p/ubuntu-aa/0039-virt-aa-helper-fix-libusb-access-to-udev-usb-data.patch:
    generate valid rules on usb passthrough (LP: #1686324)

 -- Christian Ehrhardt <email address hidden> Tue, 24 Oct 2017 14:30:34 +0200

Changed in libvirt (Ubuntu Artful):
status: Fix Committed → Fix Released
Revision history for this message
Chris Halse Rogers (raof) wrote : Update Released

The verification of the Stable Release Update for libvirt 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 regressions.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libvirt - 3.6.0-1ubuntu6

---------------
libvirt (3.6.0-1ubuntu6) artful; urgency=medium

  * d/p/ubuntu-aa/0037-virt-aa-helper...: grant locking permission on append
    files (LP: #1726804)
  * d/p/ubuntu-aa/0038-virt-aa-helper-fix-paths-for-usb-hostdevs.patch:
    fix path generation for USB host devices (LP: #1552241)
  * d/p/ubuntu-aa/0039-virt-aa-helper-fix-libusb-access-to-udev-usb-data.patch:
    generate valid rules on usb passthrough (LP: #1686324)

 -- Christian Ehrhardt <email address hidden> Tue, 24 Oct 2017 14:30:34 +0200

Changed in libvirt (Ubuntu):
status: Fix Committed → Fix Released
tags: added: qemu-file-locking
Revision history for this message
Corey Bryant (corey.bryant) wrote :

OpenStack regression testing passed successfully on pike-proposed:

xenial-pike-proposed with stable charms:

Ran: 102 tests in 1588.4030 sec.
 - Passed: 93
 - Skipped: 9
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 0
Sum of execute time for each test: 687.2787 sec.

xenial-pike-proposed with dev charms:

Ran: 102 tests in 1403.7715 sec.
 - Passed: 93
 - Skipped: 9
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 0
Sum of execute time for each test: 649.5511 sec.

tags: added: verification-pike-done
removed: verification-pike-needed
Revision history for this message
Corey Bryant (corey.bryant) wrote :

The verification of the Stable Release Update for libvirt has completed successfully and the package has now been released to -updates. 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 regressions.

Revision history for this message
Corey Bryant (corey.bryant) wrote :

This bug was fixed in the package libvirt - 3.6.0-1ubuntu6~cloud0
---------------

 libvirt (3.6.0-1ubuntu6~cloud0) xenial-pike; urgency=medium
 .
   * New update for the Ubuntu Cloud Archive.
 .
 libvirt (3.6.0-1ubuntu6) artful; urgency=medium
 .
   * d/p/ubuntu-aa/0037-virt-aa-helper...: grant locking permission on append
     files (LP: #1726804)
   * d/p/ubuntu-aa/0038-virt-aa-helper-fix-paths-for-usb-hostdevs.patch:
     fix path generation for USB host devices (LP: #1552241)
   * d/p/ubuntu-aa/0039-virt-aa-helper-fix-libusb-access-to-udev-usb-data.patch:
     generate valid rules on usb passthrough (LP: #1686324)

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

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.