Revert "Open and close clat bpf map while clat is starting and stoping" am: b135921ed7

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

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

service/src/com/android/server/connectivity/ClatCoordinator.java

@@ -39,6 +39,7 @@ 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;
@@ -115,12 +116,10 @@ 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 BpfMap<ClatIngress6Key, ClatIngress6Value> mIngressMap = null;
+    private final IBpfMap<ClatIngress6Key, ClatIngress6Value> mIngressMap;
     @Nullable
-    private BpfMap<ClatEgress4Key, ClatEgress4Value> mEgressMap = null;
+    private final IBpfMap<ClatEgress4Key, ClatEgress4Value> mEgressMap;
     @Nullable
     private ClatdTracker mClatdTracker = null;
 
@@ -248,7 +247,7 @@ public class ClatCoordinator {
 
         /** Get ingress6 BPF map. */
         @Nullable
-        public BpfMap<ClatIngress6Key, ClatIngress6Value> getBpfIngress6Map() {
+        public IBpfMap<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.
@@ -265,7 +264,7 @@ public class ClatCoordinator {
 
         /** Get egress4 BPF map. */
         @Nullable
-        public BpfMap<ClatEgress4Key, ClatEgress4Value> getBpfEgress4Map() {
+        public IBpfMap<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.
@@ -374,35 +373,12 @@ public class ClatCoordinator {
     public ClatCoordinator(@NonNull Dependencies deps) {
         mDeps = deps;
         mNetd = mDeps.getNetd();
-    }
-
-    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;
+        mIngressMap = mDeps.getBpfIngress6Map();
+        mEgressMap = mDeps.getBpfEgress4Map();
     }
 
     private void maybeStartBpf(final ClatdTracker tracker) {
-        mEgressMap = mDeps.getBpfEgress4Map();
-        if (mEgressMap == null) return;
-
-        mIngressMap = mDeps.getBpfIngress6Map();
-        if (mIngressMap == null) {
-            closeEgressMap();
-            return;
-        }
+        if (mIngressMap == null || mEgressMap == null) return;
 
         final boolean isEthernet;
         try {
@@ -746,13 +722,6 @@ 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();
     }
 
     /**
@@ -821,7 +790,7 @@ public class ClatCoordinator {
     }
 
     /**
-     * Dump the cordinator information. Only called when clat is started. See Nat464Xlat#dump.
+     * Dump the cordinator information.
      *
      * @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.BpfMap;
+import com.android.net.module.util.IBpfMap;
 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 BpfMap<ClatIngress6Key, ClatIngress6Value> mIngressMap;
-    @Mock private BpfMap<ClatEgress4Key, ClatEgress4Value> mEgressMap;
+    @Mock private IBpfMap<ClatIngress6Key, ClatIngress6Value> mIngressMap;
+    @Mock private IBpfMap<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 BpfMap<ClatIngress6Key, ClatIngress6Value> getBpfIngress6Map() {
+        public IBpfMap<ClatIngress6Key, ClatIngress6Value> getBpfIngress6Map() {
             return mIngressMap;
         }
 
         /** Get egress4 BPF map. */
         @Override
-        public BpfMap<ClatEgress4Key, ClatEgress4Value> getBpfEgress4Map() {
+        public IBpfMap<ClatEgress4Key, ClatEgress4Value> getBpfEgress4Map() {
             return mEgressMap;
         }
 
@@ -447,8 +447,6 @@ 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));
@@ -471,8 +469,6 @@ 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));