[libvirt] [PATCH 8/9] qemu: explicitly delete standard tap devices only on platforms that require it

[Daniel P. Berrangé]

On Fri, Sep 06, 2019 at 11:37:12AM -0400, Laine Stump wrote:
> On 9/6/19 5:16 AM, Daniel P. Berrangé wrote:
> > On Tue, Aug 27, 2019 at 09:46:38PM -0400, Laine Stump wrote:
> > > libvirt creates its tap devices without the IFF_PERSIST flag, so they
> > > will be automatically deleted when qemu is finished with them. In the
> > > case of tap devices created outside of libvirt, if the creating entity
> > > wants the devices to be deleted, it will also omit IFF_PERSIST, but if
> > > it wants them to remain (e.g. for re-use), then it will use
> > > IFF_PERSIST when creating the device.
> > > 
> > > Back when support was added for autocreation by libvirt of tap devices
> > > for <interface type='ethernet'> (commit 9c17d665), code was mistakenly
> > > put in qemuProcessStop to always delete tap devices for
> > > type='ethernet'. This should only be done on platforms that have
> > > VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP #defined (which is only
> > > FreeBSD).
> > This isn't right. The tap devices should *always* be deleted as we
> > don't trust that QEMU hasn't (possibly maliciously) set IFF_PERSIST
> > itself.
> 
> 
> (So you're saying this is a security issue that was coincidentally fixed by
> commit 9c17d665?)

I'm not sure I'd call it a security issue - more of a reliability
issue - since its really just a denial of service at most. We just
want to be sure we're not leaving anything behind when QEMU quits.

> Interesting point. But I wonder if it's also problematic that (in the case
> of a tap device created by someone else (not libvirt) who purposefully set
> IFF_PERSIST) QEMU could mistakenly/maliciously *clear* IFF_PERSIST. I guess
> there's really nothing we could do about that though, since the device would
> already be deleted by the time we found out about it...
> 
> 
> I found this bit of code specifically because I was creating tap devices
> with IFF_PERSIST set (that's just what "ip tuntap add" does), and they were
> disappearing after each use, which may or may not be what the user wants -
> another case of "someone" clearing IFF_PERSIST, but in this case it is *us*.

If they don't want the device deleted, then they can set managed=no now
with your patch series.

> And as a matter of fact I can't see a way to even force macvtap devices to
> be deleted by an unprivileged process - when I had libvirt try to do the
> standard delete, it would fail. So having this unconditional forced delete
> of all standard tap devices both causes an unexpected behavior for some
> users, as well as creating an inconsistency between tap and macvtap behavior
> (standard taps are always deleted, macvtaps are never deleted).

We don't currently support pre-createds macvtap, so we can fix this
inconsistency so that it works the way tap devs do.

> (This reminds me of another inconsistency I saw while looking at this, but
> then later forgot - virNetDevTapDelete() is *never * called in the case of
> hot-unplug. So if you think that we should be unconditionally deleting all
> taps after use regardless of the previous state of IFF_PERSIST of
> pre-created taps, then we should also be doing it for hot-unplug.)
> 
> 
> So how about if we remember the setting of IFF_PERSIST prior to starting
> QEMU, and restore it to its previous state afterwards? That would make
> behavior more what was expected / consistent with macvtap.

I don't think we need rmemeber IFF_PERSIST when we have the "managed" flag
now.


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