from:"Christian Ehrhardt"

On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa  wrote:
>
> On Tue, Jan 19, 2021 at 11:23:16 +0100, Christian Ehrhardt wrote:
> > When adding a rule for an image file and that image file has a chain
> > of backing files then we need to add a rule for each of those files.
> >
> > To get that iterate over the backing file chain the same way as
> > dac/selinux already do and add a label for each.
> >
> > Fixes: https://gitlab.com/libvirt/libvirt/-/issues/118
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/security_apparmor.c | 39 ++--
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/security/security_apparmor.c 
> > b/src/security/security_apparmor.c
> > index 29f0956d22..1f309c0c9f 100644
> > --- a/src/security/security_apparmor.c
> > +++ b/src/security/security_apparmor.c
> > @@ -756,22 +756,13 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
> >
> >  /* Called when hotplugging */
> >  static int
> > -AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> > -  virDomainDefPtr def,
> > -  virStorageSourcePtr src,
> > -  virSecurityDomainImageLabelFlags flags 
> > G_GNUC_UNUSED)
> > +AppArmorSetSecurityImageLabelInternal(virSecurityManagerPtr mgr,
> > +  virDomainDefPtr def,
> > +  virStorageSourcePtr src)
> >  {
> > -virSecurityLabelDefPtr secdef;
> >  g_autofree char *vfioGroupDev = NULL;
> >  const char *path;
> >
> > -secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> > -if (!secdef || !secdef->relabel)
> > -return 0;
> > -
> > -if (!secdef->imagelabel)
> > -return 0;
> > -
> >  if (src->type == VIR_STORAGE_TYPE_NVME) {
> >  const virStorageSourceNVMeDef *nvme = src->nvme;
> >
> > @@ -797,6 +788,30 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr 
> > mgr,
> >  return reload_profile(mgr, def, path, true);
> >  }
> >
> > +static int
> > +AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> > +  virDomainDefPtr def,
> > +  virStorageSourcePtr src,
> > +  virSecurityDomainImageLabelFlags flags 
> > G_GNUC_UNUSED)
> > +{
> > +virSecurityLabelDefPtr secdef;
> > +virStorageSourcePtr n;
> > +
> > +secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> > +if (!secdef || !secdef->relabel)
> > +return 0;
> > +
> > +if (!secdef->imagelabel)
> > +return 0;
>
> So apparmor doesn't support per-image security labels?
>
> > +
> > +for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
> > +if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0)
>
> It feels a bit suboptimal to fork+exec the aahelper for every single
> image. The selinux/dac drivers collect the list of things to do before
> forking when we are in the transaction mode (or do just chown/selinux
> labelling, which is trivial)
>
> Given that this is usually on an expensive code path, it's probably okay
> for now though.

We are ok with the part above in the thread we have so far.
But I've realized that I've forgotten Jim on my initial submission.

@Jim Fehlig any ack/nack/comment on this before I consider pushing it
now that 7.0 is out?

> Reviewed-by: Peter Krempa 
>
>
> > +return -1;
> > +}
> > +
> > +return 0;
> > +}
> > +
> >  static int
> >  AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> > virDomainDefPtr def)
> > --
> > 2.30.0
> >
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: let image label setting loop over backing files

On Tue, Jan 19, 2021 at 12:28 PM Peter Krempa  wrote:
>
> On Tue, Jan 19, 2021 at 12:15:31 +0100, Christian Ehrhardt wrote:
> > On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa  wrote:
> > >
> > > On Tue, Jan 19, 2021 at 11:23:16 +0100, Christian Ehrhardt wrote:
> > > > When adding a rule for an image file and that image file has a chain
> > > > of backing files then we need to add a rule for each of those files.
> > > >
> > > > To get that iterate over the backing file chain the same way as
> > > > dac/selinux already do and add a label for each.
> > > >
> > > > Fixes: https://gitlab.com/libvirt/libvirt/-/issues/118
> > > >
> > > > Signed-off-by: Christian Ehrhardt 
> > > > ---
> > > >  src/security/security_apparmor.c | 39 ++--
> > > >  1 file changed, 27 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/src/security/security_apparmor.c 
> > > > b/src/security/security_apparmor.c
> > > > index 29f0956d22..1f309c0c9f 100644
> > > > --- a/src/security/security_apparmor.c
> > > > +++ b/src/security/security_apparmor.c
> > > > @@ -756,22 +756,13 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr 
> > > > mgr,
>
> [...]
>
> > > > +static int
> > > > +AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> > > > +  virDomainDefPtr def,
> > > > +  virStorageSourcePtr src,
> > > > +  virSecurityDomainImageLabelFlags flags 
> > > > G_GNUC_UNUSED)
> > > > +{
> > > > +virSecurityLabelDefPtr secdef;
> > > > +virStorageSourcePtr n;
> > > > +
> > > > +secdef = virDomainDefGetSecurityLabelDef(def, 
> > > > SECURITY_APPARMOR_NAME);
> > > > +if (!secdef || !secdef->relabel)
> > > > +return 0;
> > > > +
> > > > +if (!secdef->imagelabel)
> > > > +return 0;
> > >
> > > So apparmor doesn't support per-image security labels?
> >
> > This was present before, it just got moved as part of this change.
> > IIRC for apparmor that is only generated once in AppArmorGenSecurityLabel
> > and later on only used to check if the struct is ok (if it would be NULL 
> > that
> > would indicate a non initialized element).
> >
> > If I'm missing some further hidden meaning of "imagelabel" please let me 
> > know.
>
> Here secdef->imagelabel is a global per-VM label, but you can configure
> a per disk (or rather per-image) label with a  element under
> . See:
>
> https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms
>
> right the first example.
>
> This allows to control labelling of individual files, but I'm not sure
> if such concept makes sense for apparmor.

AFAIU the concept would not make much sense for apparmor.
The seclabel/relable config per source are useful to control
dac/selinux and if they put labels on the entity of "a path/file".
But in that sense apparmor isn't controlling attributes of the path at
all, it is more like a whitelist associated to the qemu-process.

> For now it seems to be
> ignored, maybe it should be at least validated if it doesn't make sense.

I don't yet see how it would fit, but I appreciate the discussion -
thanks Peter!


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: let image label setting loop over backing files

On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa  wrote:
>
> On Tue, Jan 19, 2021 at 11:23:16 +0100, Christian Ehrhardt wrote:
> > When adding a rule for an image file and that image file has a chain
> > of backing files then we need to add a rule for each of those files.
> >
> > To get that iterate over the backing file chain the same way as
> > dac/selinux already do and add a label for each.
> >
> > Fixes: https://gitlab.com/libvirt/libvirt/-/issues/118
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/security_apparmor.c | 39 ++--
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/security/security_apparmor.c 
> > b/src/security/security_apparmor.c
> > index 29f0956d22..1f309c0c9f 100644
> > --- a/src/security/security_apparmor.c
> > +++ b/src/security/security_apparmor.c
> > @@ -756,22 +756,13 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
> >
> >  /* Called when hotplugging */
> >  static int
> > -AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> > -  virDomainDefPtr def,
> > -  virStorageSourcePtr src,
> > -  virSecurityDomainImageLabelFlags flags 
> > G_GNUC_UNUSED)
> > +AppArmorSetSecurityImageLabelInternal(virSecurityManagerPtr mgr,
> > +  virDomainDefPtr def,
> > +  virStorageSourcePtr src)
> >  {
> > -virSecurityLabelDefPtr secdef;
> >  g_autofree char *vfioGroupDev = NULL;
> >  const char *path;
> >
> > -secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> > -if (!secdef || !secdef->relabel)
> > -return 0;
> > -
> > -if (!secdef->imagelabel)
> > -return 0;
> > -
> >  if (src->type == VIR_STORAGE_TYPE_NVME) {
> >  const virStorageSourceNVMeDef *nvme = src->nvme;
> >
> > @@ -797,6 +788,30 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr 
> > mgr,
> >  return reload_profile(mgr, def, path, true);
> >  }
> >
> > +static int
> > +AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> > +  virDomainDefPtr def,
> > +  virStorageSourcePtr src,
> > +  virSecurityDomainImageLabelFlags flags 
> > G_GNUC_UNUSED)
> > +{
> > +virSecurityLabelDefPtr secdef;
> > +virStorageSourcePtr n;
> > +
> > +secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> > +if (!secdef || !secdef->relabel)
> > +return 0;
> > +
> > +if (!secdef->imagelabel)
> > +return 0;
>
> So apparmor doesn't support per-image security labels?

This was present before, it just got moved as part of this change.
IIRC for apparmor that is only generated once in AppArmorGenSecurityLabel
and later on only used to check if the struct is ok (if it would be NULL that
would indicate a non initialized element).

If I'm missing some further hidden meaning of "imagelabel" please let me know.

> > +
> > +for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
> > +if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0)
>
> It feels a bit suboptimal to fork+exec the aahelper for every single
> image.

I agree, but right now virt-aa-helper has no interface to take multiple
files in one pass.
Furthermore that also calls apparmor_parser which also could be done once.
Furthermore on the "append a rule" use case IMHO aa-helper isn't
doing much of it's original task - it becomes a overly huge "append a line and
call the parser".

I agree that we maybe should batch this. But when we touch it for that - at
least for the "append a rule" use case we might not want to use
virt-aa-helper at all.
But then this becomes a much bigger rewrite with moving e.g. the knowledge how
to get from UUID->filepath into libvirt to be usable from
virt-aa-helper (for other tasks)
but also from the labeling calls. A few more might move, like the
apparmor_parser call.

I've taken a note that it would be good to at least try and POC that, but TBH
that list is growing recently without much draining it :-)

But for the change presented I'd like to keep it to the issue that
we'd want to fix right now.

> The selinux/dac drivers collect the list of things to do before
> forking when we are in the transaction mode (or do just chown/selinux
> labelling, which is trivial)
>
> Given that this is usually on an expensive code path, it's probably okay
> for now though.

Thanks!

> Reviewed-by: Peter Krempa 
>
>
> > +return -1;
> > +}
> > +
> > +return 0;
> > +}
> > +
> >  static int
> >  AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> > virDomainDefPtr def)
> > --
> > 2.30.0
> >
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[PATCH] apparmor: let image label setting loop over backing files

When adding a rule for an image file and that image file has a chain
of backing files then we need to add a rule for each of those files.

To get that iterate over the backing file chain the same way as
dac/selinux already do and add a label for each.

Fixes: https://gitlab.com/libvirt/libvirt/-/issues/118

Signed-off-by: Christian Ehrhardt 
---
 src/security/security_apparmor.c | 39 ++--
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 29f0956d22..1f309c0c9f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -756,22 +756,13 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
 
 /* Called when hotplugging */
 static int
-AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
-  virDomainDefPtr def,
-  virStorageSourcePtr src,
-  virSecurityDomainImageLabelFlags flags 
G_GNUC_UNUSED)
+AppArmorSetSecurityImageLabelInternal(virSecurityManagerPtr mgr,
+  virDomainDefPtr def,
+  virStorageSourcePtr src)
 {
-virSecurityLabelDefPtr secdef;
 g_autofree char *vfioGroupDev = NULL;
 const char *path;
 
-secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
-if (!secdef || !secdef->relabel)
-return 0;
-
-if (!secdef->imagelabel)
-return 0;
-
 if (src->type == VIR_STORAGE_TYPE_NVME) {
 const virStorageSourceNVMeDef *nvme = src->nvme;
 
@@ -797,6 +788,30 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
 return reload_profile(mgr, def, path, true);
 }
 
+static int
+AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
+  virDomainDefPtr def,
+  virStorageSourcePtr src,
+  virSecurityDomainImageLabelFlags flags 
G_GNUC_UNUSED)
+{
+virSecurityLabelDefPtr secdef;
+virStorageSourcePtr n;
+
+secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
+if (!secdef || !secdef->relabel)
+return 0;
+
+if (!secdef->imagelabel)
+return 0;
+
+for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
+if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0)
+return -1;
+}
+
+return 0;
+}
+
 static int
 AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
virDomainDefPtr def)
-- 
2.30.0

Re: RFC: deprecating/obsoleting netcf package and libvirt virInterface*() APIs

2020-12-03 Thread Christian Ehrhardt

ging verbal agreement that it would be used in places other than
> libvirt (and thus there would be a community benefit in eliminated
> duplicate code in the multiple projects); this also never materialized,
> so in the end, it is a separate library that is only consumed by
> libvirt, but because it's a separate library the "barrier to entry" for
> anyone to make any changes to it is very high, and so it (effectively)
> never sees any contributions from the outside.)
>
> Because of all the above, I've thought for quite awhile that we should
> deprecate netcf itself, along with all of libvirt's virInterface APIs
> *except* the one that lists interfaces (virConnectListAllInterfaces) and

Switching to networkd/networkmanager from e/n/i caused
virConnectListAllInterfaces and such to fail for Ubuntu in 2018 [1].

This matches a bit the "occasional netcf BZes" that Laine mentioned,
just that we decided to cut it out and get rid of it and since then we
actively disable the netcf backend to avoid the various issues and
misunderstandings it can cause.

Other than "it won't make a difference for Ubuntu" the fact that makes
this worth for the thread is that it seems the original use case
wasn't important enough to get any complaints in 2.5 years since
disabling it early in Ubuntu 18.10.

IMHO like Daniel later suggested, maybe keeping the API but
neglecting the netcf backend is a good tradeoff to go for?

[1]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1764314

> the one that outputs an XML dump of the current status of interfaces
> (virInterfaceGetXMLDesc). Since netcf is only used by libvirt anyway,
> the part of functionality that performs those two tasks could be moved
> into one or two C files within libvirt, removing the dependency on netcf
> and making updates easier and more accessible to 3rd parties. As a
> followup to this, we might provide another backend that would use
> NetworkManager APIs to retrieve all this information rather than netlink
> messages (which is what netcf currently does).
>
> Alternately, Cole suggested in a separate email that since libvirt's
> node device driver already reports various status information about
> devices on the host, we could just beef up the output of
> virNodeDeviceGetXMLDesc() for net_* devices to include more of the info
> that's visible in "ip link" and "ip addr".
>
> Or, we could just decide that it's okay for a management application
> (the main consumer of libvirt) to need to use other APIs to get that
> information (especially since that's what they already do anyway!)
>
> So, that's my piece to speak. I'm looking for opinions and ideas on a
> few different fronts:
>
> 1) Does this generally sound like a good direction? Or is there
> something I'm ignoring that renders my points moot?
>
> 2) If we are going to do it, how should we proceed?
>
> We obviously can't simply *remove* the virInterface API from libvirt
> (since that would destroy backward compatibility guarantees), but could
> immediately begin logging some sort of "this API is deprecated" message
> when any of the functions are called, and then in a later release change
> the APIs to return an error (while simultaneously removing netcf from
> the build and dependency lists). At the same time, we would need to
> decide if the "interface status" functionality needs to be maintained
> within appropriate virInterface*() APIs, reproduced in
> virNodeDeviceGetXMLDesc(), or just dropped altogether.
>
> On the netcf side, there are several small patches that have been
> sitting in git for a few years without being in any official release; it
> would probably be nice to make one final release before closing up shop.
> The mailing list could then be closed down, and some final message put
> in a README in the git repo (on pagure.io) before putting it into some
> archival state.
>
> After those things are done, the various distros could be notified of
> the newfound irrelevance of netcf, and given the opportunity to remove
> the package from their releases.
>
> Anything else?
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 1/2] apparmor: Allow lxc processes to receive signals from libvirt

2020-12-03 Thread Christian Ehrhardt

On Thu, Dec 3, 2020 at 3:58 AM Jim Fehlig  wrote:
>
> LXC processes confined by apparmor are not permitted to receive signals
> from libvirtd. Attempting to destroy such a process fails
>
> virsh --connect lxc:/// destroy distro_apparmor
>  error: Failed to destroy domain distro_apparmor
>  error: Failed to kill process 29491: Permission denied
>
> And from /var/log/audit/audit.log
>
> type=AVC msg=audit(1606949706.142:6345): apparmor="DENIED"
> operation="signal" profile="libvirt-314b7109-fdce-48dc-ad28-7c47958a27c1"
> pid=29390 comm="libvirtd" requested_mask="receive" denied_mask="receive"
> signal=term peer="libvirtd"
>
> Similar to the libvirt-qemu abstraction, add a rule to the libvirt-lxc
> abstraction allowing reception of signals from libvirtd.

Agreed that it is the same rule as in libvirt-qemu and therefore
should be rather safe.

TBH I did not see the denial when testing 6.9.0 [1], but the pattern
is known and therefore I think adding the rule is fine.

Reviewed-by: Christian Ehrhardt 

[1]: 
https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-hirsute/hirsute/amd64/libv/libvirt/20201127_105058_4590a@/log.gz

> Signed-off-by: Jim Fehlig 
> ---
>  src/security/apparmor/libvirt-lxc | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/security/apparmor/libvirt-lxc 
> b/src/security/apparmor/libvirt-lxc
> index e556f2a7bd..0c8b812743 100644
> --- a/src/security/apparmor/libvirt-lxc
> +++ b/src/security/apparmor/libvirt-lxc
> @@ -1,5 +1,9 @@
>#include 
>
> + # Allow receiving signals from libvirtd
> +  signal (receive) peer=libvirtd,
> +  signal (receive) peer=/usr/sbin/libvirtd,
> +
>umount,
>
># ignore DENIED message on / remount
> --
> 2.29.2
>
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: Migration with "--p2p --tunnelled" hanging in v6.9.0

2020-11-26 Thread Christian Ehrhardt

On Wed, Nov 25, 2020 at 7:06 PM Daniel P. Berrangé  wrote:
>
> On Wed, Nov 25, 2020 at 05:20:41PM +, Daniel P. Berrangé wrote:
> > On Wed, Nov 25, 2020 at 05:00:20PM +, Daniel P. Berrangé wrote:
> > > On Wed, Nov 25, 2020 at 04:36:39PM +, Daniel P. Berrangé wrote:
> > > > On Wed, Nov 25, 2020 at 04:49:14PM +0100, Christian Ehrhardt wrote:
> > > > > I found that the same vol-download vs 127.0.0.1 gives the same 
> > > > > results.
> > > > > That in turn makes it easier to gather results as we only need one 
> > > > > system.
> > > >
> > > > Yep, that's useful, I'm able to reproduce this problem myself too
> > > > now. Will do some local tests and report back...
> > >
> > > Sigh, the problem is way too many reallocs, repeatedly growing and 
> > > shrinking
> > > the buffer we use for I/O.
> > >
> > > I guess we never noticed this awfulness in the virsh console code it was
> > > copied from, as the data volumes are lower.
> > >
> > > Switching to a fixed size buffer makes it massively faster. I'll prep a
> > > patch asap.
> >
> > Actually, it is also important to have a bigger buffer. Using a 1 MB buffer
> > makes all the difference in throughput.
>
> I've sent two patches to improve the performance and managed to test
> vol-download with our ssh helper to beat netcat. I've not had a chance
> to test with tunnelled migration yet though, so if you want to try the
> patches with your migration test scenario, that'd be useful confirmation

Thank you Daniel,
I've done extensive testing (all good) and replied with details and a
Tested-by to the patch submission.

>
> Regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [libvirt PATCH 0/2] fix regression in SSH tunnelling performance

2020-11-26 Thread Christian Ehrhardt

On Wed, Nov 25, 2020 at 7:04 PM Daniel P. Berrangé  wrote:
>
> In testing the "vol-download" command in virsh, downloading a
> 1G file takes a ridiculous amount of time (minutes) with the
> new SSH helper.
>
> After the first patch is applied the time gets down to a much
> more reasonable 5.5 seconds on my test machine.
>
> By comparison netcat achieved 4 seconds.
>
> After applying the second patch, the time is reduced to 3.5
> seconds, so we actually end up beating netcat, as long as
> we have glib >= 2.64.0 available.
>
> Daniel P. Berrangé (2):
>   remote: make ssh-helper massively faster
>   util: avoid glib event loop workaround where possible

I tested both patches and they didn't bring any new regressions as far
as my tests went.
The former issues of slow migration and vol-download are fixed and
speed is now roughly on-par with the old netcat mode.

Tested:
- vol-download local
- vol-download remote
- migration with explicit native transfer ?proxy=native
- migration with auto-mode selection
- virsh console

I also tested migrations with between patched and older versions:
- patched <-> 6.6 - fall back to netcat (expected and ok).
- patched -> 6.9 - slow (as before)
- 6.9 -> patched - fast (which is good as upgrade paths use migration
and it is sufficient to upgrade the target)

Tested-by: Christian Ehrhardt 

Thank you Daniel!

>  src/remote/remote_ssh_helper.c | 113 -
>  src/util/vireventglib.c|  29 ++---
>  2 files changed, 89 insertions(+), 53 deletions(-)
>
> --
> 2.25.4
>
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: Migration with "--p2p --tunnelled" hanging in v6.9.0

On Wed, Nov 25, 2020 at 2:47 PM Daniel P. Berrangé  wrote:
>
> On Wed, Nov 25, 2020 at 02:33:44PM +0100, Christian Ehrhardt wrote:
> > On Wed, Nov 25, 2020 at 1:38 PM Daniel P. Berrangé  
> > wrote:
> > >
> > > On Wed, Nov 25, 2020 at 01:28:09PM +0100, Christian Ehrhardt wrote:
> > > > On Wed, Nov 25, 2020 at 10:55 AM Christian Ehrhardt
> > > >  wrote:
> > > > >
> > > > > On Tue, Nov 24, 2020 at 4:30 PM Peter Krempa  
> > > > > wrote:
> > > > > >
> > > > > > On Tue, Nov 24, 2020 at 16:05:53 +0100, Christian Ehrhardt wrote:
> > > > > > > Hi,
> > > > > >
> > > > > > [...]
> > > > >
> > > > > BTW to reduce the scope what to think about - I have rebuilt 6.8 as
> > > > > well it works.
> > > > > Thereby I can confirm that the offending change should be in between
> > > > > 6.8.0 -> 6.9.0.
> > > >
> > > > I was able to get this working in git bisect builds from git between
> > > > v6.8 / v6.9.
> > > > I identified the following offending commit:
> > > >   7d959c30  rpc: Fix virt-ssh-helper detection
> > > >
> > > > Ok that makes a bit of sense, first we had in 6.8
> > > >   f8ec7c84 rpc: use new virt-ssh-helper binary for remote tunnelling
> > > > That makes it related to tunneling which matches our broken use-case.
> > > >
> > > > The identified commit "7d959c30 rpc: Fix virt-ssh-helper detection" 
> > > > might
> > > > finally really enable the new helper and that is then broken?
> > > >
> > > > With that knowledge I was able to confirm that it really is the native 
> > > > mode
> > > >
> > > > $ virsh migrate --unsafe --live --p2p --tunnelled h-migr-test
> > > > qemu+ssh://testkvm-hirsute-to/system?proxy=netcat
> > > > 
> > > > $ virsh migrate --unsafe --live --p2p --tunnelled h-migr-test
> > > > qemu+ssh://testkvm-hirsute-to/system?proxy=native
> > > > 
> > > >
> > > > I recently discussed with Andrea if we'd need apparmor rules for
> > > > virt-ssh-helper,
> > > > but there are no denials nor libvirt log entries related to 
> > > > virt-ssh-helper.
> > > > But we don't need such rules since it is spawned on the ssh login and
> > > > not under libvirtd itself.
> > > >
> > > > PS output of the hanging receiving virt-ssh-helper (looks not too 
> > > > unhappy):
> > > > Source:
> > > > 4 0   41305   1  20   0 1627796 23360 poll_s Ssl ?
> > > > 0:05 /usr/sbin/libvirtd
> > > > 0 0   41523   41305  20   0   9272  4984 poll_s S?
> > > > 0:02  \_ ssh -T -e none -- testkvm-hirsute-to sh -c 'virt-ssh-helper
> > > > 'qemu:///system''
> > > > Target
> > > > 4 0 213   1  20   0  13276  4132 poll_s Ss   ?
> > > > 0:00 sshd: /usr/sbin/sshd -D [listener] 0 of 250-500 startups
> > > > 4 0   35148 213  20   0  19048 11320 poll_s Ss   ?
> > > > 0:02  \_ sshd: root@notty
> > > > 4 0   35206   35148  20   0   2584   544 do_wai Ss   ?
> > > > 0:00  \_ sh -c virt-ssh-helper qemu:///system
> > > > 0 0   35207   35206  20   0  81348 26684 -  R?
> > > > 0:34  \_ virt-ssh-helper qemu:///system
> > > >
> > > > I've looked at it with strace [1] and gdb for backtraces [2] - it is
> > > > not dead or stuck and keeps working.
> > > > Could it be just so slow that it appears to hang until it times out?
> > > > Or is the event mechanism having issues and it wakes up too rarely?
> > >
> > > Lets take migration out of the picture. What if you simply do
> > >
> > > virsh -c qemu+ssh://testkvm-hirsute-to/system?proxy=native  list
> > >
> > > does that work ?
> >
> > Yes it does, no hang and proper results
>
> Ok, so that shows  virt-ssh-helper is not completely broken at least.
>
> Makes me think there is possibly something related to streams code
> that causes the issue.
>
> You might try the virsh "console" or "vol-upload" commands to test
> the streams stuff in isolation. If that also works, then the problem
> is specific to migration,

Thanks for the hint Daniel, it is indeed not migration specific - it
seems that virs-ssh-helper is just very slow.

rm tes

Re: Migration with "--p2p --tunnelled" hanging in v6.9.0

On Wed, Nov 25, 2020 at 1:38 PM Daniel P. Berrangé  wrote:
>
> On Wed, Nov 25, 2020 at 01:28:09PM +0100, Christian Ehrhardt wrote:
> > On Wed, Nov 25, 2020 at 10:55 AM Christian Ehrhardt
> >  wrote:
> > >
> > > On Tue, Nov 24, 2020 at 4:30 PM Peter Krempa  wrote:
> > > >
> > > > On Tue, Nov 24, 2020 at 16:05:53 +0100, Christian Ehrhardt wrote:
> > > > > Hi,
> > > >
> > > > [...]
> > >
> > > BTW to reduce the scope what to think about - I have rebuilt 6.8 as
> > > well it works.
> > > Thereby I can confirm that the offending change should be in between
> > > 6.8.0 -> 6.9.0.
> >
> > I was able to get this working in git bisect builds from git between
> > v6.8 / v6.9.
> > I identified the following offending commit:
> >   7d959c30  rpc: Fix virt-ssh-helper detection
> >
> > Ok that makes a bit of sense, first we had in 6.8
> >   f8ec7c84 rpc: use new virt-ssh-helper binary for remote tunnelling
> > That makes it related to tunneling which matches our broken use-case.
> >
> > The identified commit "7d959c30 rpc: Fix virt-ssh-helper detection" might
> > finally really enable the new helper and that is then broken?
> >
> > With that knowledge I was able to confirm that it really is the native mode
> >
> > $ virsh migrate --unsafe --live --p2p --tunnelled h-migr-test
> > qemu+ssh://testkvm-hirsute-to/system?proxy=netcat
> > 
> > $ virsh migrate --unsafe --live --p2p --tunnelled h-migr-test
> > qemu+ssh://testkvm-hirsute-to/system?proxy=native
> > 
> >
> > I recently discussed with Andrea if we'd need apparmor rules for
> > virt-ssh-helper,
> > but there are no denials nor libvirt log entries related to virt-ssh-helper.
> > But we don't need such rules since it is spawned on the ssh login and
> > not under libvirtd itself.
> >
> > PS output of the hanging receiving virt-ssh-helper (looks not too unhappy):
> > Source:
> > 4 0   41305   1  20   0 1627796 23360 poll_s Ssl ?
> > 0:05 /usr/sbin/libvirtd
> > 0 0   41523   41305  20   0   9272  4984 poll_s S?
> > 0:02  \_ ssh -T -e none -- testkvm-hirsute-to sh -c 'virt-ssh-helper
> > 'qemu:///system''
> > Target
> > 4 0 213   1  20   0  13276  4132 poll_s Ss   ?
> > 0:00 sshd: /usr/sbin/sshd -D [listener] 0 of 250-500 startups
> > 4 0   35148 213  20   0  19048 11320 poll_s Ss   ?
> > 0:02  \_ sshd: root@notty
> > 4 0   35206   35148  20   0   2584   544 do_wai Ss   ?
> > 0:00  \_ sh -c virt-ssh-helper qemu:///system
> > 0 0   35207   35206  20   0  81348 26684 -  R?
> > 0:34  \_ virt-ssh-helper qemu:///system
> >
> > I've looked at it with strace [1] and gdb for backtraces [2] - it is
> > not dead or stuck and keeps working.
> > Could it be just so slow that it appears to hang until it times out?
> > Or is the event mechanism having issues and it wakes up too rarely?
>
> Lets take migration out of the picture. What if you simply do
>
> virsh -c qemu+ssh://testkvm-hirsute-to/system?proxy=native  list
>
> does that work ?

Yes it does, no hang and proper results

I migrated the system there (with ?proxy=netcat) to ensure there is
something to report and indeed some data is coming back through this.

root@testkvm-hirsute-from:~# virsh migrate --unsafe --live --p2p
--tunnelled h-migr-test
qemu+ssh://testkvm-hirsute-to/system?proxy=netcat
root@testkvm-hirsute-from:~# virsh -c
qemu+ssh://testkvm-hirsute-to/system?proxy=native  list
 Id   Name  State
-
 6h-migr-test   running



> Regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: Migration with "--p2p --tunnelled" hanging in v6.9.0

On Wed, Nov 25, 2020 at 10:55 AM Christian Ehrhardt
 wrote:
>
> On Tue, Nov 24, 2020 at 4:30 PM Peter Krempa  wrote:
> >
> > On Tue, Nov 24, 2020 at 16:05:53 +0100, Christian Ehrhardt wrote:
> > > Hi,
> >
> > [...]
>
> BTW to reduce the scope what to think about - I have rebuilt 6.8 as
> well it works.
> Thereby I can confirm that the offending change should be in between
> 6.8.0 -> 6.9.0.

I was able to get this working in git bisect builds from git between
v6.8 / v6.9.
I identified the following offending commit:
  7d959c30  rpc: Fix virt-ssh-helper detection

Ok that makes a bit of sense, first we had in 6.8
  f8ec7c84 rpc: use new virt-ssh-helper binary for remote tunnelling
That makes it related to tunneling which matches our broken use-case.

The identified commit "7d959c30 rpc: Fix virt-ssh-helper detection" might
finally really enable the new helper and that is then broken?

With that knowledge I was able to confirm that it really is the native mode

$ virsh migrate --unsafe --live --p2p --tunnelled h-migr-test
qemu+ssh://testkvm-hirsute-to/system?proxy=netcat

$ virsh migrate --unsafe --live --p2p --tunnelled h-migr-test
qemu+ssh://testkvm-hirsute-to/system?proxy=native


I recently discussed with Andrea if we'd need apparmor rules for
virt-ssh-helper,
but there are no denials nor libvirt log entries related to virt-ssh-helper.
But we don't need such rules since it is spawned on the ssh login and
not under libvirtd itself.

PS output of the hanging receiving virt-ssh-helper (looks not too unhappy):
Source:
4 0   41305   1  20   0 1627796 23360 poll_s Ssl ?
0:05 /usr/sbin/libvirtd
0 0   41523   41305  20   0   9272  4984 poll_s S?
0:02  \_ ssh -T -e none -- testkvm-hirsute-to sh -c 'virt-ssh-helper
'qemu:///system''
Target
4 0 213   1  20   0  13276  4132 poll_s Ss   ?
0:00 sshd: /usr/sbin/sshd -D [listener] 0 of 250-500 startups
4 0   35148 213  20   0  19048 11320 poll_s Ss   ?
0:02  \_ sshd: root@notty
4 0   35206   35148  20   0   2584   544 do_wai Ss   ?
0:00  \_ sh -c virt-ssh-helper qemu:///system
0 0   35207   35206  20   0  81348 26684 -  R?
0:34  \_ virt-ssh-helper qemu:///system

I've looked at it with strace [1] and gdb for backtraces [2] - it is
not dead or stuck and keeps working.
Could it be just so slow that it appears to hang until it times out?
Or is the event mechanism having issues and it wakes up too rarely?

Also did anyone else see the same with >=v6.9.0?

[1]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1904584/comments/12
[2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1904584/comments/13


> > > In git/news I only found these changes which sounded to be relevant:
> > >   f51cbe92c0 qemu: Allow migration over UNIX socket
> > >   c69915ccaf peer2peer migration: allow connecting to local sockets
> > > But I'm not using unix: and in the logs the only unix: mentions are for 
> > > the
> > > qemu monitor and qemu-guest-agent.
> >
> > One very important part of '--tunnelled' migration is the use of
> > virStream APIs to transport the migration data. Perhaps something there
> > is broken since it doesn't reproduce when not using the tunnel
>
> Thanks for the hint Peter.
> I was now looking there as well, but other than the switch to  g_new0
> there is neither a change carrying the words "stream" nor one that
> is affecting the related files that implement virStream*.
>
> --
> Christian Ehrhardt
> Staff Engineer, Ubuntu Server
> Canonical Ltd



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: Migration with "--p2p --tunnelled" hanging in v6.9.0

On Tue, Nov 24, 2020 at 4:30 PM Peter Krempa  wrote:
>
> On Tue, Nov 24, 2020 at 16:05:53 +0100, Christian Ehrhardt wrote:
> > Hi,
>
> [...]

BTW to reduce the scope what to think about - I have rebuilt 6.8 as
well it works.
Thereby I can confirm that the offending change should be in between
6.8.0 -> 6.9.0.

> > In git/news I only found these changes which sounded to be relevant:
> >   f51cbe92c0 qemu: Allow migration over UNIX socket
> >   c69915ccaf peer2peer migration: allow connecting to local sockets
> > But I'm not using unix: and in the logs the only unix: mentions are for the
> > qemu monitor and qemu-guest-agent.
>
> One very important part of '--tunnelled' migration is the use of
> virStream APIs to transport the migration data. Perhaps something there
> is broken since it doesn't reproduce when not using the tunnel

Thanks for the hint Peter.
I was now looking there as well, but other than the switch to  g_new0
there is neither a change carrying the words "stream" nor one that
is affecting the related files that implement virStream*.

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Migration with "--p2p --tunnelled" hanging in v6.9.0

2020-11-24 Thread Christian Ehrhardt

Hi,
I'm wondering about the best next steps to debug a migration issue.
What I found is that with libvirt v6.9.0 a migration hangs if used like:
$ virsh migrate --unsafe --live --p2p --tunnelled h-migr-test \
qemu+ssh://testkvm-hirsute-to/system

Just "--live --p2p" works fine. Also a bunch of other migration option
combinations work with the same build/setup, just p2p+tunnelled fails.
Also if either source or target are not on 6.9 the migration works
(former version used is v6.6 for me).

I looked at alternating qemu versions (5.0 / 5.1), but it had no impact.
It only depends on libvirt to be on the new version on source to
trigger the issue.

Unfortunately there is no crash or error to debug into, it just gets stuck
with the "virsh migrate" hanging on the source and never leaving the "paused"
state on the target.

I have compared setups with the least amount of "change":
good: Qemu 5.1 + Libvirt 6.9 -> Qemu 5.1 / Libvirt 6.6
bad: Qemu 5.1 + Libvirt 6.9 -> Qemu 5.1 / Libvirt 6.9
[1] has the debug logs of those, beginning with a libvirtd restart that one can
likely skip and then into the migration that hangs in the bad case.
But I failed to see an obvious reason in the log.

In git/news I only found these changes which sounded to be relevant:
f51cbe92c0 qemu: Allow migration over UNIX socket
c69915ccaf peer2peer migration: allow connecting to local sockets
But I'm not using unix: and in the logs the only unix: mentions are for the
qemu monitor and qemu-guest-agent.

I wanted to ask:
- if something related was recently changed that comes to mind?
- if someone else sees anything in the linked logs that I missed?
- if someone else has seen/reproduced the same?
- for best practise to debug a hanging migration

Thanks in advance!

[1]:
https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1904584/+attachment/5437541/+files/full-log.tgz

--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: allow kvm-spice compat wrapper

2020-11-18 Thread Christian Ehrhardt

On Wed, Nov 18, 2020 at 10:38 AM Daniel P. Berrangé  wrote:
>
> On Tue, Nov 17, 2020 at 09:11:48PM -0500, Neal Gompa wrote:
> > On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt
> >  wrote:
> > >
> > > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik  
> > > wrote:
> > > >
> > > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> > > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a 
> > > > > wrapper
> > > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> > > > > for quite a while anymore, but required to work for compatibility e.g.
> > > > > when migrating in old guests.
> > > > >
> > > > > For years this was a symlink kvm-spice->kvm and therefore covered
> > > > > apparmor-wise by the existing entry:
> > > > > /usr/bin/kvm rmix,
> > > > > But due to a recent change [1] in qemu packaging this now is no 
> > > > > symlink,
> > > > > but a wrapper on its own and therefore needs an own entry that allows 
> > > > > it
> > > > > to be executed.
> > > > >
> > > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> > > > >
> > > > > Signed-off-by: Christian Ehrhardt 
> > > > > ---
> > > > >   src/security/apparmor/libvirt-qemu | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > >
> > > > Reviewed-by: Michal Privoznik 
> > >
> > > Thank you Michal,
> > > it also passed fine through my tests (as backport to 6.8 and 6.9).
> > > We are not in any freeze, review has happened, tests LGTM - pushed to git.
> > >
> >
> > Hold up, why was this merged? Did anyone validate whether this would
> > break the other AppArmor user (SUSE)?
> >
> > Unlike SELinux, AppArmor functionality is quite fragmented between
> > Ubuntu and SUSE distributions (the two major users of AppArmor), and
> > there did not seem to be any indication that this AppArmor patch was
> > validated with openSUSE before merging. My personal experience with
> > AppArmor across the two distribution families is that it's really easy
> > to make profiles that work for Ubuntu but fail on SUSE because of the
> > disparity of functionality. I also don't see Jim Fehlig stepping in to
> > indicate that this worked for him.
> >
> > I haven't had a chance to test this myself, but I am immediately
> > suspicious of a change that references a commit based on Debian
> > packaging of QEMU.
>
> Historically the AppArmor policy in libvirt has been exclusively
> maintained and tested by the Debian and Ubuntu maintainers. We have
> never considered SUSE in any changes made to it.

Ack to what Daniel wrote.
In addition neither are other - be it Suse or 3rd party - changes
gated on Debian/Ubuntu testing them.
If I fail to catch the changes on the ML-discussion as part of staring
at my inbox, then the testing for us happens whenever we merge a new
upstream version.

The general rule of thumb that we not-strictly followed in recent times aret:
- logical changes e.g. to virt-aa-helper will have a build time
self-test associated
- labelling changes (related to hot add for example) are usually up
for discussion a
  bit longer and tested by the author in the context that the issues were found
- rule allow-additions (like the one here) are discussed and added if
there are no security concerns

I don't remember we've made anything more restrictive recently, that
would probably be somewhere between the latter two points above.

The duration also depends on the complexity - in this particular case
as Michal already stated there isn't a high chance of breaking
something with it.

OTOH I'm fine to work out something more official/established if you
want to define something - but keep in mind that many of us do this as
a fraction of a part of their duties. Due to that sometimes
human/machine time isn't available for short round trip times which
are needed for a formal gating process.

> Regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: allow kvm-spice compat wrapper

2020-11-17 Thread Christian Ehrhardt

On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik  wrote:
>
> On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> > 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
> > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> > for quite a while anymore, but required to work for compatibility e.g.
> > when migrating in old guests.
> >
> > For years this was a symlink kvm-spice->kvm and therefore covered
> > apparmor-wise by the existing entry:
> > /usr/bin/kvm rmix,
> > But due to a recent change [1] in qemu packaging this now is no symlink,
> > but a wrapper on its own and therefore needs an own entry that allows it
> > to be executed.
> >
> > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >   src/security/apparmor/libvirt-qemu | 1 +
> >   1 file changed, 1 insertion(+)
> >
>
> Reviewed-by: Michal Privoznik 

Thank you Michal,
it also passed fine through my tests (as backport to 6.8 and 6.9).
We are not in any freeze, review has happened, tests LGTM - pushed to git.

> Michal
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: nwfilter issue with new ebtables

2020-11-16 Thread Christian Ehrhardt

On Mon, Nov 16, 2020 at 4:24 PM Laine Stump  wrote:
>
> On 11/16/20 2:01 AM, Christian Ehrhardt wrote:
> > Hi,
> > I have last week discussed breakage in nwfilter usage on IRC
> >
> > 
> >   
> > 
> > virsh start 
> >error: Failed to start domain 
> >error: internal error: applyDHCPOnlyRules failed - spoofing not protect
> >
> > With debug in the logs enabled I got confirmation by Daniel (thanks!)
> > that the command sequence libvirt issued looked kind of "normal".
> >
> > Hereby I wanted to let you know that some further debugging identified
> > a part of the sequence that libvirt issues as being broken in recent
> > ebtables versions.
> >
> ># ebtables --concurrent -t nat -N testrule3
> ># ebtables --concurrent -t nat -E testrule3 testrule3-renamed
> >ebtables v1.8.6 (nf_tables): Chain 'testrule3' doesn't exists
>
>
> So you're saying you can just run those two commands together and always
> get the error? (assuming that "testrule3 and testrule3-renamed don't
> exist beforehand)

yes

>  From your description it sounds like maybe the error doesn't occur when
> there is a pause between the two commands - is that right, or am I
> assuming too much?

Assuming too much, it happens when libvirt issues them at "computer
speed" as well as when I run them manually at "human speed".
I have not tried waiting an extra long time in between thou ...

>
> I tried the above commands (well, I put the two commands together on a
> single line separated by ";") on a Fedora 33 system and a RHEL 8.3.0
> system, and both of them completed successfully.
>
>
> This is the fedora ebtables -V: ebtables v2.0.11 (legacy) (December 2011)

Those worked on Ubuntu as well in older releases.

>
> And this is the ebtables -V on RHEL 8.3.0: ebtables 1.8.4 (nf_tables)

That since 1.8.5 is what is broken for us at the moment.

Thanks for cross checking Laine!

> (I don't have any idea how the version's relate to each other for legacy
> ebtables vs. the nf_tables version)
>
>
> > This led to upstream ebtables bug [1] - for now just FYI in case you
> > want/need to subscribe for your own tracking.
> >
> > [1]: https://bugzilla.netfilter.org/show_bug.cgi?id=1481
> >
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[PATCH] apparmor: allow kvm-spice compat wrapper

2020-11-16 Thread Christian Ehrhardt

'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
for quite a while anymore, but required to work for compatibility e.g.
when migrating in old guests.

For years this was a symlink kvm-spice->kvm and therefore covered
apparmor-wise by the existing entry:
   /usr/bin/kvm rmix,
But due to a recent change [1] in qemu packaging this now is no symlink,
but a wrapper on its own and therefore needs an own entry that allows it
to be executed.

[1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3

Signed-off-by: Christian Ehrhardt 
---
 src/security/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index a03e9e2c94..85c9e61d6c 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -102,6 +102,7 @@
 
   # the various binaries
   /usr/bin/kvm rmix,
+  /usr/bin/kvm-spice rmix,
   /usr/bin/qemu rmix,
   /usr/bin/qemu-aarch64 rmix,
   /usr/bin/qemu-alpha rmix,
-- 
2.29.2

nwfilter issue with new ebtables

2020-11-15 Thread Christian Ehrhardt

Hi,
I have last week discussed breakage in nwfilter usage on IRC

   
 
   
virsh start 
  error: Failed to start domain 
  error: internal error: applyDHCPOnlyRules failed - spoofing not protect

With debug in the logs enabled I got confirmation by Daniel (thanks!)
that the command sequence libvirt issued looked kind of "normal".

Hereby I wanted to let you know that some further debugging identified
a part of the sequence that libvirt issues as being broken in recent
ebtables versions.

  # ebtables --concurrent -t nat -N testrule3
  # ebtables --concurrent -t nat -E testrule3 testrule3-renamed
  ebtables v1.8.6 (nf_tables): Chain 'testrule3' doesn't exists

This led to upstream ebtables bug [1] - for now just FYI in case you
want/need to subscribe for your own tracking.

[1]: https://bugzilla.netfilter.org/show_bug.cgi?id=1481

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [RFC PATCH 2/2] qemu: Add support for max physical address size

2020-11-02 Thread Christian Ehrhardt

; +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> diff --git a/tests/qemuxml2argvdata/cpu-phys-bits-passthrough.xml
> b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough.xml
> new file mode 100644
> index 00..db570beb8d
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough.xml
> @@ -0,0 +1,20 @@
> +
> +  foo
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219136
> +  219136
> +  1
> +  
> +hvm
> +
> +  
> +  
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +  
> +
> diff --git a/tests/qemuxml2argvdata/cpu-phys-bits-passthrough2.err
> b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough2.err
> new file mode 100644
> index 00..22009cc6e6
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough2.err
> @@ -0,0 +1 @@
> +unsupported configuration: CPU maximum physical address bits mode
> 'passthrough' can only be used with 'host-passthrough' CPUs
> diff --git a/tests/qemuxml2argvdata/cpu-phys-bits-passthrough2.xml
> b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough2.xml
> new file mode 100644
> index 00..511bbf9949
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough2.xml
> @@ -0,0 +1,20 @@
> +
> +  foo
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219136
> +  219136
> +  1
> +  
> +hvm
> +
> +  
> +  
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +  
> +
> diff --git a/tests/qemuxml2argvdata/cpu-phys-bits-passthrough3.err
> b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough3.err
> new file mode 100644
> index 00..28f2e43432
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough3.err
> @@ -0,0 +1 @@
> +unsupported configuration: CPU maximum physical address bits number
> specification cannot be used with mode='passthrough'
> diff --git a/tests/qemuxml2argvdata/cpu-phys-bits-passthrough3.xml
> b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough3.xml
> new file mode 100644
> index 00..a94e567dcb
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/cpu-phys-bits-passthrough3.xml
> @@ -0,0 +1,20 @@
> +
> +  foo
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219136
> +  219136
> +  1
> +  
> +hvm
> +
> +  
> +  
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +  
> +
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index c5a0095e0d..fd17fea744 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3409,6 +3409,13 @@ mymain(void)
>
>  DO_TEST_CAPS_LATEST("virtio-9p-multidevs");
>
> +DO_TEST("cpu-phys-bits-passthrough", QEMU_CAPS_KVM,
> QEMU_CAPS_CPU_PHYS_BITS);
> +DO_TEST("cpu-phys-bits-emulate", QEMU_CAPS_KVM,
> QEMU_CAPS_CPU_PHYS_BITS);
> +DO_TEST("cpu-phys-bits-emulate2", QEMU_CAPS_KVM,
> QEMU_CAPS_CPU_PHYS_BITS);
> +DO_TEST_PARSE_ERROR("cpu-phys-bits-emulate3", QEMU_CAPS_KVM);
> +DO_TEST_PARSE_ERROR("cpu-phys-bits-passthrough2", QEMU_CAPS_KVM);
> +DO_TEST_PARSE_ERROR("cpu-phys-bits-passthrough3", QEMU_CAPS_KVM);
> +
>  if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
>  virFileDeleteTree(fakerootdir);
>
>
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: Allow /usr/libexec for libxl-save-helper and pygrub

2020-09-24 Thread Christian Ehrhardt

On Wed, Sep 23, 2020 at 6:45 PM Jim Fehlig  wrote:
>
> On 9/23/20 7:51 AM, Jim Fehlig wrote:
> > On 9/23/20 7:26 AM, Christian Ehrhardt wrote:
> >> On Wed, Sep 23, 2020 at 12:35 AM Jim Fehlig  wrote:
> >>>
> >>> Like other distros, openSUSE Tumbleweed recently changed libexecdir from
> >>> /usr/lib to /usr/libexec. Add it as an allowed path for libxl-save-helper
> >>> and pygrub.
> >>
> >> Hi Jim,
> >> ack to the intention, but I think since this should use @libexecdir@ I 
> >> think.
> >> Or did anything change that this doesn't apply anymore ... in that
> >> case I beg your pardon.
> >>
> >> [1]:
> >> https://libvirt.org/git/?p=libvirt.git;a=commit;h=5c8bd31c881e99261ac098e867a79b300440731a
> >>
> >
> > Heh, I see that skipped over the xen stuff :-). I'll send a V2 later.
>
> Thinking about it more, perhaps it is best to go with this V1 patch since 
> these
> are not files provided by libvirt but xen, where conceivably libvirt and xen
> could be built with different libexecdir? IMO it would be best to explicitly
> list the known paths distros have used for libxl-save-helper and pygrub.

You are right, and in >99% of the cases it will be one of the two
paths you have in your rule anyway.
Sorry for the detour Jim

Reviewed-by: Christian Ehrhardt 


> Regards,
> Jim
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: Allow /usr/libexec for libxl-save-helper and pygrub

2020-09-23 Thread Christian Ehrhardt

On Wed, Sep 23, 2020 at 12:35 AM Jim Fehlig  wrote:
>
> Like other distros, openSUSE Tumbleweed recently changed libexecdir from
> /usr/lib to /usr/libexec. Add it as an allowed path for libxl-save-helper
> and pygrub.

Hi Jim,
ack to the intention, but I think since this should use @libexecdir@ I think.
Or did anything change that this doesn't apply anymore ... in that
case I beg your pardon.

[1]: 
https://libvirt.org/git/?p=libvirt.git;a=commit;h=5c8bd31c881e99261ac098e867a79b300440731a

> Signed-off-by: Jim Fehlig 
> ---
>
> I considered including /usr/lib64, but I don't think any distros are
> installing xen libexecdir targets to /usr/lib64. Happy to include it
> if I'm wrong :-).
>
>  src/security/apparmor/usr.sbin.libvirtd.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
> b/src/security/apparmor/usr.sbin.libvirtd.in
> index f2030764cd..bf4563e1e8 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -86,8 +86,8 @@ profile libvirtd @sbindir@/libvirtd 
> flags=(attach_disconnected) {
>/{usr/,}lib/udev/scsi_id PUx,
>/usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
>/usr/{lib,lib64}/xen/bin/* Ux,
> -  /usr/lib/xen-*/bin/libxl-save-helper PUx,
> -  /usr/lib/xen-*/bin/pygrub PUx,
> +  /usr/{lib,libexec}/xen-*/bin/libxl-save-helper PUx,
> +  /usr/{lib,libexec}/xen-*/bin/pygrub PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
>
> --
> 2.28.0
>
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: Various issues when using multiple graphic outputs

2020-09-07 Thread Christian Ehrhardt

On Fri, Sep 4, 2020 at 1:30 PM Gerd Hoffmann  wrote:
>
>   Hi,
>
> > Well my initial target before I realized that it seemed to affect all
> > combinations in different ways was to get a mediated device to work well.
> > So If the answer is "just use only one" then I need to find a way to 
> > convince
> > libvirt to -not- re-add a graphics device when the mdev of the vgpu is
> > present.
>
> model='none'.

Arr that was it - and as always things are always easier to find once
you know what you look for :-)
It is quite literally at https://libvirt.org/formatdomain.html#video-devices

"This legacy behaviour can be inconvenient in cases where GPU mediated
devices are meant to be the only rendering device within a guest and
so specifying another video device along with type none."

Thank you Gerd and Daniel!

> HTH,
>   Gerd
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: Various issues when using multiple graphic outputs

2020-09-04 Thread Christian Ehrhardt

On Fri, Sep 4, 2020 at 12:37 PM Gerd Hoffmann  wrote:
>
> On Fri, Sep 04, 2020 at 12:05:08PM +0200, Christian Ehrhardt wrote:
> > Hi,
> > I've had continuous issues with this and wanted to reach out
> > if that is a common issue everyone has or just me lacking a little
> > detail on my setup.
>
> > - QXL with multiple heads
> >   - works fine (so multi display in general seems fine for the
> > involved components)
>
> Yep.
>
> > - VGA + QXL, QXL + Virtio, Virtio + QXL:
> >   - Guest only detects primary display
> >   - Can't convince X in the guest to use the second device as well
> >   - lspci lists both devices just fine
> >   - dmesg shows the kernel is initializing them, e.g. drm for virtio
>
> As far I know xorg can't really deal with multiple display devices.
> So typically you have a single device with multiple connectors, so
> you can hook up multiple monitors.
>
> In the virtual world qxl and virtio support multiple heads, so pick
> one of these two and enable as many heads as you need.

Well my initial target before I realized that it seemed to affect all
combinations in different ways was to get a mediated device to work well.
So If the answer is "just use only one" then I need to find a way to convince
libvirt to -not- re-add a graphics device when the mdev of the vgpu is
present.

> Also make sure
> spice-agent is active in the guest, otherwise operating the mouse is a
> PITA.

I think it was active but surely will give it a second look to be sure
- thank you!

> Dunno how things are with wayland.
>
> > - 2*QXL
> >   - X has issues to initialize on user login
> >   - no error in the log, just hung
>
> The windows guest driver doesn't support qxl devices with multiple
> outputs and expects one qxl device per head instead.  So use this
> one for windows guests.
>
> HTH,
>   Gerd
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Various issues when using multiple graphic outputs

2020-09-04 Thread Christian Ehrhardt

Hi,
I've had continuous issues with this and wanted to reach out
if that is a common issue everyone has or just me lacking a little
detail on my setup.

Setup:
- tried qemu up to 4.2
- tried libvirt up to 6.0
- virt-viewer up to 7.0-2build1
- virt-manager up to 2.2.1
- I plan to retry with qemu 5.0, libvirt 6.6 and virt-viewer 9.0 but I
  don't have all pieces ready yet.
- get a Desktop Guest (I've seen it with Ubuntu and Windows, therefore
  I'm rather sure it will affect all desktops)
- configure your guest-xml to use multiple graphic adapters and start it
- connect with virt-viewer to your guest which will open both screens

The exact issue depends on the kind of graphic device I configure.
The following list is ordered in terms of increasing confusion :-):

- QXL with multiple heads
  - works fine (so multi display in general seems fine for the
involved components)
- VGA + QXL, QXL + Virtio, Virtio + QXL:
  - Guest only detects primary display
  - Can't convince X in the guest to use the second device as well
  - lspci lists both devices just fine
  - dmesg shows the kernel is initializing them, e.g. drm for virtio
- 2*QXL
  - X has issues to initialize on user login
  - no error in the log, just hung
- QXL + Mdev
  - mouse handling seems broken
- in gnome I see no mouse pointer at all
- in windows it is just strange
  - has offsets relative to mouse pointer entering
  - and can't move in one random direction (e.g. not up)
- keyboard works just fine
  - I have played with adding/removing different input devices without
much success.
  - I disabled the qxl display via xorg conf without the case getting any better

The last case is what I initially tried and totally confused me at first.

And to be clear, each of those graphics adapter types works fine if being
attached "alone" on the guest (Except mdev alone, as I'm unable to convince
libvirt to only attach the mdev without adding back a qxl graphics adapter).

The Guest XML is the default that virt-manager gives me plus adding the second
graphics adapter (example 2*QXL: https://paste.ubuntu.com/p/cVw8GVZ9dD/).

If this is known in some way or even "yeah try version XY of component Foo" I'm
happy about any hint/pointer you can give me. But if there is a chance that I'm
the only one seeing it I'd like to understand why.

Thanks in advance for thinking into this with me,
Christian

P.S. to avoid cross-posting I'll start with libvirt as it is in the
middle of all involved components.

Re: [PATCH] storage: only fallocate when allocation matches capacity

2020-09-03 Thread Christian Ehrhardt

On Thu, Sep 3, 2020 at 12:36 PM Daniel P. Berrangé  wrote:
>
> On Thu, Sep 03, 2020 at 12:18:42PM +0200, Christian Ehrhardt wrote:
> > On Wed, Sep 2, 2020 at 6:49 PM Michal Privoznik  wrote:
> > >
> > > On 9/2/20 3:58 PM, Christian Ehrhardt wrote:
> > > > In c9ec7088 "storage: extend preallocation flags support for qemu-img"
> > > > the option to fallocate was added and meant to be active when (quote):
> > > > "the XML described storage  matches its "
> > > >
> > > > Up until recently 81a3042a12 "storage_util: fix qemu-img sparse 
> > > > allocation"
> > > > the compared allocation size was an order of magnitude too small, but 
> > > > still
> > > > it does use fallocate too often unless capacity>allocation.
> > > >
> > > > This change fixes the comparison to match the intended description
> > > > of the feature.
> > > >
> > > > Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9
> > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759454
> > > > Fixes: 
> > > > https://bugs.launchpad.net/ubuntu/focal/+source/libvirt/+bug/1847105
> > > >
> > > > Signed-off-by: Christian Ehrhardt 
> > > > ---
> > > >   src/storage/storage_util.c | 6 +++---
> > > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > Reviewed-by: Michal Privoznik 
> > >
> > > And sorry for making the mess earlier (~2 years ago).
> >
> > no problem - it turned out to be even more confusing.
> >
> > Due to some further testing and encouraged by feedback in the same
> > direction by Richard Lager (on CC now) I realized that while the
> > suggested change reads correct it will still not help my case :-/
> >
> > Even if my fix lands, we are back to square one and would need
> > virt-manager to submit a different XML.
> > Remember: my target here would be to come back to pralloca=metadata as
> > it was before for image creations from virt-manager.
> > I've started that aspect of the discussion at the BZ [1] already.
> >
> > On the libvirt side allocation>capacity sounds like being wrong anyway.
>
> It is a bit wierd as an input XML from a mgmt app. It is to be expected
> as an output XML from libvirt though. Some filesystems, notably XFS,
> will sometimes speculatively over-allocate data extents in the belief
> that further size-extending writes will probably arrive. So you can end
> up with allocated blocks being greater than the current logical file
> size.
>
> > And if that is so we have these possible conditions:
> > - capacity==allocation now and before my change falloc
> > - capacity>allocation now and before my change metadata
> > - capacity > (but this one seems invalid anyway)
> >
> > So I wonder are we really back at me asking Cole to let virt-manager
> > request things differently which is how this started about a year ago?
> > Or was I wrong trying to make the code to match the wording in the
> > commit that added it and do we actually want it to behave differently
> > (read no falloc) for the XMLs sent by virt-manager as of today?
>
> I think we should provide three flags
>
>   VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA
>   VIR_STORAGE_VOL_CREATE_PREALLOC_NONE
>   VIR_STORAGE_VOL_CREATE_PREALLOC_PAYLOAD
>
> as a way to get explicit behaviour, with those flags ignoring the
> "allocation" field. Only look at "allocation" if none of the flags
> are given for sake of back compat.

Independent to the issue I initially wanted to solve and to the other
discussion on
why fallocate on ZFS makes barely any sense I like the suggestion Daniel.

I think the past try to implicitly derive from allocation size has
proven to be misleading at best.

> Regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] storage: only fallocate when allocation matches capacity

2020-09-03 Thread Christian Ehrhardt

On Thu, Sep 3, 2020 at 12:49 PM Richard Laager  wrote:
>
> On 9/3/20 5:18 AM, Christian Ehrhardt wrote:
> > Even if my fix lands, we are back to square one and would need
> > virt-manager to submit a different XML.
> > Remember: my target here would be to come back to pralloca=metadata as
> > it was before for image creations from virt-manager.
>
> Why is that your goal?
>
> If this is simply because OpenZFS doesn't support fallocate(mode=0),

Yeah it was because behavior changed for users on upgrades and became
slow (if on ZFS).
And since the symptom was restricted to just that and also just a
slowdown the prio was always low anyway.

> that has (finally!) been resolved for the next release:
> https://github.com/openzfs/zfs/commit/f734301d2267cbb33eaffbca195fc93f1dae7b74

Glad to hear that - and I agree that fallocate @ COW/Compression FS is
not really applicable.

So it seems "my need" for this is completely gone once we get the change above.
Thanks Richard for the FYI on it!

> ZFS will "fake" the fallocate() request. It'll check to make sure
> there's enough free space at the moment, which is about all it can do
> anyway. It can't reserve the space anyway, mostly because it is a
> copy-on-write filesystem. Even if the application writes zeros, ZFS will
> just throw them away anyway (assuming you are using compression, which
> everyone should be).
>
> > On the libvirt side allocation>capacity sounds like being wrong anyway.
> > And if that is so we have these possible conditions:
> > - capacity==allocation now and before my change falloc
> > - capacity>allocation now and before my change metadata
> > - capacity > (but this one seems invalid anyway)
> >
> > So I wonder are we really back at me asking Cole to let virt-manager
> > request things differently which is how this started about a year ago?
>
> Setting aside cases of semi-allocation (capacity > allocation != 0) and
> overprovisioning (allocation > capacity), I assume the common cases are
> thin provisioning (allocation == 0) and thick provisioning (capacity ==
> allocation).
>
> virt-manager (at least in the way I use it) asks explicitly for the
> allocation and capacity. If virt-manager is properly conveying (and I'd
> assume it is) the user's capacity and allocation choices from the GUI to
> libvirt, then virt-manager is working correctly in my view and should be
> left alone.
>
> I believe the main goal for thick provisioning is to reserve the space
> as best as possible, because ENOSPC underneath a virtual machine is bad.
> Secondary goals would be allocating the space relatively contiguously
> for performance and accounting for the space immediately to help the
> administrator keep track of usage.
>
> If the filesystem supports fallocate(), using it accomplishes all of
> these goals in a very performant way. If the filesystem does not support
> fallocate(), then the application can either write zeros or do nothing.
> Writing zeros is slow, but achieves the goals to the extent possible.
> Not writing zeros is fast, but does not reserve/account for the space;
> though, depending on the filesystem, that might not be possible anyway.
>
> I think the question fundamentally comes down to: how strong do you take
> a "thick provisioning" request? Do you do everything in your power to
> achieve it (which would mean writing zeros*) or do you treat it as a
> hint that you'll only follow if it is fast to do so?
>
> If it's a demand, then try fallocate() but fall back to writing zeroes.
> (glibc's posix_fallocate() does exactly this.). If it's a hint, then
> only ever call fallocate().
>
> I think it is reasonable to treat it as a demand and write zeros if
> fallocate() fails. If it is too slow, the admin will notice and can make
> the decision to (in the future) stop requesting thick provisioning and
> just request thin provisioning.
>
> In the ZFS case, why is the admin requesting thick provisioning anyway?
>
>
> * One could go further and defeat compression by writing random data.
>   But that seems extreme, so I'm going to ignore that.
>
> --
> Richard



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] storage: only fallocate when allocation matches capacity

2020-09-03 Thread Christian Ehrhardt

On Wed, Sep 2, 2020 at 6:49 PM Michal Privoznik  wrote:
>
> On 9/2/20 3:58 PM, Christian Ehrhardt wrote:
> > In c9ec7088 "storage: extend preallocation flags support for qemu-img"
> > the option to fallocate was added and meant to be active when (quote):
> > "the XML described storage  matches its "
> >
> > Up until recently 81a3042a12 "storage_util: fix qemu-img sparse allocation"
> > the compared allocation size was an order of magnitude too small, but still
> > it does use fallocate too often unless capacity>allocation.
> >
> > This change fixes the comparison to match the intended description
> > of the feature.
> >
> > Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759454
> > Fixes: https://bugs.launchpad.net/ubuntu/focal/+source/libvirt/+bug/1847105
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >   src/storage/storage_util.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
>
> Reviewed-by: Michal Privoznik 
>
> And sorry for making the mess earlier (~2 years ago).

no problem - it turned out to be even more confusing.

Due to some further testing and encouraged by feedback in the same
direction by Richard Lager (on CC now) I realized that while the
suggested change reads correct it will still not help my case :-/

Even if my fix lands, we are back to square one and would need
virt-manager to submit a different XML.
Remember: my target here would be to come back to pralloca=metadata as
it was before for image creations from virt-manager.
I've started that aspect of the discussion at the BZ [1] already.

On the libvirt side allocation>capacity sounds like being wrong anyway.
And if that is so we have these possible conditions:
- capacity==allocation now and before my change falloc
- capacity>allocation now and before my change metadata
- capacityhttps://bugzilla.redhat.com/show_bug.cgi?id=1759454


> Michal
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[PATCH] storage: only fallocate when allocation matches capacity

2020-09-02 Thread Christian Ehrhardt

In c9ec7088 "storage: extend preallocation flags support for qemu-img"
the option to fallocate was added and meant to be active when (quote):
"the XML described storage  matches its "

Up until recently 81a3042a12 "storage_util: fix qemu-img sparse allocation"
the compared allocation size was an order of magnitude too small, but still
it does use fallocate too often unless capacity>allocation.

This change fixes the comparison to match the intended description
of the feature.

Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759454
Fixes: https://bugs.launchpad.net/ubuntu/focal/+source/libvirt/+bug/1847105

Signed-off-by: Christian Ehrhardt 
---
 src/storage/storage_util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index cf82ea0a87..85bed76863 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -710,10 +710,10 @@ 
storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr encinfo,
 virQEMUBuildQemuImgKeySecretOpts(, encinfo, info->secretAlias);
 
 if (info->preallocate) {
-if (info->size_arg > info->allocation)
-virBufferAddLit(, "preallocation=metadata,");
-else
+if (info->size_arg == info->allocation)
 virBufferAddLit(, "preallocation=falloc,");
+else
+virBufferAddLit(, "preallocation=metadata,");
 }
 
 if (info->nocow)
-- 
2.28.0

Re: [PATCH v2] virt-aa-helper: disallow graphics socket read permissions

2020-09-01 Thread Christian Ehrhardt

On Thu, May 28, 2020 at 12:45 PM Simon Arlott  wrote:
>
> The VM does not need read permission for its own sockets to create,
> bind(), listen(), accept() connections or to recv(), send(), etc. on
> those connections.
>
> This was fixed in ab9569e5460d1e4737fe8b625c67687dc2204665
> (virt-aa-helper: disallow VNC socket read permissions),
> but then b6465e1aa49397367a9cd0f27110b9c2280a7385
> (graphics: introduce new listen type 'socket')
> and acc83afe333bfadd3f7f79091d38ca3d7da1eeb2
> (acc83afe333bfadd3f7f79091d38ca3d7da1eeb2) reverted it.
>
> Unless the read permission is omitted, VMs can connect to each other's
> VNC/graphics sockets.

As only the defined path from the XML is allowed that would only apply
if one specified the same path in each VM definition right?
That would be borderline to "intentionally configured that way".

> Signed-off-by: Simon Arlott 
> ---
> Updated version that changes the test case too.
>
>  src/security/virt-aa-helper.c | 2 +-
>  tests/virt-aa-helper-test | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 6e6dd1b1db..fddbdafc41 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
[...]

>   type='address' address='0.0.0.0'/>

Isn't that option above converted to "-vnc unix:path"?
Hmm, no as ./tests/qemuxml2argvdata/graphics-vnc-remove-generated-socket.args
shows it is actually removed later.

But the attribute that virt-aa-helper parses in the lines you touched
is used for good cases as well, cases like these:
./tests/qemuxml2argvdata/graphics-vnc-socket-new-cmdline.args
./tests/qemuxml2argvdata/graphics-vnc-socket.args

In which case qemu would need to write to it as it is meant to [1]:
"... Rather than using listen/port, QEMU supports a socket attribute
for listening on a unix domain socket path Since 0.8.8 . ..."

An example would be:
./tests/qemuxml2argvdata/graphics-vnc-socket.xml-22-
./tests/qemuxml2argvdata/graphics-vnc-socket.xml:23:  
./tests/qemuxml2argvdata/graphics-vnc-socket.xml-24-
=>
./tests/qemuxml2argvdata/graphics-vnc-socket.args:27:-vnc unix:/tmp/vnc.sock \

And if I bluntly try that there are cases where qemu then creates that socket:
  $ qemu-system-x86_64 -vnc socket:/tmp/foobar
creates:
  srwxrwxr-x 1 paelzer paelzer 0 Sep  1 11:43 /tmp/foobar=

Therefore qemu would need the write permission to that value IMHO.
And as I said the concern of "VMs can connect to each other" would
only be true if the admin specifies the same path in each of them
intentionally.


[1]: https://libvirt.org/formatdomain.html#graphical-framebuffers

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[RFC] apparmor: add subprofile for virtiofsd

2020-08-26 Thread Christian Ehrhardt

This is a continuation of
https://www.redhat.com/archives/libvir-list/2020-August/msg00804.html
https://www.redhat.com/archives/libvir-list/2020-August/msg00922.html

It still has too many weak points left, but should be great as an RFC
already. virtiofsd works for me using that profile, but we need to:
- agree on common paths to expect for virtiofsd
- get the post pivot_root rules under control

---

virtiofsd runs as root and is reachable from the guest, to limit
the exploit potential this adds a apparmor subprofile to virtiofsd
as spawned by libvirt to limit it.

Known TODOs:
- rules after pivot_root need not to allow everything
- settle on common paths with the community

Signed-off-by: Christian Ehrhardt 
---
 src/security/apparmor/libvirt-qemu |  3 ++
 src/security/apparmor/usr.sbin.libvirtd.in | 46 ++
 2 files changed, 49 insertions(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index a03e9e2c94..668fc72f27 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -221,6 +221,9 @@
   unix (send, receive) type=stream addr=none peer=(label=libvirtd),
   unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd),
 
+  # allow to connect to virtiofsd
+  unix (send, receive) type=stream addr=none peer=(label=libvirtd//virtiofsd),
+
   # for gathering information about available host resources
   /sys/devices/system/cpu/ r,
   /sys/devices/system/node/ r,
diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
b/src/security/apparmor/usr.sbin.libvirtd.in
index 4518e8f865..f878398b4b 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -133,4 +133,50 @@ profile libvirtd @sbindir@/libvirtd 
flags=(attach_disconnected) {
 
/usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
   }
+
+  # child profile for virtiofsd helper process
+  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd Cx -> virtiofsd,
+  profile virtiofsd flags=(attach_disconnected) {
+   #include 
+   #include 
+
+   capability sys_admin,
+   capability sys_resource,
+
+   # init phase
+   / r,
+   mount options=(rw, rslave)  -> /,
+   umount /,
+   mount options=(rw, nosuid, nodev, noexec, relatime)  -> @{PROC},
+   owner /proc/sys/fs/file-max r,
+
+   # For communication/control from libvirtd
+   unix (send, receive) type=stream addr=none peer=(label=libvirtd),
+   signal (receive) set=("term") peer=/usr/sbin/libvirtd,
+   signal (receive) set=("term") peer=libvirtd,
+   owner /var/lib/libvirt/qemu/domain-*/fs[0-9]{[0-9],}-fs.pid w,
+   /var/lib/libvirt/qemu/domain-*/fs[0-9]{[0-9],}-fs.sock rw,
+   /var/lib/libvirt/qemu/ram/*/ram-node[0-9]{[0-9],} rw,
+
+   # For communication with confined and unconfined guests
+   unix (send, receive) type=stream addr=none 
peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*),
+   unix (send, receive) type=stream addr=none peer=(label=unconfined),
+
+   /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd rmix,
+
+   # Common host paths to share from are allowed by default
+   # Further paths should be added as local override
+   # TODO - community to settle on a list of common paths to allow
+   owner /var/lib/libvirt/virtiofsd/*/ r,
+   mount options=(rw, bind)  -> /var/lib/libvirt/virtiofsd/*/,
+   pivot_root /var/lib/libvirt/virtiofsd/*/,
+
+   # TODO - after pivot_root the rules for the actual file access by the guest
+   # through virtiofsd would need to start with / which is too open
+   /** rw,
+
+   # Site-specific additions and overrides. See local/README for details.
+   #include 
+  }
+
 }
-- 
2.28.0

Re: [PATCH v2] apparmor: allow libvirtd to call virtiofsd

2020-08-26 Thread Christian Ehrhardt

On Tue, Aug 25, 2020 at 3:31 PM Kevin Locke  wrote:
>
> When using [virtiofs], libvirtd must launch [virtiofsd] to provide
> filesystem access on the host.  When a guest is configured with
> virtiofs, such as:
>
> 
>   
>   
>   
> 
>
> Attempting to start the guest fails with:
>
> internal error: virtiofsd died unexpectedly
>
> /var/log/libvirt/qemu/$name-fs0-virtiofsd.log contains (as a single
> line, wrapped below):
>
> libvirt:  error : cannot execute binary /usr/lib/qemu/virtiofsd:
> Permission denied
>
> dmesg contains (as a single line, wrapped below):
>
> audit: type=1400 audit(1598229295.959:73): apparmor="DENIED"
> operation="exec" profile="libvirtd" name="/usr/lib/qemu/virtiofsd"
> pid=46007 comm="rpc-worker" requested_mask="x" denied_mask="x"
> fsuid=0 ouid=0
>
> To avoid this, allow execution of virtiofsd from the libvirtd AppArmor
> profile.
>
> [virtiofs]: https://libvirt.org/kbase/virtiofs.html
> [virtiofsd]: https://www.qemu.org/docs/master/interop/virtiofsd.html
>
> Signed-off-by: Kevin Locke 
> Reviewed-by: Christian Ehrhardt 

Thank you Kevin for the v2!
I've now also had the chance to test it and can confirm the reported issues
as well as the change fixing it.
With review and test in place I'll commit this apparmor change before
the 6.7.0 freeze happens.

But long term we should think about adding a profile for virtiofsd itself.
I have started some work but it is yet imperfect, it has open TODOs.
I'll reply with a RFC patch to this mail how that sub-profile could look
like and hope for a good discussion there from everyone.

In that RFC are questions for everyone (expected paths to agree on) as
well as apparmor specialists (I hope for Jamie) around pivot_root.

@Kevin - if you want you could continue your experiments with that
subprofile and let me know of the rough bumps that you find with it.

> ---
>
> Changes in v2:
> - Wrap log and dmesg messages, as requested by Christian Ehrhardt.
>
>  src/security/apparmor/usr.sbin.libvirtd.in | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
> b/src/security/apparmor/usr.sbin.libvirtd.in
> index 4518e8f865..f2030764cd 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -89,6 +89,7 @@ profile libvirtd @sbindir@/libvirtd 
> flags=(attach_disconnected) {
>/usr/lib/xen-*/bin/libxl-save-helper PUx,
>/usr/lib/xen-*/bin/pygrub PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> +  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
>
># Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
># read and run an ebtables script.
> --
> 2.28.0
>

--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: XDR related breakage in libvirt v6.6.0 when using libvirt-lxc

2020-08-25 Thread Christian Ehrhardt

On Tue, Aug 25, 2020 at 4:07 PM Daniel P. Berrangé  wrote:
>
> On Tue, Aug 25, 2020 at 03:16:50PM +0200, Christian Ehrhardt wrote:
> > Hi,
> > I expect that this falls under the "with meson now everything is
> > different anyway" umbrella but wanted to let you know about this as it
> > affects v6.6 in at least Ubuntu/Debian.
> >
> > The following recent patch has broken libvirt-lxc for us:
> > commit d7147b3797380de2d159ce6324536f3e1f2d97e3
> > Author: Pavel Hrdina 
> > Date: Fri Jun 19 00:44:07 2020 +0200
> > m4: virt-xdr: rewrite XDR check
> >
> > I was tracking that down for [1] since the tests [4] failed on me. [2]
> > holds the backtrace.
> > In Debian the tests are skipped which explains why they were not seen there:
> >   smoke-lxc SKIP Test requires machine-level isolation but testbed
> > does not provide that
> >
> > What happens is that the libvirt_lxc segfaults when using XDR functions.
> >
> > dmesg shows:
> > [582093.524644] libvirt_lxc[261446]: segfault at 0 ip 
> > sp 7ffdd2345598 error 14 in libvirt_lxc[5587e42aa000+8000]
> > [582093.524650] Code: Bad RIP value.
> >
> > There are quite some uncertainties left, but on the surface it seems
> > that it links with libtirpc but
> > then instead of calling
> > libtirpc: src/xdr.c:929:xdr_uint64_t(xdrs, ullp)
> > it ends (gdb tells us in [2]) in glibc
> > glibc: sunrpc/xdr_intXX_t.c:62:xdr_uint64_t (XDR *xdrs, uint64_t *uip)
> >
> > And the return from that function breaks it badly (instruction pointer
> > at 0x0 -> segfault)
>
> Right so that's a serious problem with clashing symbols between tirpc
> and glibc.
>
> In Fedora/RHEL it is impossible to build against glibc for the XDR
> symbols for a long time now. Glibc maintainers want everyone to be
> using tirpc.   The symbols are still exported from glibc, but they
> should only be used by legacy apps built against older glibc.
>
> Symbol versioning should ensure libvirt_lxc always resolves to the
> libtirpc library
>
> $ eu-readelf -a /usr/lib64/libc.so.6 | grep xdr_uint64 | grep GLOBAL
>  2017: 001349c0226 FUNCGLOBAL DEFAULT   15 
> xdr_uint64_t@GLIBC_2.2.5
>
>
> $ eu-readelf -a /usr/lib64/libtirpc.so | grep xdr_uint64 | grep GLOBAL
>   344: 0001ce20  9 FUNCGLOBAL DEFAULT   14 
> xdr_uint64_t@@TIRPC_0.3.0
>
> $ eu-readelf -a /usr/libexec/libvirt_lxc  | grep xdr_uint64
>   0x00024a30  X86_64_JUMP_SLOT 00  +0 xdr_uint64_t
>   149:   0 FUNCGLOBAL DEFAULTUNDEF 
> xdr_uint64_t@TIRPC_0.3.0 (13)

ubuntu@groovy:~$ eu-readelf -a /lib/x86_64-linux-gnu/libc.so.6 | grep
xdr_uint64 | grep GLOBAL
 2019: 00159ed0228 FUNCGLOBAL DEFAULT   16
xdr_uint64_t@@GLIBC_2.2.5
ubuntu@groovy:~$ eu-readelf -a /lib/x86_64-linux-gnu/libtirpc.so.3.0.0
| grep xdr_uint64 | grep GLOBAL
  343: 0001ae20  9 FUNCGLOBAL DEFAULT   15
xdr_uint64_t@@TIRPC_0.3.0

Ubuntu v6.0 builds
ubuntu@groovy:~$ eu-readelf -a /usr/lib/libvirt/libvirt_lxc  | grep xdr_uint64
  0x00026820  X86_64_JUMP_SLOT 00  +0 xdr_uint64_t
   99:   0 FUNCGLOBAL DEFAULTUNDEF
xdr_uint64_t@GLIBC_2.2.5 (4)
  [  1c02]  xdr_uint64_t

Ubuntu v6.6 builds
ubuntu@groovy:~$ eu-readelf -a /usr/lib/libvirt/libvirt_lxc  | grep xdr_uint64
  0x000268d0  X86_64_JUMP_SLOT 00  +0 xdr_uint64_t
  104:   0 FUNCGLOBAL DEFAULTUNDEF
xdr_uint64_t@GLIBC_2.2.5 (4)
  [  1a81]  xdr_uint64_t

They miss the version 3.0 entry - interesting.

libvirt 6.6 build from git on the same system:
$ eu-readelf -a libvirt/build/src/.libs/libvirt_lxc  | grep xdr_uint64
  0x00028968  X86_64_JUMP_SLOT 00  +0 xdr_uint64_t
   99:   0 FUNCGLOBAL DEFAULTUNDEF
xdr_uint64_t@GLIBC_2.2.5 (3)
  598:   0 FUNCGLOBAL DEFAULTUNDEF
xdr_uint64_t@@GLIBC_2.2.5
  [  31df]  xdr_uint64_t@@GLIBC_2.2.5
  [  18f4]  xdr_uint64_t

That is with
configure:  xdr: yes (CFLAGS='-I/usr/include/tirpc'
LIBS='-ltirpc')

So something is wrong at build time when glibc AND tirpc provide that symbol.

>
> This shows libvirt_lxc will only resolve to libtirpc.
>
>
> I see the Ubuntu package for glibc is passing --enable-obsolete-rpc  which
> allows apps to continue to build against glibc for RPC :-(
>
> So I suspect somehow libvirt has ended up using tirpc headers, but the linker
> probably resolved symbols to glibc.

As I wrote above my builds don't get the 3.0 entry in libvirt_lxc
which seems to be the reason to then jump to the wrong one.

> I don't know how the

Re: [PATCH] apparmor: allow libvirtd to call virtiofsd

2020-08-25 Thread Christian Ehrhardt

On Mon, Aug 24, 2020 at 2:21 PM Christian Ehrhardt
 wrote:
>
> On Mon, Aug 24, 2020 at 2:03 PM Kevin Locke  wrote:
> >
> > When using [virtiofs], libvirtd must launch [virtiofsd] to provide
> > filesystem access on the host.  When a guest is configured with
> > virtiofs, such as:
> >
> > 
> >   
> >   
> >   
> > 
> >
> > Attempting to start the guest fails with:
> >
> > internal error: virtiofsd died unexpectedly
> >
> > /var/log/libvirt/qemu/$name-fs0-virtiofsd.log contains:
> >
> > libvirt:  error : cannot execute binary /usr/lib/qemu/virtiofsd: 
> > Permission denied
> >
> > dmesg contains:
> >
> > audit: type=1400 audit(1598229295.959:73): apparmor="DENIED" 
> > operation="exec" profile="libvirtd" name="/usr/lib/qemu/virtiofsd" 
> > pid=46007 comm="rpc-worker" requested_mask="x" denied_mask="x" fsuid=0 
> > ouid=0

I was prepping to commit this sometime soon and for my own testing -
while doing so I realized this line is very long.
While https://libvirt.org/submitting-patches.html doesn't mention a
limit it is generally useful to wrap at 72 or at least 80 chars.
This can be done by the committer, but obviously is less work for
everyone if wrapped from the start.

> >
> > To avoid this, allow execution of virtiofsd from the libvirtd AppArmor
> > profile.
> >
> > [virtiofs]: https://libvirt.org/kbase/virtiofs.html
> > [virtiofsd]: https://www.qemu.org/docs/master/interop/virtiofsd.html
>
> The added rule and reasoning LGTM,
> Reviewed-by: Christian Ehrhardt 
>
> P.S. I'm also adding Jamie for his extra depth on apparmor topics.
>
> > Signed-off-by: Kevin Locke 
> > ---
> >  src/security/apparmor/usr.sbin.libvirtd.in | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
> > b/src/security/apparmor/usr.sbin.libvirtd.in
> > index 4518e8f865..f2030764cd 100644
> > --- a/src/security/apparmor/usr.sbin.libvirtd.in
> > +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> > @@ -89,6 +89,7 @@ profile libvirtd @sbindir@/libvirtd 
> > flags=(attach_disconnected) {
> >/usr/lib/xen-*/bin/libxl-save-helper PUx,
> >/usr/lib/xen-*/bin/pygrub PUx,
> >/usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> > +  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
> >
> ># Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
> ># read and run an ebtables script.
> > --
> > 2.28.0
> >
>
>
> --
> Christian Ehrhardt
> Staff Engineer, Ubuntu Server
> Canonical Ltd



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

XDR related breakage in libvirt v6.6.0 when using libvirt-lxc

2020-08-25 Thread Christian Ehrhardt

Hi,
I expect that this falls under the "with meson now everything is
different anyway" umbrella but wanted to let you know about this as it
affects v6.6 in at least Ubuntu/Debian.

The following recent patch has broken libvirt-lxc for us:
commit d7147b3797380de2d159ce6324536f3e1f2d97e3
Author: Pavel Hrdina 
Date: Fri Jun 19 00:44:07 2020 +0200
m4: virt-xdr: rewrite XDR check

I was tracking that down for [1] since the tests [4] failed on me. [2]
holds the backtrace.
In Debian the tests are skipped which explains why they were not seen there:
  smoke-lxc SKIP Test requires machine-level isolation but testbed
does not provide that

What happens is that the libvirt_lxc segfaults when using XDR functions.

dmesg shows:
[582093.524644] libvirt_lxc[261446]: segfault at 0 ip 
sp 7ffdd2345598 error 14 in libvirt_lxc[5587e42aa000+8000]
[582093.524650] Code: Bad RIP value.

There are quite some uncertainties left, but on the surface it seems
that it links with libtirpc but
then instead of calling
libtirpc: src/xdr.c:929:xdr_uint64_t(xdrs, ullp)
it ends (gdb tells us in [2]) in glibc
glibc: sunrpc/xdr_intXX_t.c:62:xdr_uint64_t (XDR *xdrs, uint64_t *uip)

And the return from that function breaks it badly (instruction pointer
at 0x0 -> segfault)

Bisecting pointed to the referred commit which brings libtirpc into the mix.
The former builds had xdr detected, but not with libtirpc.
configure: xdr: yes (CFLAGS='' LIBS='')
The new config now does
configure: xdr: yes (CFLAGS='-I/usr/include/tirpc' LIBS='-ltirpc')

And the resulting libvirt_lxc reflects that

v6.0.0
$ lddtree /usr/lib/libvirt/libvirt_lxc | grep tirpc
v6.6.0
$ lddtree /usr/lib/libvirt/libvirt_lxc | grep tirpc
libtirpc.so.3 => /lib/x86_64-linux-gnu/libtirpc.so.3

This seems to lead to the bad jump and the crash eventually.
Meanwhile reverting d7147b37 "m4: virt-xdr: rewrite XDR check" on top
of v6.6.0 resolves the issue back to the former state.

For anyone that wants to recreate this, I also attached a bisect
script [3] which includes the test case you'd need.

[1]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1892826
[2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1892826/comments/4
[3]: 
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1892826/+attachment/5404392/+files/bisect-libvirt.sh
[4]: 
https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-groovy/groovy/amd64/libv/libvirt/20200825_005918_44b74@/log.gz

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: allow libvirtd to call virtiofsd

2020-08-24 Thread Christian Ehrhardt

On Mon, Aug 24, 2020 at 2:03 PM Kevin Locke  wrote:
>
> When using [virtiofs], libvirtd must launch [virtiofsd] to provide
> filesystem access on the host.  When a guest is configured with
> virtiofs, such as:
>
> 
>   
>   
>   
> 
>
> Attempting to start the guest fails with:
>
> internal error: virtiofsd died unexpectedly
>
> /var/log/libvirt/qemu/$name-fs0-virtiofsd.log contains:
>
> libvirt:  error : cannot execute binary /usr/lib/qemu/virtiofsd: 
> Permission denied
>
> dmesg contains:
>
> audit: type=1400 audit(1598229295.959:73): apparmor="DENIED" 
> operation="exec" profile="libvirtd" name="/usr/lib/qemu/virtiofsd" pid=46007 
> comm="rpc-worker" requested_mask="x" denied_mask="x" fsuid=0 ouid=0
>
> To avoid this, allow execution of virtiofsd from the libvirtd AppArmor
> profile.
>
> [virtiofs]: https://libvirt.org/kbase/virtiofs.html
> [virtiofsd]: https://www.qemu.org/docs/master/interop/virtiofsd.html

The added rule and reasoning LGTM,
Reviewed-by: Christian Ehrhardt 

P.S. I'm also adding Jamie for his extra depth on apparmor topics.

> Signed-off-by: Kevin Locke 
> ---
>  src/security/apparmor/usr.sbin.libvirtd.in | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
> b/src/security/apparmor/usr.sbin.libvirtd.in
> index 4518e8f865..f2030764cd 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -89,6 +89,7 @@ profile libvirtd @sbindir@/libvirtd 
> flags=(attach_disconnected) {
>/usr/lib/xen-*/bin/libxl-save-helper PUx,
>/usr/lib/xen-*/bin/pygrub PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> +  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
>
># Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
># read and run an ebtables script.
> --
> 2.28.0
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] tools: fix libvirt-guests.sh text assignments

2020-08-21 Thread Christian Ehrhardt

On Thu, Aug 20, 2020 at 10:50 AM Michael Chapman  wrote:
>
> On Thu, 20 Aug 2020, Christian Ehrhardt wrote:
> > On Wed, Aug 19, 2020 at 12:15 PM Christian Ehrhardt
> >  wrote:
> > >
> > > In libvirt 6.6 stopping guests with libvirt-guests.sh is broken.
> > > As soon as there is more than one guest one can see
> > > `systemctl stop libvirt-guests` faiing and in the log we see:
> > >   libvirt-guests.sh[2455]: Running guests on default URI:
> > >   libvirt-guests.sh[2457]: /usr/lib/libvirt/libvirt-guests.sh: 120:
> > >   local: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae2: bad variable name
> > >   libvirt-guests.sh[2462]: no running guests.
> > >
> > > That is due do mutliple guests becoming a list of UUIDs. Without
> > > recognizing this as one single string the assignment breaks when using 
> > > 'local'
> > > (which was recently added in 6.3.0). This is because local is defined as
> > >   local [option] [name[=value] ... | - ]
> > > which makes the shell trying handle the further part of the string as
> > > variable names. In the error above that string isn't a valid variable
> > > name triggering the issue that is seen.
> > >
> > > To resolve that 'textify' all assignments that are strings or potentially
> > > can become such lists (even if they are not using the local qualifier).
> >
> > Just to illustrate the problem, this never was great and could cause
> > warnings/errors,
> > but amplified due to the 'local' it makes the script break now.
>
> Arguably the big problem here is that 'local' isn't actually specified by
> POSIX, so can not be used in a portable /bin/sh script. (It might end up
> in POSIX eventually, see [1].)
>
> If this were a Bash script, then all of those variable assignments
> (whether they're local or not) would work as expected:
>
> $ echo $BASH_VERSION
> 5.0.17(1)-release
> $ uuid='2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 
> 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3'
> $ foo() { x=$uuid; echo "<$x>"; }
> $ bar() { local x=$uuid; echo "<$x>"; }
> $ foo
> <2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 
> 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>
> $ bar
> <2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 
> 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>

Thanks for these initial tests showing that this is a real issue
(depending on shell being used).

> This works even when Bash is running in POSIX mode.

Dash in POSIX mode (used as /bin/sh interpreter) was what I used to
spot the issue.
Maybe because bash in POSIX mode works is why it wasn't spotted before.


> But POSIX shells that do not support 'local' seem to be rare

Agreed

> , and your
> suggested change would make these assignments work even on shells that do
> not apply special parsing rules for it.

Thanks, I also have pushed this through a bunch of tests that use
libvirt-guests.sh on multiple situations and architectures.
They all worked fine with the fix (and were the ones spotting the
issue in the first place).
So after adding these extra bits of info to the commit message, fixing
a typo in there and running a gitlab CI pipeline on it I'm pushing the
commit.

Thanks Michael for the extra thoughts on this!

>
> [1] https://www.austingroupbugs.net/bug_view_page.php?bug_id=767



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: qemu modularization of qemu-5.1 vs libvirt domcapabilities cache?

2020-08-20 Thread Christian Ehrhardt

On Thu, Aug 20, 2020 at 5:15 PM Mark Mielke  wrote:
>
> On Thu, Aug 20, 2020 at 8:55 AM Christian Ehrhardt 
>  wrote:
>>
>> On Thu, Aug 20, 2020 at 12:43 PM Martin Wilck  wrote:
>> > The simplest approach is to touch the qemu binaries. We discussed this
>> > already. It has the drawback that it makes "rpm -V" complain about
>> > wrong timestamps. It might also confuse backup software. Still, it
>> > might be a viable short-term workaround if nothing else is available.
>>
>> Qemu already allows to save modules in /var/run/qemu/ [1] to better handle
>> module upgrades which is already used in Debian and Ubuntu to avoid
>> late module load errors after upgrades.
>>
>> This was meant for upgrades, but if libvirt would define a known path in
>> there like /var/run/qemu/last_packaging_change the packages could easily
>> touch it on any install/remove/update as Daniel suggested and libvirt could
>> check this path like it does with the date of the qemu binary already.
>>
>> [1]: 
>> https://github.com/qemu/qemu/commit/bd83c861c0628a64997b7bd95c3bcc2e916baf2e
>
>
> Earlier in this thread - I think one or two of us had asked about the 
> timestamp on the directory that contains the modules.
>
> I'm wondering if a "last_packaging_change" provides any value over and above 
> the timestamp of the directory itself? Wouldn't the directory be best - as it 
> would work automatically for both distro packaging as well as custom installs?

Sure, if
- "list of files in module dir"
- "stat dates of files in module dir"
would be checked by libvirt that would also be a no-op for packaging
and thereby land faster.

But I guess there were reasons against it in the discussion before?

> --
> Mark Mielke 
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: qemu modularization of qemu-5.1 vs libvirt domcapabilities cache?

2020-08-20 Thread Christian Ehrhardt

On Thu, Aug 20, 2020 at 12:43 PM Martin Wilck  wrote:
>
> On Thu, 2020-08-20 at 11:00 +0100, Daniel P. BerrangÃ© wrote:
> > On Wed, Aug 05, 2020 at 10:19:26AM +0200, Andrea Bolognani wrote:
> > > contents change.
> > >
> > > That said, if upgrading QEMU results in losing features, even
> > > though
> > > you can recover them through additional steps I would argue that's
> > > a
> > > bug in the packaging that should be addressed on the QEMU side.
> >
> > Potentially we can consider this a  distro packaging problem and fix
> > it at the RPM level.
> >
> > eg the libvirt RPM can define a trigger that runs when *any* of the
> > qemu-device*  RPMs is installed/updated. This trigger can simply
> > touch a file on disk somewhere, and libvirtd can monitor this one
> > file, instead of having to monitor every module.
>
> The simplest approach is to touch the qemu binaries. We discussed this
> already. It has the drawback that it makes "rpm -V" complain about
> wrong timestamps. It might also confuse backup software. Still, it
> might be a viable short-term workaround if nothing else is available.

Qemu already allows to save modules in /var/run/qemu/ [1] to better handle
module upgrades which is already used in Debian and Ubuntu to avoid
late module load errors after upgrades.

This was meant for upgrades, but if libvirt would define a known path in
there like /var/run/qemu/last_packaging_change the packages could easily
touch it on any install/remove/update as Daniel suggested and libvirt could
check this path like it does with the date of the qemu binary already.

[1]: 
https://github.com/qemu/qemu/commit/bd83c861c0628a64997b7bd95c3bcc2e916baf2e


> Cheers,
> Martin
>
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] tools: fix libvirt-guests.sh text assignments

On Wed, Aug 19, 2020 at 12:15 PM Christian Ehrhardt
 wrote:
>
> In libvirt 6.6 stopping guests with libvirt-guests.sh is broken.
> As soon as there is more than one guest one can see
> `systemctl stop libvirt-guests` faiing and in the log we see:
>   libvirt-guests.sh[2455]: Running guests on default URI:
>   libvirt-guests.sh[2457]: /usr/lib/libvirt/libvirt-guests.sh: 120:
>   local: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae2: bad variable name
>   libvirt-guests.sh[2462]: no running guests.
>
> That is due do mutliple guests becoming a list of UUIDs. Without
> recognizing this as one single string the assignment breaks when using 'local'
> (which was recently added in 6.3.0). This is because local is defined as
>   local [option] [name[=value] ... | - ]
> which makes the shell trying handle the further part of the string as
> variable names. In the error above that string isn't a valid variable
> name triggering the issue that is seen.
>
> To resolve that 'textify' all assignments that are strings or potentially
> can become such lists (even if they are not using the local qualifier).

Just to illustrate the problem, this never was great and could cause
warnings/errors,
but amplified due to the 'local' it makes the script break now.

$ cat test.sh
#!/bin/sh

foo () {
f=2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3
echo f
}

bar () {
local b=2a49cb0f-1ff8-44b5-a61d-806b9e52dae2
2a49cb0f-1ff8-44b5-a61d-806b9e52dae3
echo b
}

foo
bar
echo end of script


$ ./test.sh
./test.sh: 4: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3: not found
f
./test.sh: 9: local: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3: bad variable name


> Fixes: 08071ec0 "tools: variables clean-up in libvirt-guests script"
>
> Signed-off-by: Christian Ehrhardt 
> ---
>  tools/libvirt-guests.sh.in | 136 ++---
>  1 file changed, 68 insertions(+), 68 deletions(-)
>
> diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
> index 534c4d5e0f..d69df908d2 100644
> --- a/tools/libvirt-guests.sh.in
> +++ b/tools/libvirt-guests.sh.in
> @@ -30,9 +30,9 @@ test ! -r "$sysconfdir"/rc.d/init.d/functions ||
>
>  export TEXTDOMAIN="@PACKAGE@" TEXTDOMAINDIR="@localedir@"
>
> -URIS=default
> -ON_BOOT=start
> -ON_SHUTDOWN=suspend
> +URIS="default"
> +ON_BOOT="start"
> +ON_SHUTDOWN="suspend"
>  SHUTDOWN_TIMEOUT=300
>  PARALLEL_SHUTDOWN=0
>  START_DELAY=0
> @@ -65,7 +65,7 @@ retval() {
>  # If URI is "default" virsh is called without the "-c" argument
>  # (using libvirt's default connection)
>  run_virsh() {
> -local uri=$1
> +local uri="$1"
>  shift
>
>  if [ "x$uri" = xdefault ]; then
> @@ -86,7 +86,7 @@ run_virsh_c() {
>  # check if URI is reachable
>  test_connect()
>  {
> -local uri=$1
> +local uri="$1"
>
>  if run_virsh "$uri" connect 2>/dev/null; then
>  return 0;
> @@ -103,9 +103,9 @@ test_connect()
>  # --transient: list only transient guests
>  # [none]: list both persistent and transient guests
>  list_guests() {
> -local uri=$1
> -local persistent=$2
> -local list=$(run_virsh_c "$uri" list --uuid $persistent)
> +local uri="$1"
> +local persistent="$2"
> +local list="$(run_virsh_c "$uri" list --uuid $persistent)"
>
>  if [ $? -ne 0 ]; then
>  RETVAL=1
> @@ -118,8 +118,8 @@ list_guests() {
>  # guest_name URI UUID
>  # return name of guest UUID on URI
>  guest_name() {
> -local uri=$1
> -local uuid=$2
> +local uri="$1"
> +local uuid="$2"
>
>  run_virsh "$uri" domname "$uuid" 2>/dev/null
>  }
> @@ -128,17 +128,17 @@ guest_name() {
>  # check if guest UUID on URI is running
>  # Result is returned by variable "guest_running"
>  guest_is_on() {
> -local uri=$1
> -local uuid=$2
> -local id=$(run_virsh "$uri" domid "$uuid")
> +local uri="$1"
> +local uuid="$2"
> +local id="$(run_virsh "$uri" domid "$uuid")"
>
> -guest_running=false
> +guest_running="false"
>  if [ $? -ne 0 ]; then
>  RETVAL=1
>  return 1
>  fi
>
> -[ -n "$id" ] && [ "x$id" != x- ] && guest_running=true
> +[ -n "$id" ] && [ "x$id" != x- ] && guest_running="true"
>  return 0
>  }
>
> @@ -151,9 +151,9 @@ started() {
>  # start
>  # Start or re

Re: [PATCH] virdevmapper: Ignore all errors when opening /dev/mapper/control

On Wed, Aug 19, 2020 at 4:13 PM Michal Privoznik  wrote:
>
> So far, only ENOENT is ignored (to deal with kernels without
> devmapper). However, as reported on the list, under certain
> scenarios a different error can occur. For instance, when libvirt
> is running inside a container which doesn't have permissions to
> talk to the devmapper. If this is the case, then open() returns
> -1 and sets errno=EPERM.
>
> Assuming that multipath devices are fairly narrow use case and
> using them in a restricted container is even more narrow the best
> fix seems to be to ignore all open errors BUT produce a warning
> on failure. To avoid flooding logs with warnings on kernels
> without devmapper the level is reduced to a plain debug message.

While I'd love a warn_once such a thing doesn't exist right now.
It isn't the most common case and as discussed on IRC users are kind
of used to trim logs, so it should be ok.

Reviewed-by: Christian Ehrhardt 

> Reported-by: Christian Ehrhardt 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virdevmapper.c | 23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
> index 3cc399f382..7c89c2a952 100644
> --- a/src/util/virdevmapper.c
> +++ b/src/util/virdevmapper.c
> @@ -35,9 +35,12 @@
>  # include "viralloc.h"
>  # include "virstring.h"
>  # include "virfile.h"
> +# include "virlog.h"
>
>  # define VIR_FROM_THIS VIR_FROM_STORAGE
>
> +VIR_LOG_INIT("util.virdevmapper");
> +
>  # define PROC_DEVICES "/proc/devices"
>  # define DM_NAME "device-mapper"
>  # define DEV_DM_DIR "/dev/" DM_DIR
> @@ -130,11 +133,15 @@ virDMOpen(void)
>  memset(, 0, sizeof(dm));
>
>  if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0) {
> -if (errno == ENOENT)
> -return -2;
> -
> -virReportSystemError(errno, _("Unable to open %s"), CONTROL_PATH);
> -return -1;
> +/* We can't talk to devmapper. Produce a warning and let
> + * the caller decide what to do next. */
> +if (errno == ENOENT) {
> +VIR_DEBUG("device mapper not available");
> +} else {
> +VIR_WARN("unable to open %s: %s",
> + CONTROL_PATH, g_strerror(errno));
> +}
> +return -2;
>  }
>
>  if (!virDMIoctl(controlFD, DM_VERSION, , )) {
> @@ -309,9 +316,9 @@ virDevMapperGetTargets(const char *path,
>
>  if ((controlFD = virDMOpen()) < 0) {
>  if (controlFD == -2) {
> -/* The CONTROL_PATH doesn't exist. Probably the
> - * module isn't loaded, yet. Don't error out, just
> - * exit. */
> +/* The CONTROL_PATH doesn't exist or is unusable.
> + * Probably the module isn't loaded, yet. Don't error
> + * out, just exit. */
>  return 0;
>  }
>
> --
> 2.26.2
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [libvirt PATCH 0/2] meson: AppArmor fixes

On Wed, Aug 19, 2020 at 1:39 AM Andrea Bolognani  wrote:
>
> Found while updating the Debian package for libvirt to a snapshot
> taken from master. Possibly more to come.
>
> Andrea Bolognani (2):
>   meson: Set WITH_APPARMOR_PROFILES
>   meson: Don't hardcode /etc in APPARMOR_DIR

Thanks a lot for doing this work early on!
Changes LGTM

Reviewed-by: Christian Ehrhardt 

>  meson.build | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> --
> 2.26.2
>
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[PATCH] tools: fix libvirt-guests.sh text assignments

In libvirt 6.6 stopping guests with libvirt-guests.sh is broken.
As soon as there is more than one guest one can see
`systemctl stop libvirt-guests` faiing and in the log we see:
  libvirt-guests.sh[2455]: Running guests on default URI:
  libvirt-guests.sh[2457]: /usr/lib/libvirt/libvirt-guests.sh: 120:
  local: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae2: bad variable name
  libvirt-guests.sh[2462]: no running guests.

That is due do mutliple guests becoming a list of UUIDs. Without
recognizing this as one single string the assignment breaks when using 'local'
(which was recently added in 6.3.0). This is because local is defined as
  local [option] [name[=value] ... | - ]
which makes the shell trying handle the further part of the string as
variable names. In the error above that string isn't a valid variable
name triggering the issue that is seen.

To resolve that 'textify' all assignments that are strings or potentially
can become such lists (even if they are not using the local qualifier).

Fixes: 08071ec0 "tools: variables clean-up in libvirt-guests script"

Signed-off-by: Christian Ehrhardt 
---
 tools/libvirt-guests.sh.in | 136 ++---
 1 file changed, 68 insertions(+), 68 deletions(-)

diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index 534c4d5e0f..d69df908d2 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -30,9 +30,9 @@ test ! -r "$sysconfdir"/rc.d/init.d/functions ||
 
 export TEXTDOMAIN="@PACKAGE@" TEXTDOMAINDIR="@localedir@"
 
-URIS=default
-ON_BOOT=start
-ON_SHUTDOWN=suspend
+URIS="default"
+ON_BOOT="start"
+ON_SHUTDOWN="suspend"
 SHUTDOWN_TIMEOUT=300
 PARALLEL_SHUTDOWN=0
 START_DELAY=0
@@ -65,7 +65,7 @@ retval() {
 # If URI is "default" virsh is called without the "-c" argument
 # (using libvirt's default connection)
 run_virsh() {
-local uri=$1
+local uri="$1"
 shift
 
 if [ "x$uri" = xdefault ]; then
@@ -86,7 +86,7 @@ run_virsh_c() {
 # check if URI is reachable
 test_connect()
 {
-local uri=$1
+local uri="$1"
 
 if run_virsh "$uri" connect 2>/dev/null; then
 return 0;
@@ -103,9 +103,9 @@ test_connect()
 # --transient: list only transient guests
 # [none]: list both persistent and transient guests
 list_guests() {
-local uri=$1
-local persistent=$2
-local list=$(run_virsh_c "$uri" list --uuid $persistent)
+local uri="$1"
+local persistent="$2"
+local list="$(run_virsh_c "$uri" list --uuid $persistent)"
 
 if [ $? -ne 0 ]; then
 RETVAL=1
@@ -118,8 +118,8 @@ list_guests() {
 # guest_name URI UUID
 # return name of guest UUID on URI
 guest_name() {
-local uri=$1
-local uuid=$2
+local uri="$1"
+local uuid="$2"
 
 run_virsh "$uri" domname "$uuid" 2>/dev/null
 }
@@ -128,17 +128,17 @@ guest_name() {
 # check if guest UUID on URI is running
 # Result is returned by variable "guest_running"
 guest_is_on() {
-local uri=$1
-local uuid=$2
-local id=$(run_virsh "$uri" domid "$uuid")
+local uri="$1"
+local uuid="$2"
+local id="$(run_virsh "$uri" domid "$uuid")"
 
-guest_running=false
+guest_running="false"
 if [ $? -ne 0 ]; then
 RETVAL=1
 return 1
 fi
 
-[ -n "$id" ] && [ "x$id" != x- ] && guest_running=true
+[ -n "$id" ] && [ "x$id" != x- ] && guest_running="true"
 return 0
 }
 
@@ -151,9 +151,9 @@ started() {
 # start
 # Start or resume the guests
 start() {
-local isfirst=true
+local isfirst="true"
 local bypass=
-local sync_time=false
+local sync_time="false"
 local uri=
 local list=
 
@@ -167,10 +167,10 @@ start() {
 return 0
 fi
 
-test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache
-test "x$SYNC_TIME" = x0 || sync_time=true
+test "x$BYPASS_CACHE" = x0 || bypass="--bypass-cache"
+test "x$SYNC_TIME" = x0 || sync_time="true"
 while read uri list; do
-local configured=false
+local configured="false"
 local confuri=
 local guest=
 
@@ -178,7 +178,7 @@ start() {
 for confuri in $URIS; do
 set +f
 if [ "x$confuri" = "x$uri" ]; then
-configured=true
+configured="true"
 break
 fi
 done
@@ -192,14 +192,14 @@ start() {
 
 eval_gettext "Resuming guests on \$uri URI..."; echo
 for guest in $list; do
-local name=$(guest_name "

Re: [PATCH v2 0/2] Deal with kernels without DM support

On Tue, Aug 18, 2020 at 12:47 PM Christian Ehrhardt
 wrote:
>
> On Tue, Aug 18, 2020 at 12:11 PM Christian Ehrhardt
>  wrote:
> >
> > On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik  
> > wrote:
> > >
> > > v2 of:
> > >
> > > https://www.redhat.com/archives/libvir-list/2020-August/msg00489.html
> > >
> > > diff to v1:
> > > - After discussion to v1 I've decided to not cache DM major number and
> > >   thus the patches looks a bit different.
> > >
> > > Michal Prívozník (2):
> > >   virdevmapper: Don't cache device-mapper major
> > >   virdevmapper: Handle kernel without device-mapper support
> >

I put this through further testing and while working on containers in general
now, I found it breaks if I expose (an unusable) /dev/mapper/control
to the guest.
The guest isn't able to access it properly, but it exists and that breaks
libvirt 6.6 even with the recent virdevmapper fixes applied.

Conditions:
It is available:
# grep mapper /proc/devices
253 device-mapper
# ll /dev/mapper/control
crw--- 1 root root 10, 236 Aug 19 08:36 /dev/mapper/control

But not usable:
# dmsetup table
/dev/mapper/control: open failed: Operation not permitted
Failure to communicate with kernel device-mapper driver.
Check that device-mapper is available in the kernel.
Incompatible libdevmapper 1.02.167 (2019-11-30) and kernel driver
(unknown version).
Command failed.

I could further unconfine it to eventually work, but what I'd want for now
is that as discusse don IRC we might consider more bad RCs as "devmapper not in
use, don't remap paths"

For reference `dmsetup table` also gets:
 0.82 stat("/dev/mapper/control", {st_mode=S_IFCHR|0600,
st_rdev=makedev(0xa, 0xec), ...}) = 0 <0.21>
 0.94 openat(AT_FDCWD, "/dev/mapper/control", O_RDWR) = -1
EPERM (Operation not permitted) <0.21>

Starting the guest fails similar like it did in former issues around this:
# virsh start kvm-testguest-groovy-1
error: Failed to start domain kvm-testguest-groovy-1
error: internal error: Process exited prior to exec: libvirt: QEMU
Driver error : Unable to get devmapper targets for
/var/lib/uvtool/libvirt/images/kvm-testguest-groovy-1.qcow: Operation
not permitted

The strace of libvirt itself looks like:
# grep -C 3 -e 'mapper\/control' libvirt.strace.*
libvirt.strace.5332- 0.66 lstat("/dev/ptmx",
{st_mode=S_IFCHR|0666, st_rdev=makedev(0x5, 0x2), ...}) = 0 <0.14>
libvirt.strace.5332- 0.79 lstat("/dev/kvm",
{st_mode=S_IFCHR|0660, st_rdev=makedev(0xa, 0xe8), ...}) = 0
<0.14>
libvirt.strace.5332- 0.72
lstat("/var/lib/uvtool/libvirt/images/kvm-testguest-groovy-1.qcow",
{st_mode=S_IFREG|0600, st_size=229638144, ...}) = 0 <0.18>
libvirt.strace.5332: 0.67 openat(AT_FDCWD,
"/dev/mapper/control", O_RDWR) = -1 EPERM (Operation not permitted)
<0.17>
libvirt.strace.5332- 0.000142
umount2("/run/libvirt/qemu/5-kvm-testguest-groovy.dev", 0) = 0
<0.000195>
libvirt.strace.5332- 0.000254
stat("/run/libvirt/qemu/5-kvm-testguest-groovy.dev",
{st_mode=S_IFDIR|0775, st_size=40, ...}) = 0 <0.16>
libvirt.strace.5332- 0.68
access("/run/libvirt/qemu/5-kvm-testguest-groovy.dev", F_OK) = 0
<0.15>

And according to our IRC discussion:
[10:33]  on the other hand, it might not be that dirty, if
we can't open /dev/mapper/control for whatever reason then we can
safely assume that devmapper is not available/able to answer our
queries and thus no mpath devices can be on the system anyway
[10:33]  true
[10:33]  mprivozn: the only problem is that  EPERM just
indicates libvirtd can't access it
[10:33]  cpaelzer_: either will show what the problem is; my
guess is that open(/dev/mapper/control) in virDMOpen() fails
[10:34]  mprivozn: doesn't mean it can't be in use
[10:36]  danpb: agreed; but if we can't get deps for mpath
(or determine that its mpath) then I'd say move on with domain startup
and hope it wasn't mpath (which is a very narrow use case anyway)
[10:36]  yeah probably best on balance

So EPERM it is, would you add a change ignoring that RC please?

Re: [PATCH v2 0/2] Deal with kernels without DM support

2020-08-18 Thread Christian Ehrhardt

On Tue, Aug 18, 2020 at 12:11 PM Christian Ehrhardt
 wrote:
>
> On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik  wrote:
> >
> > v2 of:
> >
> > https://www.redhat.com/archives/libvir-list/2020-August/msg00489.html
> >
> > diff to v1:
> > - After discussion to v1 I've decided to not cache DM major number and
> >   thus the patches looks a bit different.
> >
> > Michal Prívozník (2):
> >   virdevmapper: Don't cache device-mapper major
> >   virdevmapper: Handle kernel without device-mapper support
>
> Reviewed-by: Christian Ehrhardt 
>
> Builds have started to re-test it as well ...

Done and working
Tested-by: Christian Ehrhardt 

>
> >  src/util/virdevmapper.c | 35 ++-----
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> >
> > --
> > 2.26.2
> >
>
>
> --
> Christian Ehrhardt
> Staff Engineer, Ubuntu Server
> Canonical Ltd



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH v2 0/2] Deal with kernels without DM support

2020-08-18 Thread Christian Ehrhardt

On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik  wrote:
>
> v2 of:
>
> https://www.redhat.com/archives/libvir-list/2020-August/msg00489.html
>
> diff to v1:
> - After discussion to v1 I've decided to not cache DM major number and
>   thus the patches looks a bit different.
>
> Michal Prívozník (2):
>   virdevmapper: Don't cache device-mapper major
>   virdevmapper: Handle kernel without device-mapper support

Reviewed-by: Christian Ehrhardt 

Builds have started to re-test it as well ...

>  src/util/virdevmapper.c | 35 ++-
>  1 file changed, 22 insertions(+), 13 deletions(-)
>
> --
> 2.26.2
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 0/2] virdevmapper: Deal with kernels without DM support

2020-08-18 Thread Christian Ehrhardt

On Mon, Aug 17, 2020 at 4:27 PM Michal Privoznik  wrote:
>
> *** BLURB HERE ***
>
> Michal Prívozník (2):
>   virdevmapper: Don't error on kernels without DM support
>   virdevmapper: Deal with unloading dm module

Hi Michal,
I know from the discussions on the patches that there likely will be
some changes before this is complete.
Nevertheless I did some testing with them already.

In particular since I was reporting this to also affect system
containers that have device-mapper
loaded as module and in /proc/devices but no /dev/mapper/control
exposed. I wanted to
give your patches a try for this use case as well since I wasn't sure
anyone else would.

I can confirm that functionally your patches applied on top of 6.6 (as
we work on it for
Debian) make it work again. Therefore:

Tested-by: Christian Ehrhardt 

>  src/util/virdevmapper.c | 59 -
>  1 file changed, 47 insertions(+), 12 deletions(-)
>
> --
> 2.26.2
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 1/2] apparmor: allow adding permanent per guest rules

2020-08-13 Thread Christian Ehrhardt

On Fri, Aug 7, 2020 at 6:14 PM Daniel P. Berrangé 
wrote:

> On Fri, Aug 07, 2020 at 12:21:19PM +0200, Christian Ehrhardt wrote:
> > The design of apparmor in libvirt always had a way to define custom
> > per-guest rules as described in docs/drvqemu.html and [1].
> >
> > A fix meant to clean the profiles after guest shutdown was a bit
> > overzealous and accidentially removed this important admin feature as
> > well.
> >
> > Therefore reduce the --delete option of virt-aa-helper to only delete
> > the .files that would be re-generated in any case.
> >
> > Users/Admins are always free to clean the profiles themselve if they
> > prefer a clean directory - they will be regenerated as needed. But
> > libvirt should never remove the base profile meant to allow per-guest
> > overrides and thereby break a documented feature.
> >
> > [1]: https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage
> >
> > Fixes: eba2225b "apparmor: delete profile on VM shutdown"
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/virt-aa-helper.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé 
>

(as with the other recent apparmor patch patch)
Thanks for the review - there was no negative feedback so far and in tests
this worked fine.
I'm committing the changes to not be postponed to close to the next release.


>
> Regards,
> Daniel
> --
> |: https://berrange.com  -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 2/2] apparmor: allow unmounting .dev entries

2020-08-13 Thread Christian Ehrhardt

On Fri, Aug 7, 2020 at 6:14 PM Daniel P. Berrangé 
wrote:

> On Fri, Aug 07, 2020 at 12:21:20PM +0200, Christian Ehrhardt wrote:
> > With qemu 5.0 and libvirt 6.6 there are new apparmor denials:
> >   apparmor="DENIED" operation="umount" profile="libvirtd"
> >   name="/run/libvirt/qemu/1-kvmguest-groovy-norm.dev/" comm="rpc-worker"
> >
> > These are related to new issues around devmapper handling [1] and the
> > error path triggered by these issues now causes this new denial.
> >
> > There are already related rules for mounting and it seems right to
> > allow also the related umount.
> >
> > [1]:
> https://www.redhat.com/archives/libvir-list/2020-August/msg00236.html
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/apparmor/usr.sbin.libvirtd.in | 1 +
> >  1 file changed, 1 insertion(+)
>
> Reviewed-by: Daniel P. Berrangé 
>

Thanks for the review - there was no negative feedback so far and in tests
this worked fine.
I'm committing the changes to not be postponed to close to the next release.


> Regards,
> Daniel
> --
> |: https://berrange.com  -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies

2020-08-10 Thread Christian Ehrhardt

On Thu, Aug 6, 2020 at 4:21 PM Andrea Bolognani  wrote:

> On Thu, 2020-08-06 at 15:45 +0200, Marc Hartmayer wrote:
> > On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani <
> abolo...@redhat.com> wrote:
> > > This patch broke libvirt in Debian for certain setups.
> > >
> > > With AppArmor enabled (the default), the error is
> > >
> > >   $ virsh start cirros
> > >   error: Failed to start domain cirros
> > >   error: internal error: Process exited prior to exec: libvirt:
> > >   error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
> > >   Device or resource busy
> > >
> > > If I disable AppArmor by passing security='' on the kernel command
> > > line, the error message changes to
> > >
> > >   $ virsh start cirros
> > >   error: Failed to start domain cirros
> > >   error: internal error: Process exited prior to exec: libvirt:
> > >   QEMU Driver error : Unable to get devmapper targets for
> > >   /var/lib/libvirt/images/cirros.qcow2: Success
> > >
> > > An effective workaround is to set namespaces=[] in qemu.conf, but
> > > that's of course not something that we want users doing :)
> > >
> > > The underlying issue seems to be caused by the fact that, on a Debian
> > > installation that uses plain partitions instead of LVM, /proc/devices
> > > doesn't contain an entry for device-mapper right after boot, which...
> > > ... this code expects.
> > >
> > > Running
> > >
> > >   $ sudo dmsetup info
> > >   No devices found
> >
> > We see the same problem as mentioned by Andrea. The host kernel
> > configuration used:
> >
> > …
> > CONFIG_BLK_DEV_DM_BUILTIN=y
> > CONFIG_BLK_DEV_DM=m
> > …
> >
> > As soon as we load the kernel module ‘dm-mod‘ everything works because
> > then ‘device-mapper‘ is listed in /proc/devices.
>
> Thanks Marc! I have confirmed that Debian also uses the same kernel
> configuration as the one you have reported above, and that running
> dmsetup(8) causes the dm-mod kernel module to be loaded.
>
> For comparison Fedora, where everything works fine, uses
>
>   CONFIG_BLK_DEV_DM_BUILTIN=y
>   CONFIG_BLK_DEV_DM=y
>

FYI even having the module loaded isn't the ultimate workaround as
there is a further twist of the same issue.

Ubuntu has:
  CONFIG_BLK_DEV_DM_BUILTIN=y
  CONFIG_BLK_DEV_DM=y

And there is an entry in devices (in host and in the container)
  $ cat /proc/devices | grep map
  253 device-mapper

But libvirt 6.6 in this case running in a LXD system container
(working before) now fails related to this with what seems to be the
same high level symptom.

# virsh start kvmguest-groovy-normal3
error: Failed to start domain kvmguest-groovy-normal3
error: internal error: Process exited prior to exec: libvirt: QEMU
Driver error : Unable to get devmapper targets for
/var/lib/uvtool/libvirt/images/kvmguest-groovy-normal3.qcow: No such
file or directory



> --
> Andrea Bolognani / Red Hat / Virtualization
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH v2 0/5] Further Debian/Ubuntu Apparmor Delta

2020-08-09 Thread Christian Ehrhardt

On Fri, Aug 7, 2020 at 2:59 PM Andrea Bolognani  wrote:

> On Tue, 2020-08-04 at 23:56 +0200, Andrea Bolognani wrote:
> > Anyway, I'm absolutely not an AppArmor expert but the pointers you
> > provide along with the various changes and the discussion around v1,
> > along with the fact that these patches have been shipped in Debian
> > and Ubuntu for so long, are convincing enough in my book, so
> >
> >   Reviewed-by: Andrea Bolognani 
>
> You don't seem to have pushed these yet. I can do that for you if you
> want, but since you are in the GitLab group with "Developer" role you
> should be able to do that on your own.
>

Thanks for the offer, I planned to push these today giving people who would
look more likely to review on the weekend a chance as well.
Now pushed with all the review/ack tags I got on these changes.

-- 
> Andrea Bolognani / Red Hat / Virtualization
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[PATCH 1/2] apparmor: allow adding permanent per guest rules

2020-08-07 Thread Christian Ehrhardt

The design of apparmor in libvirt always had a way to define custom
per-guest rules as described in docs/drvqemu.html and [1].

A fix meant to clean the profiles after guest shutdown was a bit
overzealous and accidentially removed this important admin feature as
well.

Therefore reduce the --delete option of virt-aa-helper to only delete
the .files that would be re-generated in any case.

Users/Admins are always free to clean the profiles themselve if they
prefer a clean directory - they will be regenerated as needed. But
libvirt should never remove the base profile meant to allow per-guest
overrides and thereby break a documented feature.

[1]: https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage

Fixes: eba2225b "apparmor: delete profile on VM shutdown"

Signed-off-by: Christian Ehrhardt 
---
 src/security/virt-aa-helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index dadb9d1614..4b66422b8f 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -99,7 +99,7 @@ vah_usage(void)
 "  Modes:\n"
 "-a | --add load profile\n"
 "-c | --create  create profile from template\n"
-"-D | --delete  unload and delete profile\n"
+"-D | --delete  unload profile and delete 
generated rules\n"
 "-r | --replace reload profile\n"
 "-R | --remove  unload profile\n"
 "  Options:\n"
@@ -1491,7 +1491,6 @@ main(int argc, char **argv)
 rc = parserRemove(ctl->uuid);
 if (ctl->cmd == 'D') {
 unlink(include_file);
-unlink(profile);
 }
 } else if (ctl->cmd == 'c' || ctl->cmd == 'r') {
 char *included_files = NULL;
-- 
2.27.0

[PATCH 2/2] apparmor: allow unmounting .dev entries

2020-08-07 Thread Christian Ehrhardt

With qemu 5.0 and libvirt 6.6 there are new apparmor denials:
  apparmor="DENIED" operation="umount" profile="libvirtd"
  name="/run/libvirt/qemu/1-kvmguest-groovy-norm.dev/" comm="rpc-worker"

These are related to new issues around devmapper handling [1] and the
error path triggered by these issues now causes this new denial.

There are already related rules for mounting and it seems right to
allow also the related umount.

[1]: https://www.redhat.com/archives/libvir-list/2020-August/msg00236.html

Signed-off-by: Christian Ehrhardt 
---
 src/security/apparmor/usr.sbin.libvirtd.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
b/src/security/apparmor/usr.sbin.libvirtd.in
index 1e137039e9..7c48b36e3d 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -31,6 +31,7 @@ profile libvirtd @sbindir@/libvirtd 
flags=(attach_disconnected) {
 
   mount options=(rw,rslave)  -> /,
   mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/,
+  umount /{var/,}run/libvirt/qemu/*.dev/,
 
   # libvirt provides any mounts under /dev to qemu namespaces
   mount options=(rw, move) /dev/ -> /{,var/}run/libvirt/qemu/*.dev/,
-- 
2.27.0

[PATCH v2 1/5] apparmor: allow default pki path

From: Sam Hartman 

/etc/pki/qemu is a pki path recommended by qemu tls docs [1]
and one that can cause issues with spice connections when missing.

Add the path to the allowed list of pki paths to fix the issue.

Note: this is active in Debian/Ubuntu [1] for quite a while already.

[1]: https://www.qemu.org/docs/master/system/tls.html
[2]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=930100

Signed-off-by: Christian Ehrhardt 
Acked-by: Jamie Strandboge 
---
 src/security/apparmor/libvirt-qemu | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index 1a4b226612..2d08d6f7ad 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -94,6 +94,8 @@
   /etc/pki/CA/* r,
   /etc/pki/libvirt{,-spice,-vnc}/ r,
   /etc/pki/libvirt{,-spice,-vnc}/** r,
+  /etc/pki/qemu/ r,
+  /etc/pki/qemu/** r,
 
   # the various binaries
   /usr/bin/kvm rmix,
-- 
2.27.0

[PATCH v2 4/5] apparmor: qemu access to @{PROC}/*/auxv for hw_cap

From: Stefan Bader 

On some architectures (ppc, s390x, sparc, arm) qemu will read auxv
to detect hardware capabilities via qemu_getauxval.

Allow that access read-only for the entry owned by the current
qemu process.

Signed-off-by: Christian Ehrhardt 
Signed-off-by: Stefan Bader 
Acked-by: Jamie Strandboge 
---
 src/security/apparmor/libvirt-qemu | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index b132cf0226..ae3db68f82 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -33,6 +33,8 @@
   owner @{PROC}/@{pid}/task/@{tid}/comm rw,
   @{PROC}/sys/kernel/cap_last_cap r,
   @{PROC}/sys/vm/overcommit_memory r,
+  # detect hardware capabilities via qemu_getauxval
+  owner @{PROC}/*/auxv r,
 
   # For hostdev access. The actual devices will be added dynamically
   /sys/bus/usb/devices/ r,
-- 
2.27.0

[PATCH v2 2/5] apparmor: allow libvirtd to call pygrub

From: Stefan Bader 

When using xen through libxl in Debian/Ubuntu it needs to be able to
call pygrub.

This is placed in a versioned path like /usr/lib/xen-4.11/bin.
In theory the rule could be more strict by rendering the libexec_dir
setting pkg-config can derive from libbxen-dev. But that would make
particular libvirt/xen packages version-depend on each other. It seems
more reasonable to avoid these versioned dependencies and use a wildcard
rule instead as it is already in place for libxl-save-helper.

Note: This change was in Debian [1] and Ubuntu [2] for quite some time
already.

[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=931768
[2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1326003

Signed-off-by: Christian Ehrhardt 
Acked-by: Jamie Strandboge 
---
 src/security/apparmor/usr.sbin.libvirtd.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
b/src/security/apparmor/usr.sbin.libvirtd.in
index 1e137039e9..312fa4b6d1 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd 
flags=(attach_disconnected) {
   /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
   /usr/{lib,lib64}/xen/bin/* Ux,
   /usr/lib/xen-*/bin/libxl-save-helper PUx,
+  /usr/lib/xen-*/bin/pygrub PUx,
   /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
 
   # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
-- 
2.27.0

[PATCH v2 0/5] Further Debian/Ubuntu Apparmor Delta

Hi,
I don't even remember which number of submissions that is #5 maybe?
Anyway - I'm hereby continuing to bring Debian and Ubuntu apparmor
Delta into upstream libvirt.

I have kept out all patches that are either Distro-specific or we ran
into trouble/discussions in the past. But there are enough left for a
new submission.

I have kept the most-original (read the earliest - as some patches
appeared in Ubuntu and later with a different Author in Debian) patch
author that I could find intact and git-send-email should auto-cc them.

I added some more bug links and descriptions so one can understand the
case a commit tries to fix without knowing too much context.

Update since v1:
- drop a few commits that in discussion turned out to be not/no-more needed
- fixed a few typos
- added the ack's that I received by Jamie Strandboge

Christian Ehrhardt (1):
  apparmor: let qemu load old shared objects after upgrades

Jamie Strandboge (1):
  apparmor: read only access to overcommit_memory

Sam Hartman (1):
  apparmor: allow default pki path

Stefan Bader (2):
  apparmor: allow libvirtd to call pygrub
  apparmor: qemu access to @{PROC}/*/auxv for hw_cap

 src/security/apparmor/libvirt-qemu | 10 ++
 src/security/apparmor/usr.sbin.libvirtd.in |  1 +
 2 files changed, 11 insertions(+)

-- 
2.27.0

[PATCH v2 5/5] apparmor: let qemu load old shared objects after upgrades

Since [1] qemu can after upgrade fall back to pre-upgrade modules
to still be able to dynamically load qemu-module based features.

The paths for these modules are pre-defined by the code and should
be allowed to be mapped and loaded from which will allow packagers
avoiding the inability of late feature load [2] after package upgrades.

[1]: https://github.com/qemu/qemu/commit/bd83c861
[2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361

Signed-off-by: Christian Ehrhardt 
Acked-by: Jamie Strandboge 
---
 src/security/apparmor/libvirt-qemu | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index ae3db68f82..a03e9e2c94 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -169,6 +169,11 @@
   /usr/{lib,lib64}/qemu/*.so mr,
   /usr/lib/@{multiarch}/qemu/*.so mr,
 
+  # let qemu load old shared objects after upgrades (LP: #1847361)
+  /{var/,}run/qemu/*/*.so mr,
+  # but explicitly deny writing to these files
+  audit deny /{var/,}run/qemu/*/*.so w,
+
   # swtpm
   /{usr/,}bin/swtpm rmix,
   /usr/{lib,lib64}/libswtpm_libtpms.so mr,
-- 
2.27.0

[PATCH v2 3/5] apparmor: read only access to overcommit_memory

From: Jamie Strandboge 

Allow qemu to read @{PROC}/sys/vm/overcommit_memory.
This is read on guest start-up and (as read-only) not a
critical secret that has to stay hidden.

Signed-off-by: Christian Ehrhardt 
Signed-off-by: Stefan Bader 
Signed-off-by: Jamie Strandboge 
---
 src/security/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index 2d08d6f7ad..b132cf0226 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -32,6 +32,7 @@
   # only modify its comm value or those in its thread group.
   owner @{PROC}/@{pid}/task/@{tid}/comm rw,
   @{PROC}/sys/kernel/cap_last_cap r,
+  @{PROC}/sys/vm/overcommit_memory r,
 
   # For hostdev access. The actual devices will be added dynamically
   /sys/bus/usb/devices/ r,
-- 
2.27.0

Re: [PATCH 3/8] apparmor: allow virt-aa-helper nameservices

On Mon, Aug 3, 2020 at 5:05 PM Jamie Strandboge  wrote:

> On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
>
> > Since quite a while libvirt-aa-helper triggers nss related apparmor
> > denials like:
> >  operation="open" profile="virt-aa-helper" name="/etc/nsswitch.conf"
> >  operation="open" profile="virt-aa-helper" name="/etc/host.conf"
> >  operation="open" profile="virt-aa-helper" name="/etc/resolv.conf"
> >  operation="open" profile="virt-aa-helper" name="/etc/hosts"
> >
> > Rules to allow these are in Debian [1] / Ubuntu [2] since quite a
> > while but do not seem to be specific to those distributions.
> >
> > There can be much more reasons than one would think to inadvertently
> > use/trigger nameservices as can be seen in the comments in
> > profiles/apparmor.d/abstractions/nameservice at [3].
> > The nameservices abstraction provides a nice and upgrade safe
> > way to cover all of them.

That is possible, initially we only had rules that looked like the head of
/etc/apparmor.d/abstractions/nameservice which is like
/etc/{group,gai.conf,nssswitch.conf,hosts}

But you are right, chances are that is reads that by accident. Back then in
bug
1513367 the discussion was a bit complex as it was about three different
apparmor
denials at once.
And in the Debian counterpart of
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979 they headed to
"nss should be save" right away.

All of the old reports called the denials either noise or a slowdown - so
looking back there didn't seem to be a functional impact.
Which would be a +1 to your theory.

I was removing the rule, but just as when this bug came up at first it
isn't triggering for various test environments and cases that I tried.

>

> [1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979
> > [2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1546674
> > [3]: https://gitlab.com/apparmor/apparmor
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > index dd18c8ab89..dfc61e8de4 100644
> > --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > @@ -2,6 +2,7 @@
> >
> >  profile virt-aa-helper @libexecdir@/virt-aa-helper {
> >#include 
> > +  #include 
>
> nameservice brings in network rules so this is actually a lot of access.
> Why is it reaching out to nss? Is it just cause some library happens to
> look at /etc/nsswitch.conf and pull in other things or does it actually
> need networking? I suspect the former. If my suspicion is true, perhaps
> instead:
>
>   # virt-aa-helper dependent libraries read (and if successful, other
>   # files) but virt-aa-helper itself doesn't require the access, so
>   # silence the denial.
>   deny /etc/nsswitch.conf r,
>

I like your suggestion, especially as this isn't the profile for libvirtd
but just virt-aa-helper.
That denial seems non critical while most likely prevents the follow up
access.

I was looking for nss libs in virt-aa-helper context that "might be
required" but the only path
to it seems to come up in regard to rbd support which isn't needed
to render apparmor
rules in virt-aa-helper.

I'd really want to make it part of a v2 submission, but feel that it is
wrong to do so without
being able to re-create it to know for sure where the access was from
exactly.
I was trying back to 16.04 (being the time this was reported) with local
disks (that is what
triggered it back then) or ceph (as rbd is the only path to libnss3)  :-/

For now I'm just removing this commit from the list of proposed changes.

-- 
> Jamie Strandboge | http://www.canonical.com
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 7/8] apparmor: allow virt-aa-helper to read openvswitch sockets

On Mon, Aug 3, 2020 at 5:13 PM Jamie Strandboge  wrote:

> On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
>
> > From: Serge Hallyn 
> >
> > Chardevs/sockets configured for openvswitch-dpdk use cases
> > might be probed by virt-aa-helper. Allow that access to enable
> > virt-aa-helper rendering per-guest rules for the actual qemu
> > guest accessing these sockets eventually.
> >
> > Signed-off-by: Christian Ehrhardt 
> > Signed-off-by: Stefan Bader 
> > Signed-off-by: Serge Hallyn 
> > ---
> >  src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > index 3f204799a6..877cb04b1e 100644
> > --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > @@ -46,6 +46,9 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper {
> >@sysconfdir@/apparmor.d/libvirt/* r,
> >
> > @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*
> rw,
> >
> > +  # for openvswitch sockets
> > +  /{,var/}run/openvswitch/** rw,
>
> A bit unfortunate and unexpected. What kind of probing does
> virt-aa-helper do on these?
>

I'm so glad we do this exercise and you have the "investigative hat on" to
challenge the few bits of the series that seem odd.
I have read through virt-aa-helper again with a focus on this and at least
today's openvswitch-dpdk+libvirt should not need this anymore.

It seems this was a wild guess many years ago and added for bug 1513367 but
eventually (or just noadays) is no longer needed.

I have set up a 20.04 based openvswitch-dpdk system and dropped the rule.
Once with vhostuserclient and once on an older system with the older
vhostuser type connection.

Things are still working, so I'm removing this rule from this series as
well as from the Ubuntu builds.

> --
> Jamie Strandboge | http://www.canonical.com
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 6/8] apparmor: allow virt-aa-helper to read from tmp

On Mon, Aug 3, 2020 at 5:11 PM Jamie Strandboge  wrote:

> On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
>
> > From: Stefan Bader 
> >
> > temporary directories are a common place images are placed by users
> > for any sort of quick evaluation. Allow virt-aa-helper access to tmp
> > via the existing user-tmp apparmor abstraction.
> >
> > That way if a guest definition has paths in temporary directories
> > virt-aa-helper can properly probe them e.g. for further backing files in
> > the case of qcow2.
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > index dfc61e8de4..3f204799a6 100644
> > --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
> > @@ -3,6 +3,7 @@
> >  profile virt-aa-helper @libexecdir@/virt-aa-helper {
> >#include 
> >#include 
> > +  #include 
>
> user-tmp allows write and all other accesses for disks are read. We have
> these rules:
>
>   /**.img r,
>   /**.raw r,
>   /**.qcow{,2} r,
>   /**.qed r,
>   /**.vmdk r,
>   /**.vhd r,
>   /**.[iI][sS][oO] r,
>   /**/disk{,.*} r,
>
> Why are these not sufficient? What was the denial that triggered the
> issue?
>

Great question to ask - this is one of the Deltas that was "just carried"
for quite some time.
But you made me analyze the background and it isn't reasonable IMHO.
It was added quite some time ago without outlining  a particular reason.

The list you refer to above wasn't as long back then (~5 years ago), maybe
extending the list would have been all that was needed and instead the
user-tmp abstraction was added.

I'll drop the commit from this series as well as from Ubuntu on next merge
and check for any fallout - it is easy to be added back and most likely not
needed.

-- 
> Jamie Strandboge | http://www.canonical.com
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 5/8] apparmor: qemu access to @{PROC}/*/auxv for hw_cap

On Mon, Aug 3, 2020 at 5:07 PM Jamie Strandboge  wrote:

> On Mon, 03 Aug 2020, Christian Ehrhardt wrote:
>
> > From: Stefan Bader 
> >
> > On some architectures (ppc, s390x, sparc, arm) qemu will read auxv
> > to detect hardware capabilities via qemu_getauxval.
> >
> > Allow that access read-only for the entry owned by the current
> > qemu process.
> >
> > Signed-off-by: Christian Ehrhardt 
> > Signed-off-by: Stefan Bader 
> > ---
> >  src/security/apparmor/libvirt-qemu | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/security/apparmor/libvirt-qemu
> b/src/security/apparmor/libvirt-qemu
> > index b132cf0226..25eff20b82 100644
> > --- a/src/security/apparmor/libvirt-qemu
> > +++ b/src/security/apparmor/libvirt-qemu
> > @@ -33,6 +33,7 @@
> >owner @{PROC}/@{pid}/task/@{tid}/comm rw,
> >@{PROC}/sys/kernel/cap_last_cap r,
> >@{PROC}/sys/vm/overcommit_memory r,
> > +  owner @{PROC}/*/auxv r,
>
> +1 to apply. A code comment that is simply the first sentence of
> Stefan's commit message might be a nice touch, but that is not a
> blocker.
>

Yeah I added that comment when researching the reason - added a comment to
the commit, but not worth a v2 submission.

Tanks for reading through all of these changes!


> --
> Jamie Strandboge | http://www.canonical.com
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[PATCH 3/8] apparmor: allow virt-aa-helper nameservices

Since quite a while libvirt-aa-helper triggers nss related apparmor
denials like:
 operation="open" profile="virt-aa-helper" name="/etc/nsswitch.conf"
 operation="open" profile="virt-aa-helper" name="/etc/host.conf"
 operation="open" profile="virt-aa-helper" name="/etc/resolv.conf"
 operation="open" profile="virt-aa-helper" name="/etc/hosts"

Rules to allow these are in Debian [1] / Ubuntu [2] since quite a
while but do not seem to be specific to those distributions.

There can be much more reasons than one would think to inadvertently
use/trigger nameservices as can be seen in the comments in
profiles/apparmor.d/abstractions/nameservice at [3].
The nameservices abstraction provides a nice and upgrade safe
way to cover all of them.

[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882979
[2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1546674
[3]: https://gitlab.com/apparmor/apparmor

Signed-off-by: Christian Ehrhardt 
---
 src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in 
b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
index dd18c8ab89..dfc61e8de4 100644
--- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
+++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
@@ -2,6 +2,7 @@
 
 profile virt-aa-helper @libexecdir@/virt-aa-helper {
   #include 
+  #include 
 
   # needed for searching directories
   capability dac_override,
-- 
2.27.0

[PATCH 4/8] apparmor: read only access to overcommit_memory

From: Jamie Strandboge 

Allow qemu to read @{PROC}/sys/vm/overcommit_memory.
This is read on guest start-up and (as read-only) not a
critical secret that has to stay hidden.

Signed-off-by: Christian Ehrhardt 
Signed-off-by: Stefan Bader 
Signed-off-by: Jamie Strandboge 
---
 src/security/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index 2d08d6f7ad..b132cf0226 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -32,6 +32,7 @@
   # only modify its comm value or those in its thread group.
   owner @{PROC}/@{pid}/task/@{tid}/comm rw,
   @{PROC}/sys/kernel/cap_last_cap r,
+  @{PROC}/sys/vm/overcommit_memory r,
 
   # For hostdev access. The actual devices will be added dynamically
   /sys/bus/usb/devices/ r,
-- 
2.27.0

[PATCH 6/8] apparmor: allow virt-aa-helper to read from tmp

From: Stefan Bader 

temporary directories are a common place images are placed by users
for any sort of quick evaluation. Allow virt-aa-helper access to tmp
via the existing user-tmp apparmor abstraction.

That way if a guest definition has paths in temporary directories
virt-aa-helper can properly probe them e.g. for further backing files in
the case of qcow2.

Signed-off-by: Christian Ehrhardt 
---
 src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in 
b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
index dfc61e8de4..3f204799a6 100644
--- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
+++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in
@@ -3,6 +3,7 @@
 profile virt-aa-helper @libexecdir@/virt-aa-helper {
   #include 
   #include 
+  #include 
 
   # needed for searching directories
   capability dac_override,
-- 
2.27.0

[PATCH 5/8] apparmor: qemu access to @{PROC}/*/auxv for hw_cap

From: Stefan Bader 

On some architectures (ppc, s390x, sparc, arm) qemu will read auxv
to detect hardware capabilities via qemu_getauxval.

Allow that access read-only for the entry owned by the current
qemu process.

Signed-off-by: Christian Ehrhardt 
Signed-off-by: Stefan Bader 
---
 src/security/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index b132cf0226..25eff20b82 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -33,6 +33,7 @@
   owner @{PROC}/@{pid}/task/@{tid}/comm rw,
   @{PROC}/sys/kernel/cap_last_cap r,
   @{PROC}/sys/vm/overcommit_memory r,
+  owner @{PROC}/*/auxv r,
 
   # For hostdev access. The actual devices will be added dynamically
   /sys/bus/usb/devices/ r,
-- 
2.27.0

[PATCH 2/8] apparmor: allow libvirtd to call pygrub

From: Stefan Bader 

When using xen through libxl in Debian/Ubuntu it needs to be able to
call pygrub.

This is placed in a versioned path like /usr/lib/xen-4.11/bin.
In theory the rule could be more strict by rendering the libexec_dir
setting pkg-config can derive from libbxen-dev. But that would make
particular libvirt/xen packages version-depend on each other. It seems
more reasonable to avoid these versioned dependencies and use a wildcard
rule instead as it is already in place for libxl-save-helper.

Note: This change was in Debian [1] and Ubuntu [2] for quite some time
already.

[1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=931768
[2]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1326003

Signed-off-by: Christian Ehrhardt 
---
 src/security/apparmor/usr.sbin.libvirtd.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.sbin.libvirtd.in 
b/src/security/apparmor/usr.sbin.libvirtd.in
index 1e137039e9..312fa4b6d1 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd 
flags=(attach_disconnected) {
   /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
   /usr/{lib,lib64}/xen/bin/* Ux,
   /usr/lib/xen-*/bin/libxl-save-helper PUx,
+  /usr/lib/xen-*/bin/pygrub PUx,
   /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
 
   # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
-- 
2.27.0

[PATCH 0/8] Further Debian/Ubuntu Apparmor Delta

Hi,
I don't even remember which number of submissions that is #5 maybe?
Anyway - I'm hereby continuing to bring Debian and Ubuntu apparmor
Delta into upstream libvirt.

I have kept out all patches that are either Distro-specific or we ran
into trouble/discussions in the past. But there are enough left for a
new submission.

I have kept the most-original (read the earliest - as some patches
appeared in Ubuntu and later with a different Author in Debian) patch
author that I could find intact and git-send-email should auto-cc them.

I added some more bug links and descriptions so one can understand the
case a commit tries to fix without knowing too much context.

Christian Ehrhardt (2):
  apparmor: allow virt-aa-helper nameservices
  apparmor: let qemu load old shared objects after upgrades

Jamie Strandboge (1):
  apparmor: read only access to overcommit_memory

Sam Hartman (1):
  apparmor: allow default pki path

Serge Hallyn (1):
  apparmor: allow virt-aa-helper to read openvswitch sockets

Stefan Bader (3):
  apparmor: allow libvirtd to call pygrub
  apparmor: qemu access to @{PROC}/*/auxv for hw_cap
  apparmor: allow virt-aa-helper to read from tmp

 src/security/apparmor/libvirt-qemu  | 9 +
 src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 +
 src/security/apparmor/usr.sbin.libvirtd.in  | 1 +
 3 files changed, 15 insertions(+)

-- 
2.27.0

[PATCH 8/8] apparmor: let qemu load old shared objects after upgrades

Since [1] qemu can after upgrade fall back to pre-upgrade modules
to still be able to dynamically load qmeu-module based features.

The paths for these modules are pre-defined by the code and should
be allowed to be mapped and loaded from which will allow packagers
avoiding the inability of late feature load [2] after package upgrades.

[1]: https://github.com/qemu/qemu/commit/bd83c861
[2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361

Signed-off-by: Christian Ehrhardt 
---
 src/security/apparmor/libvirt-qemu | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/security/apparmor/libvirt-qemu 
b/src/security/apparmor/libvirt-qemu
index 25eff20b82..c6f7149799 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -168,6 +168,11 @@
   /usr/{lib,lib64}/qemu/*.so mr,
   /usr/lib/@{multiarch}/qemu/*.so mr,
 
+  # let qemu load old shared objects after upgrades (LP: #1847361)
+  /{var/,}run/qemu/*/*.so mr,
+  # but explicitly deny writing to these files
+  audit deny /{var/,}run/qemu/*/*.so w,
+
   # swtpm
   /{usr/,}bin/swtpm rmix,
   /usr/{lib,lib64}/libswtpm_libtpms.so mr,
-- 
2.27.0

[PATCH 1/8] apparmor: allow default pki path