tun/tap: merge

[kevans]

Summary

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.

[kevans]

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.

[bz]

Adding @tuexen as a reviewer. If my memory serves me right he worked on a small part of of tun recently.

[melifaro]

Looks like a really cool and logical thing to do! Several comments inline.

sys/net/if_tun.c

152 ↗

(On Diff #56623)

Would it be possible to make the name slightly more descriptive, like, tap_allow_uopen? It would ease understanding tunopen() / tunclone() code :-)

667 ↗

(On Diff #56623)

Would it be possible to convert direct mtx_lock() calls to something like TUN_LOCK() / TUN_UNLOCK() ?

1172 ↗

(On Diff #56623)

Would it be possible to use more descriptive variable name and a named bitmask?

1430 ↗

(On Diff #56623)

I may be wrong, but from a quick glance, BPF_MTAP() seem to the only difference between tunread() and tunread_l2(). IF true, maybe it is worth unifying these two?

[kevans]

Address comments from @melifaro:

• s/tapuopen/tap_allow_uopen/ for clarity
• Sprinkled some TUN_{UN,}LOCK() around

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:

• Adjusted the copyright
• Fixed the ioctl handler to actually not error out on tap(4) ioctls
• Rebased with the dying/locking changes I committed this morning

Lightly tested with bhyve + installation media

[kevans]

sys/net/if_tun.c

1172 ↗

(On Diff #56623)

Ah, sorry, I missed this one- I'll do this in the next round.

[rgrimes]

sys/net/if_tun.c

3 ↗

(On Diff #56646)

This tag is minimally incomplete, there are addition license conditions in this file.

30 ↗

(On Diff #56646)

I am a bit iffy on adding a "license" which is in conflict with the already existing one, ie it places additional restriction that did not exist before. Is this really needed? Did you add this (kyle), or did someone else add this to some source (Maksim Yevmenkin) your drawing from?

[kevans]

sys/net/if_tun.c

30 ↗

(On Diff #56646)

Initially added by myself, but given that the tap code I merged in should maintain Maksim Yevmenkin's notice/license I'm not sure how to represent this.

[imp]

sys/net/if_tun.c

30 ↗

(On Diff #56646)

This is not in conflict with the current license, per se. The other license is a crappy one from the early days that doesn't grant even the right to make a derived work (just distribution). The intention is clearly more, but it's poorly drafted so it's unclear.
A lot depends on how much additional code was added, as this only covers such additions. But it's not that different from other places in the tree we've done similar things over time.

[kevans]

sys/net/if_tun.c

30 ↗

(On Diff #56646)

I started trying to get in contact with Julian via e-mail to see if he'd be willing to re-license this as BSD-2-Clause just to simplify the matter. Both of the e-mail addresses I had found for him bounced, so I'm trying other methods.

He allowed OpenBSD to change the license back in 2002, so I see no reason he wouldn't be OK with it... if I can get in contact.

[kevans]

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:

• Define the identifying flag
• Create the *_match method
• Setup the tun_driver definition
• Use the identifying flag for behavioral changes

There's less room for error in the vnet bits in the new world.

[tuexen]

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?

[kevans]

In D20044#432063, @tuexen wrote:

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?

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.

[tuexen]

In D20044#432075, @kevans wrote:

In D20044#432063, @tuexen wrote:

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?

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.

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.
To minimise the impact for users, you could build a if_tuntap module and some dummy modules for if_tun and if_tap, which just load if_tuntap...

[tuexen]

I tested this patch and the tun interface worked as expected. However, the tap interface didn't. Running the program

tap-test.c1 KBDownload

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.
When running tcpdump -i tap0 -n on a patched system, no packets are shown at all.

Can you reproduce the issue?

[kevans]

I'll take a look and attempt to reproduce ASAP.

Next iteration will rename if_tun -> if_tuntap

[kevans]

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).

[kevans]

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.

[tuexen]

In D20044#432207, @kevans wrote:

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).

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
interface stays down. Is that intended?

[kevans]

In D20044#432226, @tuexen wrote:

... [snip] ...
This means: On an unpatched system, setting the IPv4 address brings the interface up, whereas on an patched system, the
interface stays down. Is that intended?

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!

[kevans]

Fix ioctl issue diagnosed by @tuexen (thanks!)
Fix up some other ioctl issues:

• #define tapinfo as tuninfo (equivalently sized/named before)
• #define all of the TAP* ioctls as TUN* ioctls, just for clarity (they were equivalent values before)

if_tap.h now includes if_tun.h.

[kevans]

Slight rework of TUNSIFMODE handling, remove stale comment.

[kevans]

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.

[kevans]

Any other comments on this?

[bcr]

OK from manpages. Sorry to keep you waiting.

[tuexen]

I tested this with syzkaller (if_tap) and packetdrill (if_tun) and it works as intended.

[dch]

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,
or a recommended improvement if there's a better way to handle this.

[kevans]

In D20044#438958, @dch wrote:

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,
or a recommended improvement if there's a better way to handle this.

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.

[dch]

thanks Kyle,

uname -a

FreeBSD wintermute.skunkwerks.at 13.0-CURRENT FreeBSD 13.0-CURRENT r348047+1c489956cbe7(D10335) GENERIC amd64

sudo kldload if_tuntap

kldload: can't load if_tuntap: module already loaded or in kernel

sudo kldload if_tap

kldload: an error occurred while loading module if_tap. Please check dmesg(8) for more details.

sudo dmesg | tail -3

[84319] interface if_tuntap.1 already present in the KLD 'kernel'!
[84319] linker_load_file: /boot/kernel/if_tap.ko - unsupported file type

dch@wintermute /p/k/a/r/unbound> l /boot/kernel/if_tap.ko
lrwxr-xr-x 1 root wheel 12B May 21 10:01 /boot/kernel/if_tap.ko@ -> if_tuntap.ko
^^^^^^ this is what's causing the problem.

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.