From c03323c8efa8b04ab1eb8b452909aab2d9e4c3ec Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Thu, 5 May 2022 13:49:24 +0800 Subject: [PATCH] Open and close clat bpf map while clat is starting and stoping BpfMap class supports AutoCloseable interface which closes file descriptor only in try-exit. BpfMap class doesn't close fds while the object is released. Change the timing of opening and closing bpf map file descriptors to clat is starting and stoping. Moreover, the reason that manual close BPF map file descriptors is as follows. Just don't rely on that GC releasing to close the file descriptors even if class BpfMap supports close file descriptor in finalize(). If the interfaces are added and removed quickly, too many unclosed file descriptors may cause unexpected problems. Bug: 230880517 Test: manual test Steps: 1. Connect to IPv6 only wifi (GoogleGuest) and mobile data 2. Check that map fds are appeared: /proc/$(system_server_pid)/fd/$(bpf_map_fd) $ adb shell ps | grep system_server system 1929 825 20311224 730060 do_epoll_wait 0 S system_server $ adb shell ls -all proc/1929/fd | grep bpf-map .. system system 64 2022-05-05 13:36:42 .. 331 -> anon_inode:bpf-map .. system system 64 2022-05-05 13:36:42 .. 348 -> anon_inode:bpf-map 3. Check the clat maps are added. $ adb shell dumpsys connectivity NetworkAgentInfo{network{105} handle{454377263117} ni{WIFI .. Nat464Xlat: .. Forwarding rules: BPF ingress map: iif nat64Prefix v6Addr -> v4Addr oif 47 /64:ff9b::/96 /2a00:79e1:abc:6f02:f182:6c29:ab56:9961 -> /192.0.0.4 62 BPF egress map: iif v4Addr -> v6Addr nat64Prefix oif 62 /192.0.0.4 -> /2a00:79e1:abc:6f02:f182:6c29:ab56:9961 /64:ff9b::/96 47 ether NetworkAgentInfo{network{106} handle{458672230413} ni{MOBILE[LTE] .. Nat464Xlat: 4. Disconnect from wifi 5. Check that map fds are disappeared: /proc/$(system_server_pid)/fd/$(bpf_map_fd) $ adb shell ls -all proc/1929/fd | grep bpf-map (fd 331 and 348 were not found) Change-Id: I60c0301bf00beae5cf5ab3535c6a3da68a2a4a9b --- .../server/connectivity/ClatCoordinator.java | 49 +++++++++++++++---- .../connectivity/ClatCoordinatorTest.java | 14 ++++-- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/service/src/com/android/server/connectivity/ClatCoordinator.java b/service/src/com/android/server/connectivity/ClatCoordinator.java index 4a7c77adf8..33494d44a3 100644 --- a/service/src/com/android/server/connectivity/ClatCoordinator.java +++ b/service/src/com/android/server/connectivity/ClatCoordinator.java @@ -39,7 +39,6 @@ import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.IndentingPrintWriter; import com.android.modules.utils.build.SdkLevel; import com.android.net.module.util.BpfMap; -import com.android.net.module.util.IBpfMap; import com.android.net.module.util.InterfaceParams; import com.android.net.module.util.TcUtils; import com.android.net.module.util.bpf.ClatEgress4Key; @@ -116,10 +115,12 @@ public class ClatCoordinator { private final INetd mNetd; @NonNull private final Dependencies mDeps; + // BpfMap objects {mIngressMap, mEgressMap} are initialized in #maybeStartBpf and closed in + // #maybeStopBpf. @Nullable - private final IBpfMap mIngressMap; + private BpfMap mIngressMap = null; @Nullable - private final IBpfMap mEgressMap; + private BpfMap mEgressMap = null; @Nullable private ClatdTracker mClatdTracker = null; @@ -247,7 +248,7 @@ public class ClatCoordinator { /** Get ingress6 BPF map. */ @Nullable - public IBpfMap getBpfIngress6Map() { + public BpfMap getBpfIngress6Map() { // Pre-T devices don't use ClatCoordinator to access clat map. Since Nat464Xlat // initializes a ClatCoordinator object to avoid redundant null pointer check // while using, ignore the BPF map initialization on pre-T devices. @@ -264,7 +265,7 @@ public class ClatCoordinator { /** Get egress4 BPF map. */ @Nullable - public IBpfMap getBpfEgress4Map() { + public BpfMap getBpfEgress4Map() { // Pre-T devices don't use ClatCoordinator to access clat map. Since Nat464Xlat // initializes a ClatCoordinator object to avoid redundant null pointer check // while using, ignore the BPF map initialization on pre-T devices. @@ -373,12 +374,35 @@ public class ClatCoordinator { public ClatCoordinator(@NonNull Dependencies deps) { mDeps = deps; mNetd = mDeps.getNetd(); - mIngressMap = mDeps.getBpfIngress6Map(); - mEgressMap = mDeps.getBpfEgress4Map(); + } + + private void closeEgressMap() { + try { + mEgressMap.close(); + } catch (ErrnoException e) { + Log.e(TAG, "Cannot close egress4 map: " + e); + } + mEgressMap = null; + } + + private void closeIngressMap() { + try { + mIngressMap.close(); + } catch (ErrnoException e) { + Log.e(TAG, "Cannot close ingress6 map: " + e); + } + mIngressMap = null; } private void maybeStartBpf(final ClatdTracker tracker) { - if (mIngressMap == null || mEgressMap == null) return; + mEgressMap = mDeps.getBpfEgress4Map(); + if (mEgressMap == null) return; + + mIngressMap = mDeps.getBpfIngress6Map(); + if (mIngressMap == null) { + closeEgressMap(); + return; + } final boolean isEthernet; try { @@ -722,6 +746,13 @@ public class ClatCoordinator { } catch (ErrnoException | IllegalStateException e) { Log.e(TAG, "Could not delete entry (" + rxKey + "): " + e); } + + // Manual close BPF map file descriptors. Just don't rely on that GC releasing to close + // the file descriptors even if class BpfMap supports close file descriptor in + // finalize(). If the interfaces are added and removed quickly, too many unclosed file + // descriptors may cause unexpected problem. + closeEgressMap(); + closeIngressMap(); } /** @@ -790,7 +821,7 @@ public class ClatCoordinator { } /** - * Dump the cordinator information. + * Dump the cordinator information. Only called when clat is started. See Nat464Xlat#dump. * * @param pw print writer. */ diff --git a/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java b/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java index f84d10f395..de5698fc4b 100644 --- a/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java +++ b/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java @@ -46,7 +46,7 @@ import android.os.ParcelFileDescriptor; import androidx.test.filters.SmallTest; -import com.android.net.module.util.IBpfMap; +import com.android.net.module.util.BpfMap; import com.android.net.module.util.bpf.ClatEgress4Key; import com.android.net.module.util.bpf.ClatEgress4Value; import com.android.net.module.util.bpf.ClatIngress6Key; @@ -123,8 +123,8 @@ public class ClatCoordinatorTest { @Mock private INetd mNetd; @Spy private TestDependencies mDeps = new TestDependencies(); - @Mock private IBpfMap mIngressMap; - @Mock private IBpfMap mEgressMap; + @Mock private BpfMap mIngressMap; + @Mock private BpfMap mEgressMap; /** * The dependency injection class is used to mock the JNI functions and system functions @@ -326,13 +326,13 @@ public class ClatCoordinatorTest { /** Get ingress6 BPF map. */ @Override - public IBpfMap getBpfIngress6Map() { + public BpfMap getBpfIngress6Map() { return mIngressMap; } /** Get egress4 BPF map. */ @Override - public IBpfMap getBpfEgress4Map() { + public BpfMap getBpfEgress4Map() { return mEgressMap; } @@ -447,6 +447,8 @@ public class ClatCoordinatorTest { argThat(fd -> Objects.equals(RAW_SOCK_PFD.getFileDescriptor(), fd)), eq(BASE_IFACE), eq(NAT64_PREFIX_STRING), eq(XLAT_LOCAL_IPV4ADDR_STRING), eq(XLAT_LOCAL_IPV6ADDR_STRING)); + inOrder.verify(mDeps).getBpfEgress4Map(); + inOrder.verify(mDeps).getBpfIngress6Map(); inOrder.verify(mEgressMap).insertEntry(eq(EGRESS_KEY), eq(EGRESS_VALUE)); inOrder.verify(mIngressMap).insertEntry(eq(INGRESS_KEY), eq(INGRESS_VALUE)); inOrder.verify(mDeps).tcQdiscAddDevClsact(eq(STACKED_IFINDEX)); @@ -469,6 +471,8 @@ public class ClatCoordinatorTest { eq((short) PRIO_CLAT), eq((short) ETH_P_IP)); inOrder.verify(mEgressMap).deleteEntry(eq(EGRESS_KEY)); inOrder.verify(mIngressMap).deleteEntry(eq(INGRESS_KEY)); + inOrder.verify(mEgressMap).close(); + inOrder.verify(mIngressMap).close(); inOrder.verify(mDeps).stopClatd(eq(BASE_IFACE), eq(NAT64_PREFIX_STRING), eq(XLAT_LOCAL_IPV4ADDR_STRING), eq(XLAT_LOCAL_IPV6ADDR_STRING), eq(CLATD_PID)); inOrder.verify(mDeps).untagSocket(eq(RAW_SOCK_COOKIE));