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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
