From 7219cfeeff4c49763c9458c3abd23584b4947936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Mon, 11 Apr 2022 17:17:11 -0700 Subject: remove specific clat iptables drop rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit these are now obsoleted by the CLATMARK based ebpf + ip6tables logic (generated via removing clat_raw_PREROUTING and everything that referenced it) Test: builds, TreeHugger, flashed an oriole, observed ping 8.8.8.8 behaviour on GoogleGuest v6-only network Signed-off-by: Maciej Żenczykowski Change-Id: I1bccfed0dfa6bd7f211979294da29884142481dc --- server/BandwidthController.cpp | 4 +--- server/ClatdController.cpp | 20 ++------------------ server/ClatdController.h | 4 ---- server/ClatdControllerTest.cpp | 22 ---------------------- server/Controllers.cpp | 1 - server/ControllersTest.cpp | 2 -- 6 files changed, 3 insertions(+), 50 deletions(-) diff --git a/server/BandwidthController.cpp b/server/BandwidthController.cpp index 27739418..96a82e23 100644 --- a/server/BandwidthController.cpp +++ b/server/BandwidthController.cpp @@ -238,9 +238,7 @@ std::vector getBasicAccountingCommands() { // interface and later correct for overhead (+20 bytes/packet). // // Note: eBPF offloaded packets never hit base interface's ip6tables, and non - // offloaded packets (which when using xt_qtaguid means all packets, because - // clat eBPF offload does not work on xt_qtaguid devices) are dropped in - // clat_raw_PREROUTING. + // offloaded packets are dropped up above due to being marked with CLAT_MARK // // Hence we will never double count and additional corrections are not needed. // We can simply take the sum of base and stacked (+20B/pkt) interface counts. diff --git a/server/ClatdController.cpp b/server/ClatdController.cpp index 07e28c2b..bf51d9e0 100644 --- a/server/ClatdController.cpp +++ b/server/ClatdController.cpp @@ -71,7 +71,6 @@ static const in_addr kV4Addr = {inet_addr(kV4AddrString)}; static const int kV4AddrLen = 29; using android::base::Result; -using android::base::StringPrintf; using android::base::unique_fd; using android::bpf::BpfMap; using android::netdutils::DumpWriter; @@ -318,17 +317,6 @@ void ClatdController::maybeStartBpf(const ClatdTracker& tracker) { // success } -void ClatdController::setIptablesDropRule(bool add, const char* iface, const char* pfx96Str, - const char* v6Str) { - std::string cmd = StringPrintf( - "*raw\n" - "%s %s -i %s -s %s/96 -d %s -j DROP\n" - "COMMIT\n", - (add ? "-A" : "-D"), LOCAL_RAW_PREROUTING, iface, pfx96Str, v6Str); - - iptablesRestoreFunction(V6, cmd); -} - void ClatdController::maybeStopBpf(const ClatdTracker& tracker) { int rv = tcFilterDelDevIngressClatIpv6(tracker.ifIndex); if (rv < 0) { @@ -805,17 +793,14 @@ int ClatdController::startClatd(const std::string& interface, const std::string& return -res; } - // 14. add the drop rule for iptables. - setIptablesDropRule(true, tracker.iface, tracker.pfx96String, tracker.v6Str); - - // 15. actually perform vfork/dup2/execve + // 14. actually perform vfork/dup2/execve res = posix_spawn(&tracker.pid, kClatdPath, &fa, &attr, (char* const*)args, nullptr); if (res) { ALOGE("posix_spawn failed (%s)", strerror(res)); return -res; } - // 16. configure eBPF offload - if possible + // 15. configure eBPF offload - if possible maybeStartBpf(tracker); mClatdTrackers[interface] = tracker; @@ -840,7 +825,6 @@ int ClatdController::stopClatd(const std::string& interface) { ::stopProcess(tracker->pid, "clatd"); - setIptablesDropRule(false, tracker->iface, tracker->pfx96String, tracker->v6Str); mClatdTrackers.erase(interface); ALOGD("clatd on %s stopped", interface.c_str()); diff --git a/server/ClatdController.h b/server/ClatdController.h index 04694705..74690ff4 100644 --- a/server/ClatdController.h +++ b/server/ClatdController.h @@ -52,8 +52,6 @@ class ClatdController { void dump(netdutils::DumpWriter& dw) EXCLUDES(mutex); - static constexpr const char LOCAL_RAW_PREROUTING[] = "clat_raw_PREROUTING"; - // Public struct ClatdTracker and tun_data for testing. gtest/TEST_F macro changes the class // name. In TEST_F(ClatdControllerTest..), can't access struct ClatdTracker and tun_data. // TODO: probably use gtest/FRIEND_TEST macro. @@ -102,8 +100,6 @@ class ClatdController { void maybeStartBpf(const ClatdTracker& tracker) REQUIRES(mutex); void maybeStopBpf(const ClatdTracker& tracker) REQUIRES(mutex); - void setIptablesDropRule(bool add, const char* iface, const char* pfx96Str, const char* v6Str) - REQUIRES(mutex); int detect_mtu(const struct in6_addr* plat_subnet, uint32_t plat_suffix, uint32_t mark); int configure_interface(struct ClatdTracker* tracker, struct tun_data* tunnel) REQUIRES(mutex); diff --git a/server/ClatdControllerTest.cpp b/server/ClatdControllerTest.cpp index 2c904df4..d44e092d 100644 --- a/server/ClatdControllerTest.cpp +++ b/server/ClatdControllerTest.cpp @@ -71,10 +71,6 @@ class ClatdControllerTest : public IptablesBaseTest { protected: ClatdController mClatdCtrl; - void setIptablesDropRule(bool a, const char* b, const char* c, const char* d) { - std::lock_guard guard(mClatdCtrl.mutex); - return mClatdCtrl.setIptablesDropRule(a, b, c, d); - } void setIpv4AddressFreeFunc(bool (*func)(in_addr_t)) { ClatdController::isIpv4AddressFreeFunc = func; } @@ -200,24 +196,6 @@ TEST_F(ClatdControllerTest, MakeChecksumNeutral) { EXPECT_GE(3210000, onebits); } -TEST_F(ClatdControllerTest, AddIptablesRule) { - setIptablesDropRule(true, "wlan0", "64:ff9b::", "2001:db8::1:2:3:4"); - expectIptablesRestoreCommands((ExpectedIptablesCommands){ - {V6, - "*raw\n" - "-A clat_raw_PREROUTING -i wlan0 -s 64:ff9b::/96 -d 2001:db8::1:2:3:4 -j DROP\n" - "COMMIT\n"}}); -} - -TEST_F(ClatdControllerTest, RemoveIptablesRule) { - setIptablesDropRule(false, "wlan0", "64:ff9b::", "2001:db8::a:b:c:d"); - expectIptablesRestoreCommands((ExpectedIptablesCommands){ - {V6, - "*raw\n" - "-D clat_raw_PREROUTING -i wlan0 -s 64:ff9b::/96 -d 2001:db8::a:b:c:d -j DROP\n" - "COMMIT\n"}}); -} - TEST_F(ClatdControllerTest, DetectMtu) { // ::1 with bottom 32 bits set to 1 is still ::1 which routes via lo with mtu of 64KiB ASSERT_EQ(detect_mtu(&in6addr_loopback, htonl(1), 0 /*MARK_UNSET*/), 65536); diff --git a/server/Controllers.cpp b/server/Controllers.cpp index 7eef04c0..62828346 100644 --- a/server/Controllers.cpp +++ b/server/Controllers.cpp @@ -75,7 +75,6 @@ static const std::vector FILTER_OUTPUT = { static const std::vector RAW_PREROUTING = { IdletimerController::LOCAL_RAW_PREROUTING, - ClatdController::LOCAL_RAW_PREROUTING, BandwidthController::LOCAL_RAW_PREROUTING, TetherController::LOCAL_RAW_PREROUTING, }; diff --git a/server/ControllersTest.cpp b/server/ControllersTest.cpp index e6487e33..9033a1d3 100644 --- a/server/ControllersTest.cpp +++ b/server/ControllersTest.cpp @@ -98,8 +98,6 @@ TEST_F(ControllersTest, TestInitIptablesRules) { "-F PREROUTING\n" ":idletimer_raw_PREROUTING -\n" "-A PREROUTING -j idletimer_raw_PREROUTING\n" - ":clat_raw_PREROUTING -\n" - "-A PREROUTING -j clat_raw_PREROUTING\n" ":bw_raw_PREROUTING -\n" "-A PREROUTING -j bw_raw_PREROUTING\n" ":tetherctrl_raw_PREROUTING -\n" -- cgit v1.2.3