On 9/6/19 11:46 AM, Daniel P. Berrangé wrote:
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.
Right. In the case of <interface type='ethernet'> anyway. There *could*
be cases where someone is using a pre-existing tap device with
<interface type='bridge'> (that works, although only by the coincidence
that TUNSETIFF will create the specified device if it doesn't exist, or
just return a handle to any existing device), but I suppose anyone
already doing that will already be accustomed to the current (since
2016) behavior. And I'm trying to think of a good reason why somebody
would want to use a pre-existing tap device but still rely on libvirt
to connect it to the bridge, and I can't think of anything, so the
number of new people who would want to do this is probably vanishingly
small.
TL;DR - I'm convinced.
I think for consistency I will make a patch that does the same thing
both for shutting qemuProcessStop() and qemuDomainRemoveNetDevice()
(hot-unplug). (Really it looks like a good reorganization is in order -
there are multiple bits that cycle through all the netdefs to shutdown
various things, and they all should just happen in one place, if for no
other reason than so that we can sequester them off in qemu_interface.c,
provide a simple API, then make that into its own module. Or "block",
some would say :-)
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.
Right. And since the only way to use a pre-created macvtap is with
managed=no, we've done that (again, as long as QEMU doesn't clear
IFF_PERSIST on standard taps).
(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.
Yeah, the only situation that would warrant intervention would be to try
and recover from QEMU maliciously clearing IFF_PERSIST, but by the time
libvirt gets control it's too late anyway - the tap is already deleted.
As always, thanks for the reviews!
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
