From 6c7c63efa8ec955cfbde5f1d11fd9665b5d4f5f6 Mon Sep 17 00:00:00 2001 From: Tomas Hruby Date: Wed, 14 Aug 2024 19:18:36 -0700 Subject: [PATCH] [BPF] fix applying and removing log filters * do not apply an empty filter (matches all) if there is no filter for that device * do not apply any filters if there the mode is not debug * do not pre-allocate filter jumps indexes before deciding whether we will apply filters (hence do not take up space if no filter is applied) * clean up state when a filter is removed. Do not incorrectly program the index in tc program config if there is no filter left. --- felix/bpf/ut/attach_test.go | 91 ++++++++++++++++++++++++ felix/dataplane/linux/bpf_ep_mgr.go | 74 +++++++++++++------ felix/dataplane/linux/bpf_ep_mgr_test.go | 48 ++++++++----- 3 files changed, 174 insertions(+), 39 deletions(-) diff --git a/felix/bpf/ut/attach_test.go b/felix/bpf/ut/attach_test.go index 6fa7b780186..4e9f22018bd 100644 --- a/felix/bpf/ut/attach_test.go +++ b/felix/bpf/ut/attach_test.go @@ -815,6 +815,97 @@ func TestRepeatedAttach(t *testing.T) { } } +func TestLogFilters(t *testing.T) { + RegisterTestingT(t) + + bpfmaps, err := bpfmap.CreateBPFMaps(false) + Expect(err).NotTo(HaveOccurred()) + + commonMaps := bpfmaps.CommonMaps + + cfg := linux.Config{ + Hostname: "uthost", + BPFLogLevel: "debug", + BPFDataIfacePattern: regexp.MustCompile("^hostep[12]"), + VXLANMTU: 1000, + VXLANPort: 1234, + BPFNodePortDSREnabled: false, + RulesConfig: rules.Config{ + EndpointToHostAction: "RETURN", + }, + BPFExtToServiceConnmark: 0, + FeatureGates: map[string]string{ + "BPFConnectTimeLoadBalancingWorkaround": "enabled", + }, + BPFPolicyDebugEnabled: true, + BPFLogFilters: map[string]string{"hostep1": "tcp"}, + } + + bpfEpMgr, err := linux.NewTestEpMgr( + &cfg, + bpfmaps, + regexp.MustCompile("^workloadep[0123]"), + ) + Expect(err).NotTo(HaveOccurred()) + + host1 := createVethName("hostep1") + defer deleteLink(host1) + + workload0 := createVethName("workloadep0") + defer deleteLink(workload0) + + t.Run("load filter", func(t *testing.T) { + bpfEpMgr.OnUpdate(linux.NewIfaceStateUpdate("hostep1", ifacemonitor.StateUp, host1.Attrs().Index)) + bpfEpMgr.OnUpdate(linux.NewIfaceStateUpdate("workloadep0", ifacemonitor.StateUp, workload0.Attrs().Index)) + bpfEpMgr.OnUpdate(linux.NewIfaceAddrsUpdate("hostep1", "1.2.3.4")) + bpfEpMgr.OnUpdate(&proto.HostMetadataUpdate{Hostname: "uthost", Ipv4Addr: "1.2.3.4"}) + err = bpfEpMgr.CompleteDeferredWork() + Expect(err).NotTo(HaveOccurred()) + + ifstateMap := ifstateMapDump(commonMaps.IfStateMap) + + Expect(ifstateMap).To(HaveKey(ifstate.NewKey(uint32(host1.Attrs().Index)))) + hostep1State := ifstateMap[ifstate.NewKey(uint32(host1.Attrs().Index))] + Expect(hostep1State.TcIngressFilter()).NotTo(Equal(-1)) + Expect(hostep1State.TcEgressFilter()).NotTo(Equal(-1)) + + Expect(ifstateMap).To(HaveKey(ifstate.NewKey(uint32(workload0.Attrs().Index)))) + wl0State := ifstateMap[ifstate.NewKey(uint32(workload0.Attrs().Index))] + Expect(wl0State.TcIngressFilter()).To(Equal(-1)) + Expect(wl0State.TcEgressFilter()).To(Equal(-1)) + }) + + cfg.BPFLogLevel = "off" + + bpfEpMgr, err = linux.NewTestEpMgr( + &cfg, + bpfmaps, + regexp.MustCompile("^workloadep[0123]"), + ) + Expect(err).NotTo(HaveOccurred()) + + t.Run("after restart, load filter", func(t *testing.T) { + bpfEpMgr.OnUpdate(linux.NewIfaceStateUpdate("hostep1", ifacemonitor.StateUp, host1.Attrs().Index)) + bpfEpMgr.OnUpdate(linux.NewIfaceStateUpdate("workloadep0", ifacemonitor.StateUp, workload0.Attrs().Index)) + bpfEpMgr.OnUpdate(linux.NewIfaceAddrsUpdate("hostep1", "1.2.3.4")) + bpfEpMgr.OnUpdate(&proto.HostMetadataUpdate{Hostname: "uthost", Ipv4Addr: "1.2.3.4"}) + err = bpfEpMgr.CompleteDeferredWork() + Expect(err).NotTo(HaveOccurred()) + + ifstateMap := ifstateMapDump(commonMaps.IfStateMap) + + Expect(ifstateMap).To(HaveKey(ifstate.NewKey(uint32(host1.Attrs().Index)))) + hostep1State := ifstateMap[ifstate.NewKey(uint32(host1.Attrs().Index))] + Expect(hostep1State.TcIngressFilter()).To(Equal(-1)) + Expect(hostep1State.TcEgressFilter()).To(Equal(-1)) + + Expect(ifstateMap).To(HaveKey(ifstate.NewKey(uint32(workload0.Attrs().Index)))) + wl0State := ifstateMap[ifstate.NewKey(uint32(workload0.Attrs().Index))] + Expect(wl0State.TcIngressFilter()).To(Equal(-1)) + Expect(wl0State.TcEgressFilter()).To(Equal(-1)) + }) +} + func ifstateMapDump(m maps.Map) ifstate.MapMem { ifstateMap := make(ifstate.MapMem) ifstateMapIter := ifstate.MapMemIter(ifstateMap) diff --git a/felix/dataplane/linux/bpf_ep_mgr.go b/felix/dataplane/linux/bpf_ep_mgr.go index a37d482aad9..f0416400830 100644 --- a/felix/dataplane/linux/bpf_ep_mgr.go +++ b/felix/dataplane/linux/bpf_ep_mgr.go @@ -946,6 +946,7 @@ func (m *bpfEndpointManager) reclaimFilterIdx(name string, iface *bpfInterface) if err := m.jumpMapAlloc.Put(iface.dpState.filterIdx[attachHook], name); err != nil { log.WithError(err).Errorf("Filter hook %s", attachHook) } + iface.dpState.filterIdx[attachHook] = -1 } } @@ -984,6 +985,7 @@ func (m *bpfEndpointManager) updateIfaceStateMap(name string, iface *bpfInterfac if iface.dpState.v6Readiness != ifaceNotReady { flags |= ifstate.FlgIPv6Ready } + v := ifstate.NewValue(flags, name, iface.dpState.v4.policyIdx[hook.XDP], iface.dpState.v4.policyIdx[hook.Ingress], @@ -1725,7 +1727,6 @@ func (m *bpfEndpointManager) doApplyPolicyToDataIface(iface, masterIface string, ) m.ifacesLock.Lock() - ifaceName := iface m.withIface(iface, func(iface *bpfInterface) bool { up = iface.info.ifaceIsUp() state = iface.dpState @@ -1736,9 +1737,6 @@ func (m *bpfEndpointManager) doApplyPolicyToDataIface(iface, masterIface string, log.WithField("iface", iface).Debug("Ignoring interface that is down") return state, nil } - if err := m.dataIfaceStateFillJumps(ifaceName, attachXDPOnly, &state); err != nil { - return state, err - } hepIface := iface if !attachXDPOnly { @@ -1762,6 +1760,10 @@ func (m *bpfEndpointManager) doApplyPolicyToDataIface(iface, masterIface string, var xdpAP4, xdpAP6 *xdp.AttachPoint tcAttachPoint := m.calculateTCAttachPoint(iface) + if err := m.dataIfaceStateFillJumps(tcAttachPoint, attachXDPOnly, &state); err != nil { + return state, err + } + xdpAttachPoint := &xdp.AttachPoint{ AttachPoint: bpf.AttachPoint{ Hook: hook.XDP, @@ -2089,12 +2091,12 @@ func (m *bpfEndpointManager) allocJumpIndicesForDataIface(ifaceName string, atta return nil } -func (m *bpfEndpointManager) wepStateFillJumps(ifaceName string, state *bpfInterfaceState) error { +func (m *bpfEndpointManager) wepStateFillJumps(ap *tc.AttachPoint, state *bpfInterfaceState) error { var err error // Allocate indices for IPv4 if m.v4 != nil { - err = m.allocJumpIndicesForWEP(ifaceName, &state.v4) + err = m.allocJumpIndicesForWEP(ap.IfaceName(), &state.v4) if err != nil { return err } @@ -2102,59 +2104,80 @@ func (m *bpfEndpointManager) wepStateFillJumps(ifaceName string, state *bpfInter // Allocate indices for IPv6 if m.v6 != nil { - err = m.allocJumpIndicesForWEP(ifaceName, &state.v6) + err = m.allocJumpIndicesForWEP(ap.IfaceName(), &state.v6) if err != nil { return err } } - if m.bpfLogLevel == "debug" { + if ap.LogLevel == "debug" { if state.filterIdx[hook.Ingress] == -1 { - state.filterIdx[hook.Ingress], err = m.jumpMapAlloc.Get(ifaceName) + state.filterIdx[hook.Ingress], err = m.jumpMapAlloc.Get(ap.IfaceName()) if err != nil { return err } } if state.filterIdx[hook.Egress] == -1 { - state.filterIdx[hook.Egress], err = m.jumpMapAlloc.Get(ifaceName) + state.filterIdx[hook.Egress], err = m.jumpMapAlloc.Get(ap.IfaceName()) if err != nil { return err } } + } else { + for _, attachHook := range []hook.Hook{hook.Ingress, hook.Egress} { + if err := m.jumpMapDelete(attachHook, state.filterIdx[attachHook]); err != nil { + log.WithError(err).Warn("Filter program may leak.") + } + if err := m.jumpMapAlloc.Put(state.filterIdx[attachHook], ap.IfaceName()); err != nil { + log.WithError(err).Errorf("Filter hook %s", attachHook) + } + state.filterIdx[attachHook] = -1 + } } + return nil } -func (m *bpfEndpointManager) dataIfaceStateFillJumps(ifaceName string, attachXDPOnly bool, state *bpfInterfaceState) error { +func (m *bpfEndpointManager) dataIfaceStateFillJumps(ap *tc.AttachPoint, attachXDPOnly bool, state *bpfInterfaceState) error { var err error if m.v4 != nil { - err = m.allocJumpIndicesForDataIface(ifaceName, attachXDPOnly, &state.v4) + err = m.allocJumpIndicesForDataIface(ap.IfaceName(), attachXDPOnly, &state.v4) if err != nil { return err } } if m.v6 != nil { - err = m.allocJumpIndicesForDataIface(ifaceName, attachXDPOnly, &state.v6) + err = m.allocJumpIndicesForDataIface(ap.IfaceName(), attachXDPOnly, &state.v6) if err != nil { return err } } - if m.bpfLogLevel == "debug" { + if ap.LogLevel == "debug" { if state.filterIdx[hook.Ingress] == -1 { - state.filterIdx[hook.Ingress], err = m.jumpMapAlloc.Get(ifaceName) + state.filterIdx[hook.Ingress], err = m.jumpMapAlloc.Get(ap.IfaceName()) if err != nil { return err } } if state.filterIdx[hook.Egress] == -1 { - state.filterIdx[hook.Egress], err = m.jumpMapAlloc.Get(ifaceName) + state.filterIdx[hook.Egress], err = m.jumpMapAlloc.Get(ap.IfaceName()) if err != nil { return err } } + } else { + for _, attachHook := range []hook.Hook{hook.Ingress, hook.Egress} { + if err := m.jumpMapDelete(attachHook, state.filterIdx[attachHook]); err != nil { + log.WithError(err).Warn("Filter program may leak.") + } + if err := m.jumpMapAlloc.Put(state.filterIdx[attachHook], ap.IfaceName()); err != nil { + log.WithError(err).Errorf("Filter hook %s", attachHook) + } + state.filterIdx[attachHook] = -1 + } } return nil } @@ -2191,10 +2214,6 @@ func (m *bpfEndpointManager) doApplyPolicy(ifaceName string) (bpfInterfaceState, return state, nil } - if err := m.wepStateFillJumps(ifaceName, &state); err != nil { - return state, err - } - // Otherwise, the interface appears to be present but we may or may not have an endpoint from the // datastore. If we don't have an endpoint then we'll attach a program to block traffic and we'll // get the jump map ready to insert the policy if the endpoint shows up. @@ -2246,6 +2265,10 @@ func (m *bpfEndpointManager) doApplyPolicy(ifaceName string) (bpfInterfaceState, ap := m.calculateTCAttachPoint(ifaceName) + if err := m.wepStateFillJumps(ap, &state); err != nil { + return state, err + } + if m.v6 != nil { wg.Add(1) go func() { @@ -2753,7 +2776,11 @@ func (polDirection PolDirection) Inverse() PolDirection { } func (m *bpfEndpointManager) apLogFilter(ap *tc.AttachPoint, iface string) (string, string) { - if m.logFilters == nil { + if len(m.logFilters) == 0 { + return m.bpfLogLevel, "" + } + + if m.bpfLogLevel != "debug" { return m.bpfLogLevel, "" } @@ -2761,21 +2788,26 @@ func (m *bpfEndpointManager) apLogFilter(ap *tc.AttachPoint, iface string) (stri if !ok { if ap.Type == tcdefs.EpTypeWorkload { if exp, ok := m.logFilters["weps"]; ok { + log.WithField("iface", iface).Debugf("Log filter for weps: %s", exp) return m.bpfLogLevel, exp } } if ap.Type == tcdefs.EpTypeHost { if exp, ok := m.logFilters["heps"]; ok { + log.WithField("iface", iface).Debugf("Log filter for heps: %s", exp) return m.bpfLogLevel, exp } } if exp, ok := m.logFilters["all"]; ok { + log.WithField("iface", iface).Debugf("Log filter for all: %s", exp) return m.bpfLogLevel, exp } + log.WithField("iface", iface).Debug("No log filter") return "off", "" } + log.WithField("iface", iface).Debugf("Log filter: %s", exp) return m.bpfLogLevel, exp } diff --git a/felix/dataplane/linux/bpf_ep_mgr_test.go b/felix/dataplane/linux/bpf_ep_mgr_test.go index 095a8191d07..986861082cb 100644 --- a/felix/dataplane/linux/bpf_ep_mgr_test.go +++ b/felix/dataplane/linux/bpf_ep_mgr_test.go @@ -117,6 +117,7 @@ func (m *mockDataplane) loadDefaultPolicies() error { func (m *mockDataplane) ensureProgramAttached(ap attachPoint) (qDiscInfo, error) { m.mutex.Lock() defer m.mutex.Unlock() + var qdisc qDiscInfo key := ap.IfaceName() + ":" + ap.HookName().String() m.numAttaches[key] = m.numAttaches[key] + 1 @@ -1355,6 +1356,10 @@ var _ = Describe("BPF Endpoint Manager", func() { } It("should reclaim indexes for active interfaces", func() { + + bpfEpMgr.bpfLogLevel = "debug" + bpfEpMgr.logFilters = map[string]string{"all": "tcp"} + for i := 0; i < 8; i++ { _ = jumpMap.Update(jump.Key(i), jump.Value(uint32(1000+i))) _ = jumpMap.Update(jump.Key(i+jump.TCMaxEntryPoints), jump.Value(uint32(1000+i))) @@ -1396,17 +1401,17 @@ var _ = Describe("BPF Endpoint Manager", func() { 123: value123.String(), })) - // Expect clean-up deletions but no value changes due to mocking. - Expect(dumpJumpMap(jumpMap)).To(Equal(map[int]int{ - 0: 1000, - 2: 1002, - 3: 1003, - 4: 1004, - 10000: 1000, - 10002: 1002, - 10003: 1003, - 10004: 1004, - })) + // Expect clean-up deletions. + jmps := dumpJumpMap(jumpMap) + Expect(jmps).To(HaveLen(8)) + Expect(jmps).To(HaveKey(0)) /* filters reloaded to reflect current expressions */ + Expect(jmps).To(HaveKey(2)) + Expect(jmps).To(HaveKey(3)) + Expect(jmps).To(HaveKey(4)) + Expect(jmps).To(HaveKeyWithValue(10000, 1000)) + Expect(jmps).To(HaveKeyWithValue(10002, 1002)) + Expect(jmps).To(HaveKeyWithValue(10003, 1003)) + Expect(jmps).To(HaveKeyWithValue(10004, 1004)) Expect(dumpJumpMap(xdpJumpMap)).To(Equal(map[int]int{ 1: 2001, })) @@ -1725,6 +1730,9 @@ var _ = Describe("BPF Endpoint Manager", func() { newBpfEpMgr(true) }) It("should clean up jump map entries for missing interfaces", func() { + bpfEpMgr.bpfLogLevel = "debug" + bpfEpMgr.logFilters = map[string]string{"all": "tcp"} + for i := 0; i < 17; i++ { _ = jumpMap.Update(jump.Key(i), jump.Value(uint32(1000+i))) _ = jumpMap.Update(jump.Key(i+jump.TCMaxEntryPoints), jump.Value(uint32(1000+i))) @@ -1789,6 +1797,9 @@ var _ = Describe("BPF Endpoint Manager", func() { } It("should reclaim indexes for active interfaces", func() { + bpfEpMgr.bpfLogLevel = "debug" + bpfEpMgr.logFilters = map[string]string{"all": "tcp"} + for i := 0; i < 8; i++ { _ = jumpMap.Update(jump.Key(i), jump.Value(uint32(1000+i))) _ = jumpMap.Update(jump.Key(i+jump.TCMaxEntryPoints), jump.Value(uint32(1000+i))) @@ -1831,13 +1842,14 @@ var _ = Describe("BPF Endpoint Manager", func() { 123: value123.String(), })) - // Expect clean-up deletions but no value changes due to mocking. - Expect(dumpJumpMap(jumpMap)).To(Equal(map[int]int{ - 2: 1002, - 3: 1003, - 10002: 1002, - 10003: 1003, - })) + // Expect clean-up deletions. + jmps := dumpJumpMap(jumpMap) + Expect(jmps).To(HaveLen(5)) + Expect(jmps).To(HaveKey(2)) /* filters reloaded to reflect current expressions */ + Expect(jmps).To(HaveKey(3)) + Expect(jmps).To(HaveKey(4)) + Expect(jmps).To(HaveKeyWithValue(10002, 1002)) + Expect(jmps).To(HaveKeyWithValue(10003, 1003)) Expect(dumpJumpMap(xdpJumpMap)).To(Equal(map[int]int{ 1: 2001, }))