diff --git a/lib/network/allocate.go b/lib/network/allocate.go index 02d9005bb..62faeb8ee 100644 --- a/lib/network/allocate.go +++ b/lib/network/allocate.go @@ -50,6 +50,19 @@ func (m *manager) CreateAllocation(ctx context.Context, req AllocateRequest) (*N return nil, err } + // Addresses are reused. Drop what the host remembers about this one so the + // first packet to the new guest is resolved, not sent to the previous + // owner's MAC until that entry times out. + _, neighborSpanEnd := startNetworkStep(ctx, "network.forget_neighbor", + attribute.String("operation", "forget_neighbor"), + attribute.String("instance_id", req.InstanceID), + ) + err = m.forgetNeighbor(network.Bridge, netConfig.IP) + neighborSpanEnd(err) + if err != nil { + log.WarnContext(ctx, "failed to forget neighbor entry for allocated address", "ip", netConfig.IP, "error", err) + } + // Create the TAP synchronously because the VMM restore depends on it. tapCtx, tapSpanEnd := startNetworkStep(ctx, "network.create_tap", attribute.String("operation", "create_tap"), diff --git a/lib/network/bridge_darwin.go b/lib/network/bridge_darwin.go index 50799f43c..f4c559a46 100644 --- a/lib/network/bridge_darwin.go +++ b/lib/network/bridge_darwin.go @@ -58,6 +58,11 @@ func (m *manager) tapDeviceExists(tapName string) bool { return true } +// forgetNeighbor is a no-op on macOS: vmnet owns the host side of the network. +func (m *manager) forgetNeighbor(bridgeName, ip string) error { + return nil +} + // queryNetworkState returns the default vmnet shared-NAT network targeted by // Hypeman. Host-level Shared_Net_Address overrides are not currently supported. func (m *manager) queryNetworkState(bridgeName string) (*Network, error) { diff --git a/lib/network/bridge_linux.go b/lib/network/bridge_linux.go index 469b56f30..4e72245f0 100644 --- a/lib/network/bridge_linux.go +++ b/lib/network/bridge_linux.go @@ -1093,6 +1093,24 @@ func (m *manager) tapDeviceExists(tapName string) bool { return err == nil } +// forgetNeighbor removes the host's neighbor entry for a guest address on the +// bridge. A missing entry is not an error. +func (m *manager) forgetNeighbor(bridgeName, ip string) error { + addr := net.ParseIP(ip).To4() + if addr == nil { + return fmt.Errorf("parse IPv4 address %q", ip) + } + bridge, err := netlink.LinkByName(bridgeName) + if err != nil { + return fmt.Errorf("get bridge: %w", err) + } + err = netlink.NeighDel(&netlink.Neigh{LinkIndex: bridge.Attrs().Index, Family: netlink.FAMILY_V4, IP: addr}) + if err != nil && !errors.Is(err, unix.ENOENT) { + return fmt.Errorf("delete neighbor entry: %w", err) + } + return nil +} + // queryNetworkState queries kernel for bridge state. func (m *manager) queryNetworkState(bridgeName string) (*Network, error) { link, err := netlink.LinkByName(bridgeName) diff --git a/lib/network/bridge_linux_test.go b/lib/network/bridge_linux_test.go index df3f9264e..192ffe2fa 100644 --- a/lib/network/bridge_linux_test.go +++ b/lib/network/bridge_linux_test.go @@ -3,9 +3,15 @@ package network import ( + "context" "errors" + "fmt" + "net" + "os" "testing" + "github.com/kernel/hypeman/cmd/api/config" + "github.com/kernel/hypeman/lib/paths" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/vishvananda/netlink" @@ -107,3 +113,116 @@ func TestRetryNetlinkDumpGivesUpAfterBudget(t *testing.T) { require.ErrorIs(t, err, netlink.ErrDumpInterrupted) assert.Equal(t, netlinkDumpRetryCount, calls) } + +func TestForgetNeighborDropsReusedAddressEntry(t *testing.T) { + bridge := newNeighborTestBridge(t) + ip := net.ParseIP("198.51.100.7").To4() + addStaleNeighbor(t, bridge, ip) + require.True(t, hasUsableNeighbor(t, bridge, ip)) + + m := &manager{} + require.NoError(t, m.forgetNeighbor(bridge.Name, ip.String())) + assert.False(t, hasUsableNeighbor(t, bridge, ip)) + + require.NoError(t, m.forgetNeighbor(bridge.Name, ip.String()), "a missing entry is not an error") + assert.Error(t, m.forgetNeighbor("hm-no-such-bridge", ip.String())) + assert.Error(t, m.forgetNeighbor(bridge.Name, "not-an-address")) +} + +// A new guest must not inherit the neighbor entry of whichever guest held its +// address before: the host would keep sending to the old MAC. +func TestCreateAllocationForgetsNeighborOfReusedAddress(t *testing.T) { + bridge := newNeighborTestBridge(t) + const subnet = "198.51.100.0/29" + var candidates []net.IP + for last := byte(2); last <= 6; last++ { + ip := net.IPv4(198, 51, 100, last).To4() + candidates = append(candidates, ip) + addStaleNeighbor(t, bridge, ip) + } + + m := &manager{ + paths: paths.New(t.TempDir()), + config: &config.Config{Network: config.NetworkConfig{BridgeName: bridge.Name}}, + pendingAllocations: make(map[string]pendingAllocation), + } + m.setDefaultNetwork(newDefaultNetwork(bridge.Name, subnet, "198.51.100.1")) + + // The TAP is named after the first eight characters of the instance ID. + instanceID := fmt.Sprintf("n%07d", os.Getpid()) + allocated, err := m.CreateAllocation(context.Background(), AllocateRequest{InstanceID: instanceID, InstanceName: instanceID}) + require.NoError(t, err) + t.Cleanup(func() { + if tap, err := netlink.LinkByName(allocated.TAPDevice); err == nil { + _ = netlink.LinkDel(tap) + } + }) + + for _, ip := range candidates { + assert.Equal(t, ip.String() != allocated.IP, hasUsableNeighbor(t, bridge, ip), "neighbor entry for %s", ip) + } +} + +func newNeighborTestBridge(t *testing.T) *netlink.Bridge { + t.Helper() + if os.Geteuid() != 0 { + t.Skip("managing neighbor entries requires CAP_NET_ADMIN") + } + bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: fmt.Sprintf("hmn%d", os.Getpid())}} + require.NoError(t, netlink.LinkAdd(bridge)) + t.Cleanup(func() { _ = netlink.LinkDel(bridge) }) + + // The kernel drops every neighbor entry of a bridge whose address changes + // or that has no carrier when a port is added. Pin the address and give + // the bridge a running port, like a host with guests has, so only the + // entry under test goes away. The TAP has carrier while this process + // holds it open. + address, err := net.ParseMAC("02:00:00:00:00:01") + require.NoError(t, err) + require.NoError(t, netlink.LinkSetHardwareAddr(bridge, address)) + port := &netlink.Tuntap{ + LinkAttrs: netlink.LinkAttrs{Name: fmt.Sprintf("hmp%d", os.Getpid())}, + Mode: netlink.TUNTAP_MODE_TAP, + Queues: 1, + NonPersist: true, + } + require.NoError(t, netlink.LinkAdd(port)) + t.Cleanup(func() { + for _, fd := range port.Fds { + _ = fd.Close() + } + }) + require.NoError(t, netlink.LinkSetMaster(port, bridge)) + require.NoError(t, netlink.LinkSetUp(port)) + require.NoError(t, netlink.LinkSetUp(bridge)) + return bridge +} + +func addStaleNeighbor(t *testing.T, bridge *netlink.Bridge, ip net.IP) { + t.Helper() + previousOwner, err := net.ParseMAC("02:00:00:aa:bb:cc") + require.NoError(t, err) + require.NoError(t, netlink.NeighSet(&netlink.Neigh{ + LinkIndex: bridge.Attrs().Index, + Family: netlink.FAMILY_V4, + State: netlink.NUD_STALE, + IP: ip, + HardwareAddr: previousOwner, + })) +} + +func hasUsableNeighbor(t *testing.T, bridge *netlink.Bridge, ip net.IP) bool { + t.Helper() + var neighbors []netlink.Neigh + require.NoError(t, retryNetlinkDump(func() error { + var err error + neighbors, err = netlink.NeighList(bridge.Attrs().Index, netlink.FAMILY_V4) + return err + })) + for _, neighbor := range neighbors { + if neighbor.IP.Equal(ip) && neighbor.State != netlink.NUD_FAILED { + return true + } + } + return false +}