Entering freeze for libvirt-9.1.0

[Stefano Brivio]

On Thu, 23 Feb 2023 10:25:28 +0100
Jiri Denemark <jdenemar at redhat.com> wrote:

> On Wed, Feb 22, 2023 at 17:02:48 +0100, Stefano Brivio wrote:
> > On Wed, 22 Feb 2023 15:23:04 +0100
> > Jiri Denemark <jdenemar at redhat.com> wrote:
> >   
> > > I have just tagged v9.1.0-rc1 in the repository and pushed signed
> > > tarballs and source RPMs to https://libvirt.org/sources/
> > > 
> > > Please give the release candidate some testing and in case you find a
> > > serious issue which should have a fix in the upcoming release, feel
> > > free to reply to this thread to make sure the issue is more visible.  
> > 
> > The "passt" network back-end is entirely non-functional on distributions
> > shipping with SELinux: the binary helper can't be executed. The
> > 'virsh start' command reports:
> > 
> >   error: internal error: Could not start 'passt': libvirt:  error : cannot execute binary /usr/bin/passt: Permission denied
> > 
> > and the guest doesn't start. This is on Fedora 37, but it should be
> > universally reproducible.
> > 
> > I provided more details on the thread at:
> >   https://listman.redhat.com/archives/libvir-list/2023-February/238096.html
> > 
> > This is the relevant snippet from my domain XML file:
> > 
> >     <interface type='user'>
> >       <mac address='52:54:00:36:21:6f'/>
> >       <model type='virtio'/>
> >       <backend type='passt'/>
> >       <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
> >     </interface>  
> 
> Yes, this is quite unfortunate, but there are even distributions that do
> not ship SELinux.

There's an issue with a similar outcome (albeit different nature) for
AppArmor, and I'm working on it with Andrea (who maintains the libvirt
Debian package -- I maintain the passt one). Fixing that doesn't require
code changes in libvirt, though.

> And this is not a regression since 9.0.0, is it?

It's not.

> As we're in freeze for 9.1.0 release so reasonable bug fixes considered
> safe (as in the chance for them to break more than they are fixing is
> considered low) are welcome. But if, e.g., a patch (series) even though
> being a bug fix contains a nontrivial refactor, it should really wait
> until after the release. Unless it's fixing a critical bug.

I totally agree (of course...). I think it's a reasonable policy.

A refactor of even two/three functions of the security manager wouldn't
seem to fit this. I might be wrong as I'm not familiar with that code,
but that's the impression I had. Big or small, nobody implemented it
in time for 9.1.0.

> That said, if this can reasonably be fixed without risking other issues
> before the release, we can do so. But otherwise since this is a new
> functionality and SELinux is not present in all distributions, there's
> no reason to push something big and risky at the last moment or delay
> the release because of this issue. We don't do this for AppArmor either.

Note: I'm not discussing about progress here -- this is just for the
records.

Also for the records: wouldn't it be appropriate to mark the
functionality as broken (with SELinux), so that users don't waste their
time thinking they're doing something wrong? This is the main reason
why I wanted to have this working in 9.1.0 by the way.

I proposed two different fixes (one was almost entirely proposed by
Michal to be fair -- I just added the bugs you found), which I tested
quite thoroughly (unlike what was merged in 9.0.0, which, my bad, I
didn't have time to review).

Neither of them is what I'd call "big". Note that 2/3 and 3/3 are
unrelated fixes (and 3/3 is already merged):

v1:
  1 file changed, 1 insertion(+), 6 deletions(-)

  The functionality can be used without disabling SELinux, the correct
  domain transition happens.

  Risk: the MCS part of the labels of different passt instances is the
  same, which is not ideal (i.e. SELinux wouldn't prevent one passt
  process to write the PID file of another passt process), but it's
  obviously a minor degradation compared to "setenforce 0".

v2:
  1 file changed, 27 insertions(+), 6 deletions(-)

  The MCS part of SELinux labels is also differentiated between
  instances.

  Risk: the missing #ifdef you just pointed out and (potentially)
  similar issues.

-- 
Stefano