Merge "Open and close clat bpf map while clat is starting and stoping" am: e01d2733bc

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/2085871

Change-Id: I70496679dafafa8856ffed16e4cb874c123a5308
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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<ClatIngress6Key, ClatIngress6Value> mIngressMap;
+    private BpfMap<ClatIngress6Key, ClatIngress6Value> mIngressMap = null;
     @Nullable
-    private final IBpfMap<ClatEgress4Key, ClatEgress4Value> mEgressMap;
+    private BpfMap<ClatEgress4Key, ClatEgress4Value> mEgressMap = null;
     @Nullable
     private ClatdTracker mClatdTracker = null;
 
@@ -247,7 +248,7 @@ public class ClatCoordinator {
 
         /** Get ingress6 BPF map. */
         @Nullable
-        public IBpfMap<ClatIngress6Key, ClatIngress6Value> getBpfIngress6Map() {
+        public BpfMap<ClatIngress6Key, ClatIngress6Value> 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<ClatEgress4Key, ClatEgress4Value> getBpfEgress4Map() {
+        public BpfMap<ClatEgress4Key, ClatEgress4Value> 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.
      */

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<ClatIngress6Key, ClatIngress6Value> mIngressMap;
-    @Mock private IBpfMap<ClatEgress4Key, ClatEgress4Value> mEgressMap;
+    @Mock private BpfMap<ClatIngress6Key, ClatIngress6Value> mIngressMap;
+    @Mock private BpfMap<ClatEgress4Key, ClatEgress4Value> 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<ClatIngress6Key, ClatIngress6Value> getBpfIngress6Map() {
+        public BpfMap<ClatIngress6Key, ClatIngress6Value> getBpfIngress6Map() {
             return mIngressMap;
         }
 
         /** Get egress4 BPF map. */
         @Override
-        public IBpfMap<ClatEgress4Key, ClatEgress4Value> getBpfEgress4Map() {
+        public BpfMap<ClatEgress4Key, ClatEgress4Value> 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));