router_linux: use only baseline 'ip rule' features that exist in old kernels.

This removes the use of suppress_ifgroup and fwmark "x/y" notation,
which are, among other things, not available in busybox and centos6.

We also use the return codes from the 'ip' program instead of trying to
parse its output.

I also had to remove the previous hack that routed all of 100.64.0.0/10
by default, because that would add the /10 route into the 'main' route
table instead of the new table 88, which is no good. It was a terrible
hack anyway; if we wanted to capture that route, we should have
captured it explicitly as a subnet route, not as part of the addr. Note
however that this change affects all platforms, so hopefully there
won't be any surprises elsewhere.

Fixes #405
Updates #320, #144

Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>

ipn/local.go

@@ -778,11 +778,7 @@ func routerConfig(cfg *wgcfg.Config, prefs *Prefs, dnsDomains []string) *router.
 	for _, addr := range cfg.Addresses {
 		addrs = append(addrs, wgcfg.CIDR{
 			IP:   addr.IP,
-			// TODO(apenwarr): this shouldn't be hardcoded in the client
+			Mask: 32,
-			// TODO(danderson): fairly sure we can make this a /32 or
-			// /128 based on address family. Need to check behavior on
-			// !linux OSes.
-			Mask: 10,
 		})
 	}
 

wgengine/router/router_linux.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 
 	"github.com/coreos/go-iptables/iptables"
@@ -42,10 +43,10 @@ import (
 const (
 	// Packet is from Tailscale and to a subnet route destination, so
 	// is allowed to be routed through this machine.
-	tailscaleSubnetRouteMark = "0x10000/0x10000"
+	tailscaleSubnetRouteMark = "0x10000"
 	// Packet was originated by tailscaled itself, and must not be
 	// routed over the Tailscale network.
-	tailscaleBypassMark = "0x20000/0x20000"
+	tailscaleBypassMark = "0x20000"
 )
 
 // chromeOSVMRange is the subset of the CGNAT IPv4 range used by
@@ -114,6 +115,24 @@ func newUserspaceRouterAdvanced(logf logger.Logf, tunname string, netfilter netf
 
 type osCommandRunner struct{}
 
+func errCode(err error) int {
+	if err == nil {
+		return 0
+	}
+	var e *exec.ExitError
+	if ok := errors.As(err, &e); ok {
+		return e.ExitCode()
+	}
+	s := err.Error()
+	if strings.HasPrefix(s, "exitcode:") {
+		code, err := strconv.Atoi(s[9:])
+		if err == nil {
+			return code
+		}
+	}
+	return -42
+}
+
 func (o osCommandRunner) run(args ...string) error {
 	_, err := o.output(args...)
 	return err
@@ -126,12 +145,40 @@ func (o osCommandRunner) output(args ...string) ([]byte, error) {
 
 	out, err := exec.Command(args[0], args[1:]...).CombinedOutput()
 	if err != nil {
-		return nil, fmt.Errorf("running %q failed: %v\n%s", strings.Join(args, " "), err, out)
+		return nil, fmt.Errorf("running %q failed: %w\n%s", strings.Join(args, " "), err, out)
 	}
 
 	return out, nil
 }
 
+type runGroup struct {
+	OkCode int           // an error code that is acceptable, other than 0, if any
+	Runner commandRunner // the runner that actually runs our commands
+	ErrAcc error         // first error encountered, if any
+}
+
+func newRunGroup(okCode int, runner commandRunner) *runGroup {
+	return &runGroup{
+		OkCode: okCode,
+		Runner: runner,
+	}
+}
+
+func (rg *runGroup) Output(args ...string) []byte {
+	b, err := rg.Runner.output(args...)
+	if rg.ErrAcc == nil && err != nil && errCode(err) != rg.OkCode {
+		rg.ErrAcc = err
+	}
+	return b
+}
+
+func (rg *runGroup) Run(args ...string) {
+	err := rg.Runner.run(args...)
+	if rg.ErrAcc == nil && err != nil && errCode(err) != rg.OkCode {
+		rg.ErrAcc = err
+	}
+}
+
 func (r *linuxRouter) Up() error {
 	if err := r.delLegacyNetfilter(); err != nil {
 		return err
@@ -469,14 +516,24 @@ func (r *linuxRouter) delLoopbackRule(addr netaddr.IP) error {
 // interface. Fails if the route already exists, or if adding the
 // route fails.
 func (r *linuxRouter) addRoute(cidr netaddr.IPPrefix) error {
-	return r.cmd.run("ip", "route", "add", normalizeCIDR(cidr), "dev", r.tunname, "scope", "global")
+	return r.cmd.run(
+		"ip", "route", "add",
+		normalizeCIDR(cidr),
+		"dev", r.tunname,
+		"table", "88",
+	)
 }
 
 // delRoute removes the route for cidr pointing to the tunnel
 // interface. Fails if the route doesn't exist, or if removing the
 // route fails.
 func (r *linuxRouter) delRoute(cidr netaddr.IPPrefix) error {
-	return r.cmd.run("ip", "route", "del", normalizeCIDR(cidr), "dev", r.tunname, "scope", "global")
+	return r.cmd.run(
+		"ip", "route", "del",
+		normalizeCIDR(cidr),
+		"dev", r.tunname,
+		"table", "88",
+	)
 }
 
 // addSubnetRule adds a netfilter rule that allows traffic to flow
@@ -512,53 +569,130 @@ func (r *linuxRouter) delSubnetRule(cidr netaddr.IPPrefix) error {
 	return nil
 }
 
-// upInterface brings up the tunnel interface and adds it to the
+// upInterface brings up the tunnel interface.
-// Tailscale interface group.
 func (r *linuxRouter) upInterface() error {
-	return r.cmd.run("ip", "link", "set", "dev", r.tunname, "group", "10000", "up")
+	return r.cmd.run("ip", "link", "set", "dev", r.tunname, "up")
 }
 
-// downInterface sets the tunnel interface administratively down, and
+// downInterface sets the tunnel interface administratively down.
-// returns it to the default interface group.
 func (r *linuxRouter) downInterface() error {
-	return r.cmd.run("ip", "link", "set", "dev", r.tunname, "group", "0", "down")
+	return r.cmd.run("ip", "link", "set", "dev", r.tunname, "down")
 }
 
 // addBypassRule adds the policy routing rule that avoids tailscaled
 // routing loops. If the rule exists and appears to be a
 // tailscale-managed rule, it is gracefully replaced.
 func (r *linuxRouter) addBypassRule() error {
+	// Clear out old rules. After that, any error adding a rule is fatal,
+	// because there should be no reason we add a duplicate.
 	if err := r.delBypassRule(); err != nil {
 		return err
 	}
-	return r.cmd.run("ip", "rule", "add", "fwmark", tailscaleBypassMark, "priority", "10000", "table", "main", "suppress_ifgroup", "10000")
+
+	rg := newRunGroup(0, r.cmd)
+
+	// NOTE(apenwarr): We leave spaces between each pref number.
+	// This is so the sysadmin can override by inserting rules in
+	// between if they want.
+
+	// NOTE(apenwarr): This sequence seems complicated, right?
+	// If we could simply have a rule that said "match packets that
+	// *don't* have this fwmark", then we would only need to add one
+	// link to table 88 and we'd be done. Unfortunately, older kernels
+	// and 'ip rule' implementations (including busybox), don't support
+	// checking for the lack of a fwmark, only the presence. The technique
+	// below works even on very old kernels.
+
+	// Packets from us, tagged with our fwmark, first try the kernel's
+	// main routing table.
+	rg.Run(
+		"ip", "rule", "add",
+		"pref", "8810",
+		"fwmark", tailscaleBypassMark,
+		"table", "main",
+	)
+	// ...and then we try the 'default' table, for correctness,
+	// even though it's been empty on every Linux system I've ever seen.
+	rg.Run(
+		"ip", "rule", "add",
+		"pref", "8830",
+		"fwmark", tailscaleBypassMark,
+		"table", "default",
+	)
+	// If neither of those matched (no default route on this system?)
+	// then packets from us should be aborted rather than falling through
+	// to the tailscale routes, because that would create routing loops.
+	rg.Run(
+		"ip", "rule", "add",
+		"pref", "8850",
+		"fwmark", tailscaleBypassMark,
+		"type", "unreachable",
+	)
+	// If we get to this point, capture all packets and send them
+	// through to table 88, the set of tailscale routes.
+	// For apps other than us (ie. with no fwmark set), this is the
+	// first routing table, so it takes precedence over all the others,
+	// ie. VPN routes always beat non-VPN routes.
+	//
+	// NOTE(apenwarr): tables >255 are not supported in busybox.
+	// I really wanted to use table 8888 here for symmetry, but no luck
+	// with busybox alas.
+	rg.Run(
+		"ip", "rule", "add",
+		"pref", "8888",
+		"table", "88",
+	)
+	// If that didn't match, then non-fwmark packets fall through to the
+	// usual rules (pref 32766 and 32767, ie. main and default).
+
+	return rg.ErrAcc
 }
 
-// delBypassrule removes the policy routing rule that avoids
+// delBypassrule removes the policy routing rules that avoid
 // tailscaled routing loops, if it exists.
 func (r *linuxRouter) delBypassRule() error {
-	out, err := r.cmd.output("ip", "rule", "list", "priority", "10000")
+	// Error codes: 'ip rule' returns error code 2 if the rule is a
-	if err != nil {
+	// duplicate (add) or not found (del). It returns a different code
-		// Busybox ships an `ip` binary that doesn't understand
+	// for syntax errors. This is also true of busybox.
-		// uncommon rules. Try to detect this explicitly, and steer
+	rg := newRunGroup(2, r.cmd)
-		// the user towards the correct fix. See
+
-		// https://github.com/tailscale/tailscale/issues/368 for an
+	// When deleting rules, we want to be a bit specific (mention which
-		// example of this issue.
+	// table we were routing to) but not *too* specific (fwmarks, etc).
-		if bytes.Contains(out, []byte("ip: ignoring all arguments")) || bytes.Contains(out, []byte("does not take any arguments")) {
+	// That leaves us some flexibility to change these values in later
-			return errors.New("cannot list ip rules, `ip` appears to be the busybox implementation. Please install iproute2")
+	// versions without having ongoing hacks for every possible
-		}
+	// combination.
-		return fmt.Errorf("listing ip rules: %v\n%s", err, out)
+
-	}
+	// Delete old-style tailscale rules
-	if len(out) == 0 {
+	// (never released in a stable version, so we can drop this
-		// No rule exists.
+	// support eventually).
-		return nil
+	rg.Run(
-	}
+		"ip", "rule", "del",
-	// Approximate sanity check that the rule we're about to delete
+		"pref", "10000",
-	// looks like one that handles Tailscale's fwmark.
+		"table", "main",
-	if !bytes.Contains(out, []byte(" fwmark "+tailscaleBypassMark)) {
+	)
-		return fmt.Errorf("ip rule 10000 doesn't look like a Tailscale policy rule: %q", string(out))
+
-	}
+	// Delete new-style tailscale rules.
-	return r.cmd.run("ip", "rule", "del", "priority", "10000")
+	rg.Run(
+		"ip", "rule", "del",
+		"pref", "8810",
+		"table", "main",
+	)
+	rg.Run(
+		"ip", "rule", "del",
+		"pref", "8830",
+		"table", "default",
+	)
+	rg.Run(
+		"ip", "rule", "del",
+		"pref", "8850",
+		"type", "unreachable",
+	)
+	rg.Run(
+		"ip", "rule", "del",
+		"pref", "8888",
+		"table", "88",
+	)
+	return rg.ErrAcc
 }
 
 // addNetfilterBase adds custom Tailscale chains to netfilter, along

wgengine/router/router_linux_test.go

@@ -33,6 +33,12 @@ func mustCIDRs(ss ...string) []netaddr.IPPrefix {
 }
 
 func TestRouterStates(t *testing.T) {
+	basic := `
+ip rule add pref 8810 fwmark 0x20000 table main
+ip rule add pref 8830 fwmark 0x20000 table default
+ip rule add pref 8850 fwmark 0x20000 type unreachable
+ip rule add pref 8888 table 88
+`
 	states := []struct {
 		name string
 		in   *Config
@@ -42,9 +48,7 @@ func TestRouterStates(t *testing.T) {
 			name: "no config",
 			in:   nil,
 			want: `
-up
+up` + basic,
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
-`,
 		},
 		{
 			name: "local addr only",
@@ -54,9 +58,7 @@ ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10
 			},
 			want: `
 up
-ip addr add 100.101.102.103/10 dev tailscale0
+ip addr add 100.101.102.103/10 dev tailscale0` + basic,
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
-`,
 		},
 
 		{
@@ -69,10 +71,8 @@ ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10
 			want: `
 up
 ip addr add 100.101.102.103/10 dev tailscale0
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88
-ip route add 192.168.16.0/24 dev tailscale0 scope global
+ip route add 192.168.16.0/24 dev tailscale0 table 88` + basic,
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
-`,
 		},
 
 		{
@@ -86,10 +86,8 @@ ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10
 			want: `
 up
 ip addr add 100.101.102.103/10 dev tailscale0
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88
-ip route add 192.168.16.0/24 dev tailscale0 scope global
+ip route add 192.168.16.0/24 dev tailscale0 table 88` + basic,
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
-`,
 		},
 
 		{
@@ -104,20 +102,19 @@ ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10
 			want: `
 up
 ip addr add 100.101.102.104/10 dev tailscale0
-ip route add 10.0.0.0/8 dev tailscale0 scope global
+ip route add 10.0.0.0/8 dev tailscale0 table 88
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic +
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
+				`filter/FORWARD -j ts-forward
-filter/FORWARD -j ts-forward
 filter/INPUT -j ts-input
 filter/ts-forward -o tailscale0 -s 200.0.0.0/8 -j ACCEPT
-filter/ts-forward -i tailscale0 -d 200.0.0.0/8 -j MARK --set-mark 0x10000/0x10000
+filter/ts-forward -i tailscale0 -d 200.0.0.0/8 -j MARK --set-mark 0x10000
-filter/ts-forward -m mark --mark 0x10000/0x10000 -j ACCEPT
+filter/ts-forward -m mark --mark 0x10000 -j ACCEPT
 filter/ts-forward -i tailscale0 -j DROP
 filter/ts-input -i lo -s 100.101.102.104 -j ACCEPT
 filter/ts-input ! -i tailscale0 -s 100.115.92.0/23 -j RETURN
 filter/ts-input ! -i tailscale0 -s 100.64.0.0/10 -j DROP
 nat/POSTROUTING -j ts-postrouting
-nat/ts-postrouting -m mark --mark 0x10000/0x10000 -j MASQUERADE
+nat/ts-postrouting -m mark --mark 0x10000 -j MASQUERADE
 `,
 		},
 		{
@@ -130,12 +127,11 @@ nat/ts-postrouting -m mark --mark 0x10000/0x10000 -j MASQUERADE
 			want: `
 up
 ip addr add 100.101.102.104/10 dev tailscale0
-ip route add 10.0.0.0/8 dev tailscale0 scope global
+ip route add 10.0.0.0/8 dev tailscale0 table 88
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic +
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
+				`filter/FORWARD -j ts-forward
-filter/FORWARD -j ts-forward
 filter/INPUT -j ts-input
-filter/ts-forward -m mark --mark 0x10000/0x10000 -j ACCEPT
+filter/ts-forward -m mark --mark 0x10000 -j ACCEPT
 filter/ts-forward -i tailscale0 -j DROP
 filter/ts-input -i lo -s 100.101.102.104 -j ACCEPT
 filter/ts-input ! -i tailscale0 -s 100.115.92.0/23 -j RETURN
@@ -156,14 +152,13 @@ nat/POSTROUTING -j ts-postrouting
 			want: `
 up
 ip addr add 100.101.102.104/10 dev tailscale0
-ip route add 10.0.0.0/8 dev tailscale0 scope global
+ip route add 10.0.0.0/8 dev tailscale0 table 88
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic +
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
+				`filter/FORWARD -j ts-forward
-filter/FORWARD -j ts-forward
 filter/INPUT -j ts-input
 filter/ts-forward -o tailscale0 -s 200.0.0.0/8 -j ACCEPT
-filter/ts-forward -i tailscale0 -d 200.0.0.0/8 -j MARK --set-mark 0x10000/0x10000
+filter/ts-forward -i tailscale0 -d 200.0.0.0/8 -j MARK --set-mark 0x10000
-filter/ts-forward -m mark --mark 0x10000/0x10000 -j ACCEPT
+filter/ts-forward -m mark --mark 0x10000 -j ACCEPT
 filter/ts-forward -i tailscale0 -j DROP
 filter/ts-input -i lo -s 100.101.102.104 -j ACCEPT
 filter/ts-input ! -i tailscale0 -s 100.115.92.0/23 -j RETURN
@@ -181,12 +176,11 @@ nat/POSTROUTING -j ts-postrouting
 			want: `
 up
 ip addr add 100.101.102.104/10 dev tailscale0
-ip route add 10.0.0.0/8 dev tailscale0 scope global
+ip route add 10.0.0.0/8 dev tailscale0 table 88
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic +
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
+				`filter/FORWARD -j ts-forward
-filter/FORWARD -j ts-forward
 filter/INPUT -j ts-input
-filter/ts-forward -m mark --mark 0x10000/0x10000 -j ACCEPT
+filter/ts-forward -m mark --mark 0x10000 -j ACCEPT
 filter/ts-forward -i tailscale0 -j DROP
 filter/ts-input -i lo -s 100.101.102.104 -j ACCEPT
 filter/ts-input ! -i tailscale0 -s 100.115.92.0/23 -j RETURN
@@ -205,10 +199,9 @@ nat/POSTROUTING -j ts-postrouting
 			want: `
 up
 ip addr add 100.101.102.104/10 dev tailscale0
-ip route add 10.0.0.0/8 dev tailscale0 scope global
+ip route add 10.0.0.0/8 dev tailscale0 table 88
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic +
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
+				`filter/ts-forward -m mark --mark 0x10000 -j ACCEPT
-filter/ts-forward -m mark --mark 0x10000/0x10000 -j ACCEPT
 filter/ts-forward -i tailscale0 -j DROP
 filter/ts-input -i lo -s 100.101.102.104 -j ACCEPT
 filter/ts-input ! -i tailscale0 -s 100.115.92.0/23 -j RETURN
@@ -216,7 +209,7 @@ filter/ts-input ! -i tailscale0 -s 100.64.0.0/10 -j DROP
 `,
 		},
 		{
-			name: "addr and routes with netfilter",
+			name: "addr and routes with netfilter2",
 			in: &Config{
 				LocalAddrs:    mustCIDRs("100.101.102.104/10"),
 				Routes:        mustCIDRs("100.100.100.100/32", "10.0.0.0/8"),
@@ -225,12 +218,11 @@ filter/ts-input ! -i tailscale0 -s 100.64.0.0/10 -j DROP
 			want: `
 up
 ip addr add 100.101.102.104/10 dev tailscale0
-ip route add 10.0.0.0/8 dev tailscale0 scope global
+ip route add 10.0.0.0/8 dev tailscale0 table 88
-ip route add 100.100.100.100/32 dev tailscale0 scope global
+ip route add 100.100.100.100/32 dev tailscale0 table 88` + basic +
-ip rule add fwmark 0x20000/0x20000 priority 10000 table main suppress_ifgroup 10000
+				`filter/FORWARD -j ts-forward
-filter/FORWARD -j ts-forward
 filter/INPUT -j ts-input
-filter/ts-forward -m mark --mark 0x10000/0x10000 -j ACCEPT
+filter/ts-forward -m mark --mark 0x10000 -j ACCEPT
 filter/ts-forward -i tailscale0 -j DROP
 filter/ts-input -i lo -s 100.101.102.104 -j ACCEPT
 filter/ts-input ! -i tailscale0 -s 100.115.92.0/23 -j RETURN
@@ -453,9 +445,9 @@ func (o *fakeOS) run(args ...string) error {
 	case "link":
 		got := strings.Join(args[2:], " ")
 		switch got {
-		case "set dev tailscale0 group 10000 up":
+		case "set dev tailscale0 up":
 			o.up = true
-		case "set dev tailscale0 group 0 down":
+		case "set dev tailscale0 down":
 			o.up = false
 		default:
 			return unexpected()
@@ -491,8 +483,19 @@ func (o *fakeOS) run(args ...string) error {
 			}
 		}
 		if !found {
-			o.t.Errorf("can't delete %q, not present", rest)
+			o.t.Logf("note: can't delete %q, not present", rest)
-			return errors.New("not present")
+			// 'ip rule del' exits with code 2 when a row is
+			// missing. We don't want to consider that an error,
+			// for cleanup purposes.
+
+			// TODO(apenwarr): this is a hack.
+			// I'd like to return an exec.ExitError(2) here, but
+			// I can't, because the ExitCode is implemented in
+			// os.ProcessState, which is an opaque object I can't
+			// instantiate or modify. Go's 75 levels of abstraction
+			// between me and an 8-bit int are really paying off
+			// here, as you can see.
+			return errors.New("exitcode:2")
 		}
 	default:
 		return unexpected()