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));