Differential D20044
tun/tap: merge Authored by kevans on Apr 25 2019, 2:05 AM. Tags None Referenced Files
Details tun(4) and tap(4) share the same general management interface and have a lot in common. Bugs exist in tap(4) that have been fixed in tun(4), and vice-versa. Let's reduce the maintenance requirements by merging them together. Many of the early differences are carried in the tun_driver introduced here. The cdev methods are basically identical between the two; L2 versions of read/write are pulled over from tap(4) to keep it simple for now.
Diff Detail
Event TimelineComment Actions Update tap(4) to reflect the new reality. vmnet unit numbers no longer start above 0x800000 because it's actually easier to base tun(4) behavior on the name and use separate unit namespace for the three different flavors of tun. Comment Actions Adding @tuexen as a reviewer. If my memory serves me right he worked on a small part of of tun recently. Comment Actions Looks like a really cool and logical thing to do! Several comments inline.
Comment Actions Address comments from @melifaro:
Combined the read/write implementations a bit more: tunread and tunread_l2 were substantially the same except for IFQ_DEQUEUE vs. IF_DEQUEUE and a well-placed BPF_MTAP. For our purposes tap(4) should be fine with IFQ_DEQUEUE. tunwrite and tunwrite_l2 had a common prologue; tunwrite_{l2,l3} have been split out and all three character devices now only differ in name-only. I've also fixed some other nits:
Lightly tested with bhyve + installation media
Comment Actions Addressed the last of @melifaro's comments. Completed the illusion that we're a flexible driver; tun_driver now includes the match/create/destroy functions that we need to create the cloners. We maintain a per-vnet slist of tun_driver/cloner sets and populate it on vnet init from only the information in tun_drivers. This reduces the work needed to add/remove variations on the tun driver. New variations would need to:
There's less room for error in the vnet bits in the new world. Comment Actions Would it make sense to use if_tuntap.c instead of if_tun.c, the same for the name of the module to load, the device to compile in? Comment Actions I was pretty torn on naming- tuntap would definitely make sense and has the advantage of being a lot more obvious if one is looking for the tap driver source. Tun also makes sense to an extent (since tap is just a layer-2 tunnel) and only forces tap users to change things, rather than tap and the subset of tun users that load it as a module rather than letting it remain in their kernel config. That's probably really not that big of a deal, given the nature of this. Comment Actions I can see that you consider a tap interface a tunnelling interface, but the same applies to other tunnelling interfaces not related to the tun interface. Comment Actions I tested this patch and the tun interface worked as expected. However, the tap interface didn't. Running the program on an unpatched system results in output like:tuexen@bsd14:~ % sudo ./tap-test Received packet of length 42 Received packet of length 86 Received packet of length 130 Received packet of length 150 Received packet of length 86 Received packet of length 90 Received packet of length 74 tuexen@bsd14:~ % On a patched system I only get: tuexen@head:~ % sudo ./tap-test tuexen@head:~ % When capturing traffic on the tap0 interface on the unpatched system, I see (after some other packets) the injected UDP packet and the resulting ICMP packet. Can you reproduce the issue? Comment Actions I'll take a look and attempt to reproduce ASAP. Next iteration will rename if_tun -> if_tuntap Comment Actions I can't seem to reproduce this issue (though some things aren't functional in my current test system (until tomorrow), as I'm bridging with encrypted wifi): # sysctl net.link.tap.up_on_open=1 # ifconfig tap0 create # ifconfig bridge0 create # ifconfig bridge0 addm wlan0 addm tap0 up # cc tap-test.c # ./a.out Received packet of length 86 Received packet of length 130 Received packet of length 130 Received packet of length 74 I'll run more tests in the morning on another system on ethernet, but the only way I was able to reproduce is by not setting the up_on_open sysctl (since the interface isn't up'd in the test). Comment Actions Rename to tuntap, amend UPDATING and all kernel configs that have it included, as well as NOTES. Alias logic stolen from the fusefs module, since it recently went through a rename as well. Some bits within tuntap, mostly in what I've added, were renamed to reflect the new module name. Comment Actions OK, here is the difference: On an unpatched system I get: tuexen@bsd14:~ % sysctl net.link.tap.up_on_open net.link.tap.up_on_open: 0 tuexen@bsd14:~ % sudo ifconfig tap0 create tuexen@bsd14:~ % ifconfig tap0 tap0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=80000<LINKSTATE> ether 00:bd:e6:5d:c2:00 groups: tap media: Ethernet autoselect status: no carrier nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL> tuexen@bsd14:~ % sudo ifconfig tap0 inet 172.20.0.170 netmask 255.255.255.0 tuexen@bsd14:~ % ifconfig tap0 tap0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=80000<LINKSTATE> ether 00:bd:e6:5d:c2:00 inet 172.20.0.170 netmask 0xffffff00 broadcast 172.20.0.255 groups: tap media: Ethernet autoselect status: no carrier nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL> tuexen@bsd14:~ % On a patched system I get: tuexen@head:~ % sysctl net.link.tap.up_on_open net.link.tap.up_on_open: 1 tuexen@head:~ % sudo ifconfig tap0 create tuexen@head:~ % ifconfig tap0 tap0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=80000<LINKSTATE> ether 58:9c:fc:10:ff:ed groups: tap media: Ethernet autoselect status: no carrier nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL> tuexen@head:~ % sudo ifconfig tap0 inet 172.20.0.170 netmask 255.255.255.0 tuexen@head:~ % ifconfig tap0 tap0: flags=8842<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=80000<LINKSTATE> ether 58:9c:fc:10:ff:ed inet 172.20.0.170 netmask 0xffffff00 broadcast 172.20.0.255 groups: tap media: Ethernet autoselect status: no carrier nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL> tuexen@head:~ % This means: On an unpatched system, setting the IPv4 address brings the interface up, whereas on an patched system, the Comment Actions Oy, that's a major oversight on my part. =( if_tun handles SIOCSIFADDR with tuninit, tap drops it through to ether_ioctl -- easy fix... I'll audit the rest of the ioctls and make sure I didn't mess up the behavior in any of the others. Thanks for the diagnosis! Comment Actions Fix ioctl issue diagnosed by @tuexen (thanks!)
if_tap.h now includes if_tun.h. Comment Actions Perhaps out of scope for this and should be split out, but add a mechanism to ifconfig(8) to allow explicitly mapping interface names to a specific kld. This allows remapping tun*, tap*, and vmnet* to if_tuntap to kill off errors akin to: interface if_tuntap.1 already present in the KLD 'if_tun.ko'! linker_load_file: /boot/kernel/if_tap.ko - unsupported file type that arise due to how we do backwards compatibility stuff. It's unclear to me if ifconfig should always auto-load if_tuntap for tun* and tap* operations, or if it should only be provided for compatibility for the next N major versions. Comment Actions I tested this with syzkaller (if_tap) and packetdrill (if_tun) and it works as intended. Comment Actions just noting that ports and scripts that rely on doing kldload if_tap will obviously break at this point. Something like: + # tap(4) & tun(4) were unified in r347241, this is closest ABI bump
+ if [ `uname -U` -ge 1300029 ]; then
+ if_tap="if_tuntap"
+ else
+ if_tap="if_tap"
+ fi
+ kldload "${if_tap}"is all that should be needed; leaving this as a reference for others, Comment Actions if_tap.ko and if_tun.ko still exist, because the if_tuntap module creates compatibility links and will do so for the foreseeable future. These things should still slowly migrate to using if_tuntap instead, but it's not clear to me how best to migrate current ports and potential future ports to using the new name. Comment Actions thanks Kyle,
FreeBSD wintermute.skunkwerks.at 13.0-CURRENT FreeBSD 13.0-CURRENT r348047+1c489956cbe7(D10335) GENERIC amd64
kldload: can't load if_tuntap: module already loaded or in kernel
kldload: an error occurred while loading module if_tap. Please check dmesg(8) for more details.
[84319] interface if_tuntap.1 already present in the KLD 'kernel'! dch@wintermute /p/k/a/r/unbound> l /boot/kernel/if_tap.ko I'd be delighted if the behaviour could be changed, as this simply breaks stuff at present. Which is fine, that's what current is for, but a less breaking change might be a nicer choice. From a user perspective people are expecting that kldload if_tap should return the same result as kldload if_tuntap assuming that the appropriate module is loaded. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||