Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tap update for modern linux versions #64

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Tap update for modern linux versions #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a77713
temporarily disable forking causing basiliskii to hang
hfmanson Mar 10, 2015
8f7b5de
SheepShaver ditto
hfmanson Mar 11, 2015
f7eaa77
tap interface update for modern linux using 'ip tuntap'
hfmanson Mar 12, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions BasiliskII/src/Unix/configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,7 @@ dnl Check that the host supports TUN/TAP devices
AC_CACHE_CHECK([whether TUN/TAP is supported],
ac_cv_tun_tap_support, [
AC_TRY_COMPILE([
#include <sys/socket.h>
#if defined(HAVE_LINUX_IF_H) && defined(HAVE_LINUX_IF_TUN_H)
#include <linux/if.h>
#include <linux/if_tun.h>
Expand Down
50 changes: 27 additions & 23 deletions BasiliskII/src/Unix/ether_unix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,11 +240,12 @@ bool ether_init(void)

// Determine Ethernet device type
net_if_type = -1;
if (strncmp(name, "tap", 3) == 0)
net_if_type = NET_IF_ETHERTAP;
#if ENABLE_TUNTAP
else if (strcmp(name, "tun") == 0)
if (strncmp(name, "tap", 3) == 0)
net_if_type = NET_IF_TUNTAP;
#else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to "tun"?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is a bit of a wall of text, hopefully the headings help a little:

tun is a confusing name since it's a tap device

As mentioned in the blog post @hfmanson linked to, and this more obviously canonical kernel.org source (which I'm quoting from):

Depending on the type of device chosen the userspace program has to read/write
IP packets (with tun) or ethernet frames (with tap). Which one is being used
depends on the flags given with the ioctl().

As you can see in the code, it uses ifr.ifr_flags = IFF_TAP, so it's using a tap-type interface (thankfully, because I want to use AppleTalk over one of these interfaces!), but the code being deleted above requires you to set ether tun in your configuration to select this, which is confusing.

This is why @hfmanson also says "tunconfig (which should be renamed tapconfig IMO) [...]".

Prior to this patch, tap is used to refer to ethertap

I guess that the term tun is used to refer to this type of interface because it is a "TUN/TAP" interface (as per the kernel.org document linked above) and although the code is using the tap type, the code also supports ethertap interfaces by setting ether tapN, so the term tap was already taken.

However, it's quite confusing to use tun to refer to these tap devices. It makes it difficult to search the web for help with issues if you think you're using a tun device when you're using a tap device.

This patch makes TUN/TAP and ethertap mutually exclusive at build time

As @hfmanson says, "when tuntap is detected in the configure script ethertap is no longer used and the user can enter tapN as ether device". In other words, this patch means that when you run configure, if your host supports both ethertap and TUN/TAP, then you'll only be able to use TUN/TAP. That means tap can be used to refer to the interface regardless of whether you're using ethertap or TUN/TAP.

I think that makes sense - http://user-mode-linux.sourceforge.net/old/UserModeLinux-HOWTO-6.html indicates that ethertap is for users of the Linux kernel version 2.2, and TUN/TAP is for 2.4 and later. It also says "Ethertap is available on 2.4 and works fine. TUN/TAP is preferred to it because it has better performance and ethertap is officially considered obsolete in 2.4." CentOS 7's kernel (version 3.10, which is relatively old) seems to be built without ethertap support.

How to select TUN/TAP or ethertap in the future

The patch causes both ethertap and TUN/TAP to be selected by providing the name of an existing tapN interface.

Since the handling of TUN/TAP is changed significantly by the patch - the tunconfig script is called to create and configure the interface before BasiliskII attaches to it, instead of the previous behaviour where the tunconfig script was called to configure the interface after BasiliskII had created it, and therefore the script is significantly different - any users that previously used tun are going to have to do some work to move to the new scheme.

I think that the code ought to still do something about users with tun in their configuration, though. With this patch, it looks to me like that configuration would cause the code to fall back to the NET_IF_SHEEPNET case, and presumably the user would then get an error message saying that there's no interface called tun. It would be much nicer to give them an error message saying that they should read the updated README file. Also, there really needs to be an update to the README file!

Alternatives

In Bochs, you specify that you want to use a tap type of TUN/TAP interface by specifying tuntap (I guess it is implicit that it's using a tap type). You also need to provide the path /dev/net/tun (which you can see BasiliskII hard-codes, which is probably fine). Then, if you have an existing tap interface, you can append :tapN to that path (probably the interface's name doesn't even need to start with tap, as the kernel doesn't enforce that, but I haven't tested this), otherwise Bochs will create the interface for you, much like BasiliskII does without this patch.

It would probably be nice to act like Bochs in terms of separating the "I want to use a tap-type interface" configuration and the "here is the name of the existing tap-type interface I want to use" configuration rather than enforcing that the interface's name start with tap.

As for emulating Bochs's ability to optionally create the interface for you, the fact that this patch runs the tunconfig script before it attaches to the interface would make this tricky, since you'd want to run it after if you just created the interface. I think that the scheme used in this patch, where the script is run before attaching to the interface, is pretty unconventional for emulators, and that generally if you wanted BasiliskII to not create the interface itself using cap_net_admin, you would simply create the interface before starting BasiliskII, which one could of course do with a wrapper script. I would prefer to have something that worked more like Bochs and less like this patch in this regard, although I don't have strong feelings either way - I'm happy to just create the interface myself and then tell BasiliskII to run an empty script.

if (strncmp(name, "tap", 3) == 0)
net_if_type = NET_IF_ETHERTAP;
#endif
#ifdef HAVE_SLIRP
else if (strcmp(name, "slirp") == 0)
Expand Down Expand Up @@ -291,12 +292,15 @@ bool ether_init(void)
// Open sheep_net or ethertap or TUN/TAP device
char dev_name[16];
switch (net_if_type) {
case NET_IF_ETHERTAP:
sprintf(dev_name, "/dev/%s", name);
break;
#if ENABLE_TUNTAP
case NET_IF_TUNTAP:
strcpy(dev_name, "/dev/net/tun");
break;
#else
case NET_IF_ETHERTAP:
sprintf(dev_name, "/dev/%s", name);
break;
#endif
case NET_IF_SHEEPNET:
strcpy(dev_name, "/dev/sheep_net");
break;
Expand All @@ -314,14 +318,6 @@ bool ether_init(void)
// Open TUN/TAP interface
if (net_if_type == NET_IF_TUNTAP) {
struct ifreq ifr;
memset(&ifr, 0, sizeof(ifr));
ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
strcpy(ifr.ifr_name, "tun%d");
if (ioctl(fd, TUNSETIFF, (void *) &ifr) != 0) {
sprintf(str, GetString(STR_SHEEP_NET_ATTACH_WARN), strerror(errno));
WarningAlert(str);
goto open_error;
}

// Get network config script file path
net_if_script = PrefsFindString("etherconfig");
Expand All @@ -334,12 +330,20 @@ bool ether_init(void)
WarningAlert(str);
goto open_error;
}
net_if_name = strdup(ifr.ifr_name);
net_if_name = strdup(name);
if (!execute_network_script("up")) {
sprintf(str, GetString(STR_TUN_TAP_CONFIG_WARN), "script execute error");
WarningAlert(str);
goto open_error;
}
memset(&ifr, 0, sizeof(ifr));
ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
strcpy(ifr.ifr_name, name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What ensures that ifr_name is big enough to hold name? Please use strlcpy instead.

if (ioctl(fd, TUNSETIFF, (void *) &ifr) != 0) {
sprintf(str, GetString(STR_SHEEP_NET_ATTACH_WARN), strerror(errno));
WarningAlert(str);
goto open_error;
}
D(bug("Connected to host network interface: %s\n", net_if_name));
}
#endif
Expand Down Expand Up @@ -431,14 +435,6 @@ void ether_exit(void)
// Stop reception threads
stop_thread();

// Shut down TUN/TAP interface
if (net_if_type == NET_IF_TUNTAP)
execute_network_script("down");

// Free TUN/TAP device name
if (net_if_name)
free(net_if_name);

// Close sheep_net device
if (fd > 0)
close(fd);
Expand All @@ -453,6 +449,14 @@ void ether_exit(void)
if (slirp_output_fd > 0)
close(slirp_output_fd);

// Shut down TUN/TAP interface
if (net_if_type == NET_IF_TUNTAP)
execute_network_script("down");

// Free TUN/TAP device name
if (net_if_name)
free(net_if_name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're changing this, could you add a line that sets it to NULL?


#if STATISTICS
// Show statistics
printf("%ld messages put on write queue\n", num_wput);
Expand Down
2 changes: 1 addition & 1 deletion BasiliskII/src/Unix/main_unix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ int main(int argc, char **argv)

#if defined(ENABLE_XF86_DGA) && !defined(ENABLE_MON)
// Fork out, so we can return from fullscreen mode when things get ugly
XF86DGAForkApp(DefaultScreen(x_display));
// XF86DGAForkApp(DefaultScreen(x_display));
#endif
#endif

Expand Down
34 changes: 19 additions & 15 deletions BasiliskII/src/Unix/tunconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#!/bin/bash

###########################################################################
# Configuration of the tunN devices for usage with Basilisk II.
# (derived MOL tunconfig script)
Expand All @@ -18,7 +19,7 @@
###########################################################################

SUDO=/usr/bin/sudo
IFCONFIG=/sbin/ifconfig
IP=/sbin/ip
IPTABLES=/sbin/iptables

#########################################################
Expand All @@ -28,13 +29,14 @@ IPTABLES=/sbin/iptables
shift 1
}

TUN_DEV=$1
TAP_DEV=$1
ACTION=$2

TUN_NUM=`echo $TUN_DEV | sed s/[^0-9]//g`
NET_NUM=`expr 40 + $TUN_NUM`
TUN_NET=172.20.$NET_NUM.0/24
TUN_HOST=172.20.$NET_NUM.1
TAP_NUM=`echo $TAP_DEV | sed s/[^0-9]//g`
NET_NUM=`expr 40 + $TAP_NUM`
TAP_NET=172.20.$NET_NUM.0/24
TAP_HOST=172.20.$NET_NUM.1/24
USER_ID=`id -u`

#########################################################
# Misc Checks
Expand All @@ -45,7 +47,7 @@ TUN_HOST=172.20.$NET_NUM.1
exit 2
}

[[ "`id -u`" = "0" ]] && {
[[ "$USER_ID" = "0" ]] && {
echo "---> $SUDO not necessary." 1>&2
SUDO=""
}
Expand All @@ -56,15 +58,15 @@ TUN_HOST=172.20.$NET_NUM.1
}

if [ -n "$SUDO" ]; then
$SUDO -l | grep -q "NOPASSWD: $IFCONFIG" || {
echo "---> Missing sudo NOPASSWD: $IFCONFIG." 1>&2
$SUDO -l | grep -q "NOPASSWD: $IP" || {
echo "---> Missing sudo NOPASSWD: $IP." 1>&2
exit 1
}
$SUDO -l | grep -q "NOPASSWD: $IPTABLES" || {
echo "---> Missing sudo NOPASSWD: $IPTABLES." 1>&2
exit 1
}
IFCONFIG="$SUDO $IFCONFIG"
IP="$SUDO $IP"
IPTABLES="$SUDO $IPTABLES"
fi

Expand All @@ -77,26 +79,28 @@ $IPTABLES -L -n -t nat > /dev/null || exit 1
#########################################################

{
$IPTABLES -t nat -D POSTROUTING -s $TUN_NET -d ! $TUN_NET -j MASQUERADE
$IPTABLES -t nat -D POSTROUTING -s $TAP_NET ! -d $TAP_NET -j MASQUERADE
} >& /dev/null

#########################################################
# Bring down interface
#########################################################

[[ "$ACTION" = down ]] && {
$IFCONFIG $TUN_DEV down
$IP tuntap del $TAP_DEV mode tap
}

#########################################################
# Configure interface
#########################################################

[[ "$ACTION" = up ]] && {
$IFCONFIG $TUN_DEV $TUN_HOST

$IP tuntap add $TAP_DEV mode tap user $USER_ID
$IP addr add $TAP_HOST dev $TAP_DEV
$IP link set $TAP_DEV up
# masquerade the tun network
$IPTABLES -t nat -A POSTROUTING -s $TUN_NET -d ! $TUN_NET -j MASQUERADE
$IPTABLES -t nat -A POSTROUTING -s $TAP_NET ! -d $TAP_NET -j MASQUERADE
}

exit 0

2 changes: 1 addition & 1 deletion SheepShaver/src/Unix/main_unix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,7 @@ int main(int argc, char **argv)

#if defined(ENABLE_XF86_DGA) && !defined(ENABLE_MON)
// Fork out, so we can return from fullscreen mode when things get ugly
XF86DGAForkApp(DefaultScreen(x_display));
//XF86DGAForkApp(DefaultScreen(x_display));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to the TAP fixes and I don't think should be in this commit.

#endif
#endif

Expand Down