Check the tun mtu against the kernel on reload, not the last config

overlay/tun_linux.go

@@ -255,7 +255,7 @@ func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error {
 		return err
 	}
 
-	if !initial && !routeChange && !c.HasChanged("tun.mtu") {
+	if !initial && !routeChange && !c.HasChanged("tun.mtu") && t.deviceHasConfiguredMTU() {
 		return nil
 	}
 
@@ -280,8 +280,6 @@ func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error {
 	}
 	t.routesFromSystemLock.Unlock()
 
-	oldDefaultMTU := t.DefaultMTU
-	oldMaxMTU := t.MaxMTU
 	newDefaultMTU := getMTU(t.l, c)
 	newMaxMTU := newDefaultMTU
 	for i, r := range routes {
@@ -302,20 +300,27 @@ func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error {
 	t.routeTree.Store(routeTree)
 
 	if !initial {
-		if oldMaxMTU != newMaxMTU {
-			t.setMTU(r)
-			t.l.Info("Set max MTU", "mtu", t.MaxMTU, "oldMTU", oldMaxMTU)
+		// Compare with the device rather than the last config, the mtu may have been refused or changed under us
+		if !t.deviceHasMaxMTU() {
+			oldMTU, _ := t.deviceMTU()
+			if err := t.setMTU(t.MaxMTU); err != nil {
+				t.l.Error("Failed to set tun mtu", "mtu", t.MaxMTU, "error", err)
+				r.Failed()
+			} else {
+				t.l.Info("Set max MTU", "mtu", t.MaxMTU, "oldMTU", oldMTU)
+			}
 		}
 
-		if oldDefaultMTU != newDefaultMTU {
-			for i := range t.vpnNetworks {
-				err := t.setDefaultRoute(t.vpnNetworks[i])
-				if err != nil {
-					t.l.Error(err.Error())
-					r.Failed()
-				} else {
-					t.l.Info("Set default MTU", "mtu", t.DefaultMTU, "oldMTU", oldDefaultMTU)
-				}
+		for _, network := range t.vpnNetworks {
+			if t.defaultRouteHasMTU(network) {
+				continue
+			}
+			err := t.setDefaultRoute(network)
+			if err != nil {
+				t.l.Error(err.Error())
+				r.Failed()
+			} else {
+				t.l.Info("Set default MTU", "mtu", t.DefaultMTU, "network", network)
 			}
 		}
 
@@ -482,7 +487,10 @@ func (t *tun) Activate() error {
 	t.deviceIndex = link.Attrs().Index
 
 	// Setup our default MTU
-	t.setMTU(nil)
+	if err := t.setMTU(t.MaxMTU); err != nil {
+		// This is currently a non fatal condition because the route table must have the MTU set appropriately as well
+		t.l.Error("Failed to set tun mtu", "error", err)
+	}
 
 	// Set the transmit queue length
 	ifrq := ifreqQLEN{Name: devName, Value: int32(t.TXQueueLen)}
@@ -527,14 +535,72 @@ func (t *tun) Activate() error {
 	return nil
 }
 
-func (t *tun) setMTU(r *config.Reload) {
-	// Set the MTU on the device
-	ifm := ifreqMTU{Name: t.deviceBytes(), MTU: int32(t.MaxMTU)}
-	if err := ioctl(t.ioctlFd, unix.SIOCSIFMTU, uintptr(unsafe.Pointer(&ifm))); err != nil {
-		// This is currently a non fatal condition because the route table must have the MTU set appropriately as well
-		t.l.Error("Failed to set tun mtu", "error", err)
-		r.Failed()
+func (t *tun) setMTU(mtu int) error {
+	ifm := ifreqMTU{Name: t.deviceBytes(), MTU: int32(mtu)}
+	return ioctl(t.ioctlFd, unix.SIOCSIFMTU, uintptr(unsafe.Pointer(&ifm)))
+}
+
+// deviceMTU asks the kernel for the device's mtu.
+func (t *tun) deviceMTU() (int, error) {
+	ifm := ifreqMTU{Name: t.deviceBytes()}
+	if err := ioctl(t.ioctlFd, unix.SIOCGIFMTU, uintptr(unsafe.Pointer(&ifm))); err != nil {
+		return 0, err
+	}
+	return int(ifm.MTU), nil
+}
+
+// deviceHasConfiguredMTU is true when the device and the route for each of our networks carry the mtu the config asks
+// for.
+func (t *tun) deviceHasConfiguredMTU() bool {
+	if !t.deviceHasMaxMTU() {
+		return false
+	}
+	for _, network := range t.vpnNetworks {
+		if !t.defaultRouteHasMTU(network) {
+			return false
+		}
+	}
+	return true
+}
+
+// defaultRouteHasMTU is true when the kernel's route for network on our device has the default mtu and advmss. Before
+// Activate there's nothing to check, Activate installs it.
+func (t *tun) defaultRouteHasMTU(network netip.Prefix) bool {
+	if t.deviceIndex == 0 {
+		return true
+	}
+	family := netlink.FAMILY_V4
+	if network.Addr().Is6() {
+		family = netlink.FAMILY_V6
+	}
+	filter := &netlink.Route{
+		LinkIndex: t.deviceIndex,
+		Dst: &net.IPNet{
+			IP:   network.Masked().Addr().AsSlice(),
+			Mask: net.CIDRMask(network.Bits(), network.Addr().BitLen()),
+		},
+		Table: unix.RT_TABLE_MAIN,
+	}
+	routes, err := netlink.RouteListFiltered(family, filter, netlink.RT_FILTER_OIF|netlink.RT_FILTER_DST|netlink.RT_FILTER_TABLE)
+	if err != nil {
+		return false
+	}
+	for _, r := range routes {
+		if r.MTU == t.DefaultMTU && r.AdvMSS == t.advMSS(Route{}) {
+			return true
+		}
+	}
+	return false
+}
+
+// deviceHasMaxMTU is true when the device's mtu is the one the config asks for. Before Activate there's nothing to
+// check, Activate sets it.
+func (t *tun) deviceHasMaxMTU() bool {
+	if t.ioctlFd == 0 {
+		return true
 	}
+	mtu, err := t.deviceMTU()
+	return err == nil && mtu == t.MaxMTU
 }
 
 func (t *tun) setDefaultRoute(cidr netip.Prefix) error {

overlay/tun_reload_linux_test.go

@@ -7,11 +7,13 @@ import (
 	"os"
 	"testing"
 
+	"github.com/DefinedNet/netlink"
 	"github.com/slackhq/nebula/config"
 	"github.com/slackhq/nebula/routing"
 	"github.com/slackhq/nebula/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
 )
 
 // Only tun.mtu changes. The system calls fail without a real device, what matters is the routes nebula keeps
@@ -76,7 +78,7 @@ func TestReloadRouteFailures(t *testing.T) {
 			tn := &tun{
 				l:                test.NewLogger(),
 				vpnNetworks:      []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")},
-				ioctlFd:          ^uintptr(0),
+				ioctlFd:          0, // not activated, the mtu is not checked
 				routesFromSystem: map[netip.Prefix]routing.Gateways{},
 			}
 			require.NoError(t, tn.reload(c, true, nil))
@@ -105,3 +107,83 @@ func TestTunReloadRestartKeys(t *testing.T) {
 	require.NoError(t, res.Err)
 	assert.ElementsMatch(t, []string{"tun.disabled", "tun.dev", "tun.tx_queue"}, res.RestartKeys)
 }
+
+// There is no device behind this tun, so it refuses every mtu. Repeating the reload tries again rather than reporting
+// it done
+func TestReloadRefusedMaxMTURetries(t *testing.T) {
+	c := config.NewC(test.NewLogger())
+	require.NoError(t, c.LoadString("tun:\n  mtu: 1300\n"))
+	tn := &tun{
+		l:                test.NewLogger(),
+		vpnNetworks:      []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")},
+		ioctlFd:          ^uintptr(0),
+		routesFromSystem: map[netip.Prefix]routing.Gateways{},
+	}
+	require.NoError(t, tn.reload(c, true, nil))
+	c.RegisterReloadCallback(func(c *config.C, r *config.Reload) {
+		require.NoError(t, tn.reload(c, false, r))
+	})
+
+	for range 2 {
+		res := c.ReloadConfigString("tun:\n  mtu: 1400\n")
+		require.NoError(t, res.Err)
+		assert.True(t, res.Failed)
+	}
+}
+
+// A reload puts the configured mtu back on the device and our network's route when they were changed outside nebula
+func TestTunReloadRestoresMTU(t *testing.T) {
+	c := config.NewC(test.NewLogger())
+	require.NoError(t, c.LoadString("tun:\n  dev: nebmtu0\n  mtu: 1300\n"))
+	tn, err := newTun(c, test.NewLogger(), []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, false)
+	if err != nil {
+		t.Skipf("can't create a tun device here: %v", err)
+	}
+	t.Cleanup(func() { _ = tn.Close() })
+	require.NoError(t, tn.Activate())
+
+	link, err := netlink.LinkByName(tn.Device)
+	require.NoError(t, err)
+	require.NoError(t, netlink.LinkSetMTU(link, 1280))
+	network := &netlink.Route{LinkIndex: tn.deviceIndex, Table: unix.RT_TABLE_MAIN}
+	routeMTU := func() int {
+		routes, err := netlink.RouteListFiltered(netlink.FAMILY_V4, network, netlink.RT_FILTER_OIF|netlink.RT_FILTER_TABLE)
+		require.NoError(t, err)
+		for _, r := range routes {
+			if r.Dst != nil && r.Dst.String() == "10.1.0.0/24" {
+				return r.MTU
+			}
+		}
+		t.Fatal("no route for our network")
+		return 0
+	}
+	require.Equal(t, 1300, routeMTU())
+	setRouteMTU := func(mtu int) {
+		routes, err := netlink.RouteListFiltered(netlink.FAMILY_V4, network, netlink.RT_FILTER_OIF|netlink.RT_FILTER_TABLE)
+		require.NoError(t, err)
+		for _, r := range routes {
+			if r.Dst != nil && r.Dst.String() == "10.1.0.0/24" {
+				r.MTU = mtu
+				require.NoError(t, netlink.RouteReplace(&r))
+			}
+		}
+		require.Equal(t, mtu, routeMTU())
+	}
+	reload := func() {
+		res := c.ReloadConfigString("tun:\n  dev: nebmtu0\n  mtu: 1300\n")
+		require.NoError(t, res.Err)
+		assert.False(t, res.Failed)
+	}
+
+	setRouteMTU(1280)
+	reload()
+	mtu, err := tn.deviceMTU()
+	require.NoError(t, err)
+	assert.Equal(t, 1300, mtu)
+	assert.Equal(t, 1300, routeMTU())
+
+	// Only the route this time
+	setRouteMTU(1280)
+	reload()
+	assert.Equal(t, 1300, routeMTU())
+}