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

On 9/6/19 5:16 AM, Daniel P. Berrangé wrote: (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. 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.