diff --git a/overlay/tun_linux.go b/overlay/tun_linux.go index 541ef6b8..fd84ece1 100644 --- a/overlay/tun_linux.go +++ b/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 { diff --git a/overlay/tun_reload_linux_test.go b/overlay/tun_reload_linux_test.go index de358ebf..4cd6574a 100644 --- a/overlay/tun_reload_linux_test.go +++ b/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()) +}