allow /dev/vfio/vfio in general

[Christian Ehrhardt ]

[Impact]

 * using PCI hotplug based on vfio fails if the guest was not started with
   at least one hostdev already passed through.

 * After Artful we dropped an apparmor rule due to an upstream discussion
   and identifying it as not needed. Unfortunately that was a mistake and
   we now identified the use case. With that I brought the change upstream
   and now backport it to the affected releases.

[Test Case]

 * Get a KVM guest e.g. via uvtool-libvirt

 * Select a pci device to hotplug (see lspci)
   Create a XML file based on this like for me the device on 0xdb in my
   example
     <hostdev mode='subsystem' type='pci' managed='yes'>
       <source>
          <address domain='0x0000' bus='0xdb' slot='0x0' function='0x0'/>
       </source>
     </hostdev>

 * attach the device described in the file to your guest
    $ virsh attach-device <guestname> <filename>

 * Without the fix this will fail due to an apparmor block to
   /dev/vfio/vfio. With the fix it will work (be aware that many systems
   have crappy iommu's still, since I don't know the testers system lets
   say it won't fail due to this - I already tested on systems known to be
   good and it worked as expected).

 * can be tested from PPA build at https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3292

[Regression Potential]

 * This is opening up one more path to the guest, there is no regression
   that will block the guest from running.
 * If anything I could think of people being concerned this might regress
   isolation, but please realize we do not open up a device /dev/vfio/[0-
   9]* instead just the container management node. This is rather safe per
   kernel doc and does not allow to access any device (it only allows to
   request a new vfio group by opening it).

[Other Info]

 * n/a

---

VFIO is currently leaving an edge case that can kill PCI Hotplug.

There are these things in place:
1. if a guest spec has a hostdev it will add
     /dev/vfio/vfio
     /dev/vfio/[0-9]*
   This works fine
2. If a device is hotplugged the custom vfio addr is resolved and added
   to the per-guest profile as per-device entry like
     "/dev/vfio/162" rwk
   This works fine and is even better since at this time it can deternine just the device it needs

But #2 needs /dev/vfio/vfio access before knowing any of that.
That leads to the case that if one has one hostdev, he can plug more and be fine.
But without any hostdev in the initial description the guest will not be allowed to access /dev/vfio/vfio which is needs to determine the indifidual entry.

Due to that it completely fails and it can't be hotplugged.

Per https://www.kernel.org/doc/Documentation/vfio.txt the base /dev/vfio/vfio is safe and is not an attach vector. So we should allow /dev/vfio/vfio in general.

[Christian Ehrhardt  (paelzer)]

I did some history on this as we had that in the past.
We had it for bug 1678322
But then in a discussion around upstreaming dropped it https://www.redhat.com/archives/libvir-list/2017-December/msg00714.html

What we missed back then is this:
A) system has hostdev in initial guest, virt-aa-helper adds /dev/vfio/vfio and all works
B) as A, but later one does hotplug with such a device - still works
C) the initial system has no hostdev, so virt-aa-helper adds no rule
   But if on (c) later hotplug is done it will break as it will need to access /dev/vfio/vfio
   before it knows anything.
   The actual per device rule is added to the guest which is all the isolation we want.

Now one could try to find the spot in BEFORE the guest needs /dev/vfio/vfio and add it to the guest profile then. But I haven't found it and until one can spend more time on this we need to bring the static rule back.

After all /dev/vfio/vfio is safe to be shared.

So I don't mind if someone codes up a long term dynamic vfio access, but for now add the line to un-break case (C).

[Christian Ehrhardt  (paelzer)]

Submitted for upstream review at:
  https://www.redhat.com/archives/libvir-list/2018-June/msg00846.html

If the mood is too much on "we need the super total dynamic rule" then we will take it as Ubuntu Delta for now, but I really think the change is safe and should be acceptable.

[Christian Ehrhardt  (paelzer)]

Upstream accepted, now integrating to Cosmic

[Christian Ehrhardt  (paelzer)]

Prepared the upload for Cosmic

[Launchpad Janitor (janitor)]

This bug was fixed in the package libvirt - 4.0.0-1ubuntu12

---------------
libvirt (4.0.0-1ubuntu12) cosmic; urgency=medium

  * d/p/ubuntu-aa/lp1775777-vfio-usage-without-initial-hostdev.patch: fix
    hotplug use cases where the initial guest had no hostdev at all and
    therefore vrit-aa-helper did not allow /dev/vfio/vfio (LP: #1775777)

 -- Christian Ehrhardt <email address hidden> Tue, 12 Jun 2018 16:24:01 +0200

[Christian Ehrhardt  (paelzer)]

- Final test build and test from PPA for target release
- Added SRU Template
- Uploaded to review/acceptance by the SRU Team
- Pushed to package repositories as we consider it proposed

Waiting on SRU Team now.

[Robie Basak (racb)]

Hello , or anyone else affected,

Accepted libvirt into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libvirt/4.0.0-1ubuntu8.3 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-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. 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!

[Christian Ehrhardt  (paelzer)]

Upgraded:
libvirt-clients/bionic-proposed 4.0.0-1ubuntu8.3 amd64 [upgradable from: 4.0.0-1ubuntu8.2]
libvirt-daemon/bionic-proposed 4.0.0-1ubuntu8.3 amd64 [upgradable from: 4.0.0-1ubuntu8.2]
libvirt-daemon-system/bionic-proposed 4.0.0-1ubuntu8.3 amd64 [upgradable from: 4.0.0-1ubuntu8.2]
libvirt0/bionic-proposed 4.0.0-1ubuntu8.3 amd64 [upgradable from: 4.0.0-1ubuntu8.2]

Upgrade worked fine as expected.

Guest without hostdev initially:

$ virsh attach-device bionic-q35-manycpu-alldevs hw-nvme-b2.xml
Device attached successfully

Worked as expected no apparmor denies anymore.

[Christian Ehrhardt  (paelzer)]

In addition we had the same pushed through regression tests (on ppa with same content).

[Launchpad Janitor (janitor)]

This bug was fixed in the package libvirt - 4.0.0-1ubuntu8.3

---------------
libvirt (4.0.0-1ubuntu8.3) bionic; urgency=medium

  * d/p/ubuntu-aa/lp1775777-vfio-usage-without-initial-hostdev.patch: fix
    hotplug use cases where the initial guest had no hostdev at all and
    therefore vrit-aa-helper did not allow /dev/vfio/vfio (LP: #1775777)

 -- Christian Ehrhardt <email address hidden> Wed, 13 Jun 2018 10:48:18 +0200

[Łukasz Zemczak (sil2100)]

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.