From d7cb0d42113fae1975a536d8540a69f2bab6fd1a Mon Sep 17 00:00:00 2001 From: Tomas Hruby Date: Mon, 17 Jun 2024 14:16:55 -0700 Subject: [PATCH 1/2] [BPF/UT] set RPF to strict as test provision for that. Make tests execute the path again after we introduced the option to disable RPF. --- felix/bpf/ut/bpf_prog_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/felix/bpf/ut/bpf_prog_test.go b/felix/bpf/ut/bpf_prog_test.go index 4d3c28a26bf..ad2beb7d019 100644 --- a/felix/bpf/ut/bpf_prog_test.go +++ b/felix/bpf/ut/bpf_prog_test.go @@ -760,7 +760,7 @@ func objLoad(fname, bpfFsDir, ipFamily string, topts testOpts, polProg, hasHostC VxlanPort: testVxlanPort, PSNatStart: uint16(topts.psnaStart), PSNatLen: uint16(topts.psnatEnd-topts.psnaStart) + 1, - Flags: libbpf.GlobalsNoDSRCidrs, + Flags: libbpf.GlobalsNoDSRCidrs | libbpf.GlobalsRPFOptionStrict, LogFilterJmp: 0xffffffff, } From 327c4fdd8607080479e5fc022c880dc26e386720 Mon Sep 17 00:00:00 2001 From: Tomas Hruby Date: Mon, 17 Jun 2024 15:00:00 -0700 Subject: [PATCH 2/2] [BPF] skip FIB when dest is unknown at HEP * fix when CALI_ST_SKIP_FIB is set on the way to the host, set CALI_CT_FLAG_SKIP_FIB on conntrack - not just when from WEP * add test for ^^^ and issue https://github.com/projectcalico/calico/issues/6450 * In addition to skipping FIB when there is no route to post-dnat destination, also skip FIB when there is a route, but it is not local while there was no service involved. In that case, we are not forwarding a service (NodePort) to another node and we should only forward locally. Let the host decide what to do with such a packet. Fixes https://github.com/projectcalico/calico/issues/8918 --- felix/bpf-gpl/tc.c | 12 ++- felix/bpf/ut/unknown_dest_test.go | 125 ++++++++++++++++++++++++++++++ felix/bpf/ut/whitelist_test.go | 4 +- 3 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 felix/bpf/ut/unknown_dest_test.go diff --git a/felix/bpf-gpl/tc.c b/felix/bpf-gpl/tc.c index f7f4ab66b4e..50e84680fca 100644 --- a/felix/bpf-gpl/tc.c +++ b/felix/bpf-gpl/tc.c @@ -598,6 +598,16 @@ static CALI_BPF_INLINE void calico_tc_process_ct_lookup(struct cali_tc_ctx *ctx) goto skip_policy; } ctx->state->flags |= CALI_ST_DEST_IS_HOST; + } else if (CALI_F_FROM_HEP && !ctx->nat_dest && !cali_rt_is_local(dest_rt)) { + /* Disable FIB, let the packet go through the host after it is + * policed. It is ingress into the system and we got a packet, which is + * not for this host, and it wasn't resolved as a service and it is not + * for a local workload either. But we hit a route so it may be some L2 + * broadcast, we do not quite know. Let the host route it or dump it. + * + * https://github.com/projectcalico/calico/issues/8918 + */ + ctx->state->flags |= CALI_ST_SKIP_FIB; } if (CALI_F_TO_HEP && ctx->nat_dest && !skb_seen(ctx->skb) && !(ctx->state->flags & CALI_ST_HOST_PSNAT)) { @@ -1306,7 +1316,7 @@ int calico_tc_skb_new_flow_entrypoint(struct __sk_buff *skb) if (state->flags & CALI_ST_NAT_OUTGOING) { ct_ctx_nat->flags |= CALI_CT_FLAG_NAT_OUT; } - if (CALI_F_FROM_WEP && state->flags & CALI_ST_SKIP_FIB) { + if (CALI_F_TO_HOST && state->flags & CALI_ST_SKIP_FIB) { ct_ctx_nat->flags |= CALI_CT_FLAG_SKIP_FIB; } /* Packets received at WEP with CALI_CT_FLAG_SKIP_FIB mark signal diff --git a/felix/bpf/ut/unknown_dest_test.go b/felix/bpf/ut/unknown_dest_test.go new file mode 100644 index 00000000000..3a41db5a354 --- /dev/null +++ b/felix/bpf/ut/unknown_dest_test.go @@ -0,0 +1,125 @@ +// Copyright (c) 2024 Tigera, Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ut_test + +import ( + "testing" + + . "github.com/onsi/gomega" + + tcdefs "github.com/projectcalico/calico/felix/bpf/tc/defs" + + "github.com/projectcalico/calico/felix/bpf/routes" +) + +func TestUnknownEnterHostNoRoute(t *testing.T) { + RegisterTestingT(t) + + bpfIfaceName = "noRt" + defer func() { bpfIfaceName = "" }() + + _, _, _, _, pktBytes, err := testPacketUDPDefault() + Expect(err).NotTo(HaveOccurred()) + + resetCTMap(ctMap) // ensure it is clean + + hostIP = node1ip + + // Insert a reverse route for the source workload. + rtKey := routes.NewKey(srcV4CIDR).AsBytes() + rtVal := routes.NewValue(routes.FlagsRemoteWorkload | routes.FlagInIPAMPool).AsBytes() + err = rtMap.Update(rtKey, rtVal) + Expect(err).NotTo(HaveOccurred()) + defer resetRTMap(rtMap) + + skbMark = 0 + runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) { + res, err := bpfrun(pktBytes) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC)) + }) + + expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet. +} + +func TestUnknownEnterHostRouteNotLocal(t *testing.T) { + RegisterTestingT(t) + + bpfIfaceName = "noLo" + defer func() { bpfIfaceName = "" }() + + _, _, _, _, pktBytes, err := testPacketUDPDefault() + Expect(err).NotTo(HaveOccurred()) + + resetCTMap(ctMap) // ensure it is clean + + hostIP = node1ip + + // Insert a reverse route for the source workload. + rtKey := routes.NewKey(srcV4CIDR).AsBytes() + rtVal := routes.NewValue(routes.FlagsRemoteWorkload | routes.FlagInIPAMPool).AsBytes() + err = rtMap.Update(rtKey, rtVal) + Expect(err).NotTo(HaveOccurred()) + defer resetRTMap(rtMap) + + rtKey = routes.NewKey(dstV4CIDR).AsBytes() + rtVal = routes.NewValueWithIfIndex(routes.FlagsRemoteWorkload|routes.FlagInIPAMPool, 1).AsBytes() + err = rtMap.Update(rtKey, rtVal) + Expect(err).NotTo(HaveOccurred()) + + skbMark = 0 + runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) { + res, err := bpfrun(pktBytes) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC)) + }) + + expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet. + + // Next packet will follow conntrack, but must skip FIB as well. + skbMark = 0 + runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) { + res, err := bpfrun(pktBytes) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC)) + }) + + expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet. + + // Next packet will follow conntrack, but must skip FIB as well. + skbMark = 0 + runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) { + res, err := bpfrun(pktBytes) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC)) + }) + + expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet. + + resetCTMap(ctMap) + + rtVal = routes.NewValueWithIfIndex(routes.FlagsRemoteHost|routes.FlagInIPAMPool, 1).AsBytes() + err = rtMap.Update(rtKey, rtVal) + Expect(err).NotTo(HaveOccurred()) + + skbMark = 0 + runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) { + res, err := bpfrun(pktBytes) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC)) + }) + + expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet. +} diff --git a/felix/bpf/ut/whitelist_test.go b/felix/bpf/ut/whitelist_test.go index e003980b4e6..b81d4f9c6e3 100644 --- a/felix/bpf/ut/whitelist_test.go +++ b/felix/bpf/ut/whitelist_test.go @@ -116,7 +116,7 @@ func TestAllowEnterHostToWorkload(t *testing.T) { err = rtMap.Update(rtKey, rtVal) Expect(err).NotTo(HaveOccurred()) rtKey = routes.NewKey(dstV4CIDR).AsBytes() - rtVal = routes.NewValueWithIfIndex(routes.FlagsRemoteWorkload|routes.FlagInIPAMPool, 1).AsBytes() + rtVal = routes.NewValueWithIfIndex(routes.FlagsLocalWorkload|routes.FlagInIPAMPool, 1).AsBytes() err = rtMap.Update(rtKey, rtVal) Expect(err).NotTo(HaveOccurred()) defer resetRTMap(rtMap) @@ -338,7 +338,7 @@ func TestAllowEnterHostToWorkloadV6(t *testing.T) { err = rtMapV6.Update(rtKey, rtVal) Expect(err).NotTo(HaveOccurred()) rtKey = routes.NewKeyV6(dstV6CIDR).AsBytes() - rtVal = routes.NewValueV6WithIfIndex(routes.FlagsRemoteWorkload|routes.FlagInIPAMPool, 1).AsBytes() + rtVal = routes.NewValueV6WithIfIndex(routes.FlagsLocalWorkload|routes.FlagInIPAMPool, 1).AsBytes() err = rtMapV6.Update(rtKey, rtVal) Expect(err).NotTo(HaveOccurred()) defer resetRTMap(rtMapV6)