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

Laine Stump laine at redhat.com
Fri Sep 6 15:37:12 UTC 2019


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 at redhat.com>
>> ---
>>   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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> Regards,
> Daniel





More information about the libvir-list mailing list