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?)
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*. 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).
(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.
To change the topic a bit - I actually find it strange that some of the
IF flags can be modified by an unprivileged process and some can't. From
what I've seen (if I'm remembering my experiments correctly - I didn't
take notes, but the implementation is based on the following
observations), an unprivileged process can set/clear:
IFF_VNET_HDR
IFF_PERSIST
but can't set/clear
IFF_MULTI_QUEUE
IFF_UP
I can't really see a way to categorize the implications of these flags
to justify the difference - each looks just as important/potentially
disruptive/not as the other. (Also it's not possible to change the MAC
address).
And to top that off, the method of getting a handle to a standard tap
device is via opening /dev/tun/tap and then calling TUNSETIFF (which
magically makes the handle you have to /dev/tun/tap be a handle to the
specific tap device), and if you request the wrong setting of an
"unmodifiable" IF flag in the TUNSETIFF ifreq struct, it will fail. For
example, if the tap was created with IFF_MULTI_QUEUE and libvirt doesn't
set IFF_MULTI_QUEUE in the TUNSETIFF, the ioctl will fail (and the same
for the opposite setting of the flags). For IFF_VNET_HDR, though, the
value it was created with is essentially ignored, and the setting given
in libvirt's TUNSETIFF is honored. The result is that we can't just
"leave all the flags alone", we have to make sure some are set the same
as at tap creation, and others are set as we want them to be (regardless
of how the tap was created).
(The list of which flags can/can't be set by an unprivileged process is
at least consistent between tap and macvtap, although it's problematic
that you can clear IFF_PERSIST for a tap (effectively deleting it), but
*can't* do an RTM_DELLINK on a macvtap).
I went in assuming that *none* of the flags would be changeable, and I
could just not set any of them, but was sorely disappointed - I have to
set IFF_VNET_HDR appropriately or performance of virtio devices will
suffer, and I have to set IFF_MULTI_QUEUE appropriately or the TUNSETIFF
will fail completely.
This mistake has been corrected, along with the unnecessary check for
non-null net->ifname (it must always be non-null), and erroneous
VIR_FREE of net->ifname.
There could be a risk of net->ifname being NULL if qemuProcessStart
fails early in startup before all tap devices have finished being
created IIUC.
Ah, I hadn't thought of the case where qemuProcessStop() is just being
used to clean up after a failed qemuProcessStart(). I'll go back and
recheck.
Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
---
src/qemu/qemu_process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 11c1ba8fb9..3449abf2ec 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7548,10 +7548,10 @@ void qemuProcessStop(virQEMUDriverPtr driver,
cfg->stateDir));
break;
case VIR_DOMAIN_NET_TYPE_ETHERNET:
- if (net->managed_tap != VIR_TRISTATE_BOOL_NO && net->ifname) {
+#ifdef VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP
+ if (net->managed_tap != VIR_TRISTATE_BOOL_NO)
ignore_value(virNetDevTapDelete(net->ifname, net->backend.tap));
- VIR_FREE(net->ifname);
- }
+#endif
break;
case VIR_DOMAIN_NET_TYPE_BRIDGE:
case VIR_DOMAIN_NET_TYPE_NETWORK:
--
2.21.0
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
