From 2fde98c87b8134c3ebfc8d4be7074c782a258558 Mon Sep 17 00:00:00 2001 From: Wayne Ma Date: Mon, 17 Jan 2022 18:04:05 +0800 Subject: [PATCH 1/4] [NETD-TC#15] Make ConnectivityService and PermissionMonitor calls BpfNetMaps on T Since TrafficController moves to mainline module for T, so some netd binder interfaces revelant to BPF are going to deprecated. Provide JNI APIs to call TrafficController inside mainline module for T. Bug: 209935649 Test: atest CtsHostsideNetworkTests Change-Id: Ib3b43cf2840e02806395af9f1e019ca6fccd032e --- .../src/com/android/server/BpfNetMaps.java | 112 ++++++++++++- .../android/server/ConnectivityService.java | 47 ++++-- .../connectivity/PermissionMonitor.java | 28 ++-- .../com/android/server/BpfNetMapsTest.java | 86 ++++++++++ .../connectivity/PermissionMonitorTest.java | 155 ++++++++++-------- 5 files changed, 319 insertions(+), 109 deletions(-) create mode 100644 tests/unit/java/com/android/server/BpfNetMapsTest.java diff --git a/service/src/com/android/server/BpfNetMaps.java b/service/src/com/android/server/BpfNetMaps.java index 9d897883c2..e444a12b6a 100644 --- a/service/src/com/android/server/BpfNetMaps.java +++ b/service/src/com/android/server/BpfNetMaps.java @@ -16,6 +16,8 @@ package com.android.server; +import android.net.INetd; +import android.os.RemoteException; import android.os.ServiceSpecificException; import android.system.Os; import android.util.Log; @@ -27,10 +29,19 @@ import android.util.Log; */ public class BpfNetMaps { private static final String TAG = "BpfNetMaps"; + private final INetd mNetd; + // TODO: change USE_JNI to SdkLevel.isAtLeastT() + private static final boolean USE_JNI = false; static { - System.loadLibrary("traffic_controller_jni"); - native_init(); + if (USE_JNI) { + System.loadLibrary("traffic_controller_jni"); + native_init(); + } + } + + public BpfNetMaps(INetd netd) { + mNetd = netd; } /** @@ -41,6 +52,14 @@ public class BpfNetMaps { * cause of the failure. */ public void addNaughtyApp(final int uid) { + if (!USE_JNI) { + try { + mNetd.bandwidthAddNaughtyApp(uid); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return; + } final int err = native_addNaughtyApp(uid); if (err != 0) { throw new ServiceSpecificException(err, "Unable to add naughty app: " @@ -56,6 +75,14 @@ public class BpfNetMaps { * cause of the failure. */ public void removeNaughtyApp(final int uid) { + if (!USE_JNI) { + try { + mNetd.bandwidthRemoveNaughtyApp(uid); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return; + } final int err = native_removeNaughtyApp(uid); if (err != 0) { throw new ServiceSpecificException(err, "Unable to remove naughty app: " @@ -71,6 +98,14 @@ public class BpfNetMaps { * cause of the failure. */ public void addNiceApp(final int uid) { + if (!USE_JNI) { + try { + mNetd.bandwidthAddNiceApp(uid); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return; + } final int err = native_addNiceApp(uid); if (err != 0) { throw new ServiceSpecificException(err, "Unable to add nice app: " @@ -86,6 +121,14 @@ public class BpfNetMaps { * cause of the failure. */ public void removeNiceApp(final int uid) { + if (!USE_JNI) { + try { + mNetd.bandwidthRemoveNiceApp(uid); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return; + } final int err = native_removeNiceApp(uid); if (err != 0) { throw new ServiceSpecificException(err, "Unable to remove nice app: " @@ -102,6 +145,14 @@ public class BpfNetMaps { * cause of the failure. */ public void setChildChain(final int childChain, final boolean enable) { + if (!USE_JNI) { + try { + mNetd.firewallEnableChildChain(childChain, enable); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return; + } final int err = native_setChildChain(childChain, enable); if (err != 0) { throw new ServiceSpecificException(-err, "Unable to set child chain: " @@ -124,6 +175,14 @@ public class BpfNetMaps { */ public int replaceUidChain(final String chainName, final boolean isAllowlist, final int[] uids) { + if (!USE_JNI) { + try { + mNetd.firewallReplaceUidChain(chainName, isAllowlist, uids); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return 0; + } final int err = native_replaceUidChain(chainName, isAllowlist, uids); if (err != 0) { Log.e(TAG, "replaceUidChain failed: " + Os.strerror(-err)); @@ -140,8 +199,15 @@ public class BpfNetMaps { * @throws ServiceSpecificException in case of failure, with an error code indicating the * cause of the failure. */ - public void setUidRule(final int childChain, final int uid, - final int firewallRule) { + public void setUidRule(final int childChain, final int uid, final int firewallRule) { + if (!USE_JNI) { + try { + mNetd.firewallSetUidRule(childChain, uid, firewallRule); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return; + } final int err = native_setUidRule(childChain, uid, firewallRule); if (err != 0) { throw new ServiceSpecificException(-err, "Unable to set uid rule: " @@ -166,6 +232,14 @@ public class BpfNetMaps { * cause of the failure. */ public void addUidInterfaceRules(final String ifName, final int[] uids) { + if (!USE_JNI) { + try { + mNetd.firewallAddUidInterfaceRules(ifName, uids); + } catch (RemoteException e) { + Log.e(TAG, "Exception when updating permissions: " + e); + } + return; + } final int err = native_addUidInterfaceRules(ifName, uids); if (err != 0) { throw new ServiceSpecificException(err, "Unable to add uid interface rules: " @@ -184,6 +258,14 @@ public class BpfNetMaps { * cause of the failure. */ public void removeUidInterfaceRules(final int[] uids) { + if (!USE_JNI) { + try { + mNetd.firewallRemoveUidInterfaceRules(uids); + } catch (RemoteException e) { + Log.e(TAG, "Exception when updating permissions: " + e); + } + return; + } final int err = native_removeUidInterfaceRules(uids); if (err != 0) { throw new ServiceSpecificException(err, "Unable to remove uid interface rules: " @@ -197,6 +279,14 @@ public class BpfNetMaps { * cause of the failure. */ public void swapActiveStatsMap() { + if (!USE_JNI) { + try { + mNetd.trafficSwapActiveStatsMap(); + } catch (RemoteException e) { + throw new IllegalStateException(e); + } + return; + } final int err = native_swapActiveStatsMap(); if (err != 0) { throw new ServiceSpecificException(err, "Unable to swap active stats map: " @@ -213,8 +303,16 @@ public class BpfNetMaps { * revoke all permissions for the uids. * @param uids uid of users to grant permission */ - public void setNetPermForUids(final int permission, final int[] uids) { - native_setPermissionForUids(permission, uids); + public void setNetPermForUids(final int permissions, final int[] uids) { + if (!USE_JNI) { + try { + mNetd.trafficSetNetPermForUids(permissions, uids); + } catch (RemoteException e) { + Log.e(TAG, "Pass appId list of special permission failed." + e); + } + return; + } + native_setPermissionForUids(permissions, uids); } /** @@ -255,7 +353,7 @@ public class BpfNetMaps { private native int native_addUidInterfaceRules(String ifName, int[] uids); private native int native_removeUidInterfaceRules(int[] uids); private native int native_swapActiveStatsMap(); - private native void native_setPermissionForUids(int permission, int[] uids); + private native void native_setPermissionForUids(int permissions, int[] uids); private native int native_setCounterSet(int counterSet, int uid); private native int native_deleteTagData(int tag, int uid); } diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index fa7dfa9846..0259f7df8c 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -396,6 +396,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private NetworkStatsManager mStatsManager; private NetworkPolicyManager mPolicyManager; private final NetdCallback mNetdCallback; + private final BpfNetMaps mBpfNetMaps; /** * TestNetworkService (lazily) created upon first usage. Locked to prevent creation of multiple @@ -1335,6 +1336,15 @@ public class ConnectivityService extends IConnectivityManager.Stub return DeviceConfigUtils.isFeatureEnabled(context, NAMESPACE_CONNECTIVITY, name, TETHERING_MODULE_NAME, defaultEnabled); } + + /** + * Get the BpfNetMaps implementation to use in ConnectivityService. + * @param netd + * @return BpfNetMaps implementation. + */ + public BpfNetMaps getBpfNetMaps(INetd netd) { + return new BpfNetMaps(netd); + } } public ConnectivityService(Context context) { @@ -1403,6 +1413,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mProxyTracker = mDeps.makeProxyTracker(mContext, mHandler); mNetd = netd; + mBpfNetMaps = mDeps.getBpfNetMaps(netd); mTelephonyManager = (TelephonyManager) mContext.getSystemService(Context.TELEPHONY_SERVICE); mAppOpsManager = (AppOpsManager) mContext.getSystemService(Context.APP_OPS_SERVICE); mLocationPermissionChecker = mDeps.makeLocationPermissionChecker(mContext); @@ -1430,7 +1441,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mUserManager = (UserManager) context.getSystemService(Context.USER_SERVICE); - mPermissionMonitor = new PermissionMonitor(mContext, mNetd); + mPermissionMonitor = new PermissionMonitor(mContext, mNetd, mBpfNetMaps); mUserAllContext = mContext.createContextAsUser(UserHandle.ALL, 0 /* flags */); // Listen for user add/removes to inform PermissionMonitor. @@ -10709,11 +10720,11 @@ public class ConnectivityService extends IConnectivityManager.Stub try { if (add) { - mNetd.bandwidthAddNiceApp(uid); + mBpfNetMaps.addNiceApp(uid); } else { - mNetd.bandwidthRemoveNiceApp(uid); + mBpfNetMaps.removeNiceApp(uid); } - } catch (RemoteException | ServiceSpecificException e) { + } catch (ServiceSpecificException e) { throw new IllegalStateException(e); } } @@ -10724,11 +10735,11 @@ public class ConnectivityService extends IConnectivityManager.Stub try { if (add) { - mNetd.bandwidthAddNaughtyApp(uid); + mBpfNetMaps.addNaughtyApp(uid); } else { - mNetd.bandwidthRemoveNaughtyApp(uid); + mBpfNetMaps.removeNaughtyApp(uid); } - } catch (RemoteException | ServiceSpecificException e) { + } catch (ServiceSpecificException e) { throw new IllegalStateException(e); } } @@ -10738,9 +10749,9 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceNetworkStackOrSettingsPermission(); try { - mNetd.firewallSetUidRule(chain, uid, + mBpfNetMaps.setUidRule(chain, uid, allow ? INetd.FIREWALL_RULE_ALLOW : INetd.FIREWALL_RULE_DENY); - } catch (RemoteException | ServiceSpecificException e) { + } catch (ServiceSpecificException e) { throw new IllegalStateException(e); } } @@ -10750,8 +10761,8 @@ public class ConnectivityService extends IConnectivityManager.Stub enforceNetworkStackOrSettingsPermission(); try { - mNetd.firewallEnableChildChain(chain, enable); - } catch (RemoteException | ServiceSpecificException e) { + mBpfNetMaps.setChildChain(chain, enable); + } catch (ServiceSpecificException e) { throw new IllegalStateException(e); } } @@ -10763,22 +10774,22 @@ public class ConnectivityService extends IConnectivityManager.Stub try { switch (chain) { case ConnectivityManager.FIREWALL_CHAIN_DOZABLE: - mNetd.firewallReplaceUidChain("fw_dozable", true /* isAllowList */, uids); + mBpfNetMaps.replaceUidChain("fw_dozable", true /* isAllowList */, uids); break; case ConnectivityManager.FIREWALL_CHAIN_STANDBY: - mNetd.firewallReplaceUidChain("fw_standby", false /* isAllowList */, uids); + mBpfNetMaps.replaceUidChain("fw_standby", false /* isAllowList */, uids); break; case ConnectivityManager.FIREWALL_CHAIN_POWERSAVE: - mNetd.firewallReplaceUidChain("fw_powersave", true /* isAllowList */, uids); + mBpfNetMaps.replaceUidChain("fw_powersave", true /* isAllowList */, uids); break; case ConnectivityManager.FIREWALL_CHAIN_RESTRICTED: - mNetd.firewallReplaceUidChain("fw_restricted", true /* isAllowList */, uids); + mBpfNetMaps.replaceUidChain("fw_restricted", true /* isAllowList */, uids); break; default: throw new IllegalArgumentException("replaceFirewallChain with invalid chain: " + chain); } - } catch (RemoteException | ServiceSpecificException e) { + } catch (ServiceSpecificException e) { throw new IllegalStateException(e); } } @@ -10787,8 +10798,8 @@ public class ConnectivityService extends IConnectivityManager.Stub public void swapActiveStatsMap() { enforceNetworkStackOrSettingsPermission(); try { - mNetd.trafficSwapActiveStatsMap(); - } catch (RemoteException | ServiceSpecificException e) { + mBpfNetMaps.swapActiveStatsMap(); + } catch (ServiceSpecificException e) { throw new IllegalStateException(e); } } diff --git a/service/src/com/android/server/connectivity/PermissionMonitor.java b/service/src/com/android/server/connectivity/PermissionMonitor.java index 439db89166..c9c177663f 100755 --- a/service/src/com/android/server/connectivity/PermissionMonitor.java +++ b/service/src/com/android/server/connectivity/PermissionMonitor.java @@ -68,6 +68,7 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.IndentingPrintWriter; import com.android.net.module.util.CollectionUtils; +import com.android.server.BpfNetMaps; import java.util.ArrayList; import java.util.HashMap; @@ -93,6 +94,7 @@ public class PermissionMonitor { private final INetd mNetd; private final Dependencies mDeps; private final Context mContext; + private final BpfNetMaps mBpfNetMaps; @GuardedBy("this") private final Set mUsers = new HashSet<>(); @@ -184,12 +186,14 @@ public class PermissionMonitor { } } - public PermissionMonitor(@NonNull final Context context, @NonNull final INetd netd) { - this(context, netd, new Dependencies()); + public PermissionMonitor(@NonNull final Context context, @NonNull final INetd netd, + @NonNull final BpfNetMaps bpfNetMaps) { + this(context, netd, bpfNetMaps, new Dependencies()); } @VisibleForTesting PermissionMonitor(@NonNull final Context context, @NonNull final INetd netd, + @NonNull final BpfNetMaps bpfNetMaps, @NonNull final Dependencies deps) { mPackageManager = context.getPackageManager(); mUserManager = (UserManager) context.getSystemService(Context.USER_SERVICE); @@ -197,6 +201,7 @@ public class PermissionMonitor { mNetd = netd; mDeps = deps; mContext = context; + mBpfNetMaps = bpfNetMaps; } private int getPackageNetdNetworkPermission(@NonNull final PackageInfo app) { @@ -803,9 +808,9 @@ public class PermissionMonitor { } try { if (add) { - mNetd.firewallAddUidInterfaceRules(iface, toIntArray(uids)); + mBpfNetMaps.addUidInterfaceRules(iface, toIntArray(uids)); } else { - mNetd.firewallRemoveUidInterfaceRules(toIntArray(uids)); + mBpfNetMaps.removeUidInterfaceRules(toIntArray(uids)); } } catch (ServiceSpecificException e) { // Silently ignore exception when device does not support eBPF, otherwise just log @@ -813,8 +818,6 @@ public class PermissionMonitor { if (e.errorCode != OsConstants.EOPNOTSUPP) { loge("Exception when updating permissions: ", e); } - } catch (RemoteException e) { - loge("Exception when updating permissions: ", e); } } @@ -878,26 +881,27 @@ public class PermissionMonitor { try { // TODO: add a lock inside netd to protect IPC trafficSetNetPermForUids() if (allPermissionAppIds.size() != 0) { - mNetd.trafficSetNetPermForUids( + mBpfNetMaps.setNetPermForUids( PERMISSION_INTERNET | PERMISSION_UPDATE_DEVICE_STATS, toIntArray(allPermissionAppIds)); } if (internetPermissionAppIds.size() != 0) { - mNetd.trafficSetNetPermForUids(PERMISSION_INTERNET, + mBpfNetMaps.setNetPermForUids(PERMISSION_INTERNET, toIntArray(internetPermissionAppIds)); } if (updateStatsPermissionAppIds.size() != 0) { - mNetd.trafficSetNetPermForUids(PERMISSION_UPDATE_DEVICE_STATS, + mBpfNetMaps.setNetPermForUids(PERMISSION_UPDATE_DEVICE_STATS, toIntArray(updateStatsPermissionAppIds)); } if (noPermissionAppIds.size() != 0) { - mNetd.trafficSetNetPermForUids(PERMISSION_NONE, toIntArray(noPermissionAppIds)); + mBpfNetMaps.setNetPermForUids(PERMISSION_NONE, + toIntArray(noPermissionAppIds)); } if (uninstalledAppIds.size() != 0) { - mNetd.trafficSetNetPermForUids(PERMISSION_UNINSTALLED, + mBpfNetMaps.setNetPermForUids(PERMISSION_UNINSTALLED, toIntArray(uninstalledAppIds)); } - } catch (RemoteException e) { + } catch (ServiceSpecificException e) { Log.e(TAG, "Pass appId list of special permission failed." + e); } } diff --git a/tests/unit/java/com/android/server/BpfNetMapsTest.java b/tests/unit/java/com/android/server/BpfNetMapsTest.java new file mode 100644 index 0000000000..ac21e77b77 --- /dev/null +++ b/tests/unit/java/com/android/server/BpfNetMapsTest.java @@ -0,0 +1,86 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server; + +import static android.net.INetd.FIREWALL_CHAIN_DOZABLE; +import static android.net.INetd.FIREWALL_RULE_ALLOW; +import static android.net.INetd.PERMISSION_INTERNET; + +import static org.junit.Assume.assumeFalse; +import static org.mockito.Mockito.verify; + +import android.net.INetd; +import android.os.Build; + +import androidx.test.filters.SmallTest; + +import com.android.modules.utils.build.SdkLevel; +import com.android.testutils.DevSdkIgnoreRule; +import com.android.testutils.DevSdkIgnoreRunner; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +@RunWith(DevSdkIgnoreRunner.class) +@SmallTest +@DevSdkIgnoreRule.IgnoreUpTo(Build.VERSION_CODES.R) +public final class BpfNetMapsTest { + private static final String TAG = "BpfNetMapsTest"; + private static final int TEST_UID = 10086; + private static final int[] TEST_UIDS = {10002, 10003}; + private static final String IFNAME = "wlan0"; + private static final String CHAINNAME = "fw_dozable"; + private BpfNetMaps mBpfNetMaps; + + @Mock INetd mNetd; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + mBpfNetMaps = new BpfNetMaps(mNetd); + } + + @Test + public void testBpfNetMapsBeforeT() throws Exception { + assumeFalse(SdkLevel.isAtLeastT()); + mBpfNetMaps.addNaughtyApp(TEST_UID); + verify(mNetd).bandwidthAddNaughtyApp(TEST_UID); + mBpfNetMaps.removeNaughtyApp(TEST_UID); + verify(mNetd).bandwidthRemoveNaughtyApp(TEST_UID); + mBpfNetMaps.addNiceApp(TEST_UID); + verify(mNetd).bandwidthAddNiceApp(TEST_UID); + mBpfNetMaps.removeNiceApp(TEST_UID); + verify(mNetd).bandwidthRemoveNiceApp(TEST_UID); + mBpfNetMaps.setChildChain(FIREWALL_CHAIN_DOZABLE, true); + verify(mNetd).firewallEnableChildChain(FIREWALL_CHAIN_DOZABLE, true); + mBpfNetMaps.replaceUidChain(CHAINNAME, true, TEST_UIDS); + verify(mNetd).firewallReplaceUidChain(CHAINNAME, true, TEST_UIDS); + mBpfNetMaps.setUidRule(FIREWALL_CHAIN_DOZABLE, TEST_UID, FIREWALL_RULE_ALLOW); + verify(mNetd).firewallSetUidRule(FIREWALL_CHAIN_DOZABLE, TEST_UID, FIREWALL_RULE_ALLOW); + mBpfNetMaps.addUidInterfaceRules(IFNAME, TEST_UIDS); + verify(mNetd).firewallAddUidInterfaceRules(IFNAME, TEST_UIDS); + mBpfNetMaps.removeUidInterfaceRules(TEST_UIDS); + verify(mNetd).firewallRemoveUidInterfaceRules(TEST_UIDS); + mBpfNetMaps.swapActiveStatsMap(); + verify(mNetd).trafficSwapActiveStatsMap(); + mBpfNetMaps.setNetPermForUids(PERMISSION_INTERNET, TEST_UIDS); + verify(mNetd).trafficSetNetPermForUids(PERMISSION_INTERNET, TEST_UIDS); + } +} diff --git a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java index 99ef80b616..65905430f5 100644 --- a/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java +++ b/tests/unit/java/com/android/server/connectivity/PermissionMonitorTest.java @@ -89,6 +89,7 @@ import androidx.test.InstrumentationRegistry; import androidx.test.filters.SmallTest; import com.android.net.module.util.CollectionUtils; +import com.android.server.BpfNetMaps; import com.android.testutils.DevSdkIgnoreRule; import com.android.testutils.DevSdkIgnoreRunner; @@ -146,9 +147,11 @@ public class PermissionMonitorTest { @Mock private UserManager mUserManager; @Mock private PermissionMonitor.Dependencies mDeps; @Mock private SystemConfigManager mSystemConfigManager; + @Mock private BpfNetMaps mBpfNetMaps; private PermissionMonitor mPermissionMonitor; private NetdMonitor mNetdMonitor; + private BpfMapMonitor mBpfMapMonitor; @Before public void setUp() throws Exception { @@ -177,8 +180,9 @@ public class PermissionMonitorTest { // by default. doReturn(VERSION_Q).when(mDeps).getDeviceFirstSdkInt(); - mPermissionMonitor = new PermissionMonitor(mContext, mNetdService, mDeps); + mPermissionMonitor = new PermissionMonitor(mContext, mNetdService, mBpfNetMaps, mDeps); mNetdMonitor = new NetdMonitor(mNetdService); + mBpfMapMonitor = new BpfMapMonitor(mBpfNetMaps); doReturn(List.of()).when(mPackageManager).getInstalledPackagesAsUser(anyInt(), anyInt()); } @@ -511,9 +515,37 @@ public class PermissionMonitorTest { assertBackgroundPermission(true, MOCK_PACKAGE2, MOCK_UID12, NETWORK_STACK); } + private class BpfMapMonitor { + private final SparseIntArray mAppIdsTrafficPermission = new SparseIntArray(); + private static final int DOES_NOT_EXIST = -2; + + BpfMapMonitor(BpfNetMaps mockBpfmap) throws Exception { + // Add hook to verify and track result of trafficSetNetPerm. + doAnswer((InvocationOnMock invocation) -> { + final Object[] args = invocation.getArguments(); + final int permission = (int) args[0]; + for (final int appId : (int[]) args[1]) { + mAppIdsTrafficPermission.put(appId, permission); + } + return null; + }).when(mockBpfmap).setNetPermForUids(anyInt(), any(int[].class)); + } + + public void expectTrafficPerm(int permission, int... appIds) { + for (final int appId : appIds) { + if (mAppIdsTrafficPermission.get(appId, DOES_NOT_EXIST) == DOES_NOT_EXIST) { + fail("appId " + appId + " does not exist."); + } + if (mAppIdsTrafficPermission.get(appId) != permission) { + fail("appId " + appId + " has wrong permission: " + + mAppIdsTrafficPermission.get(appId)); + } + } + } + } + private class NetdMonitor { private final SparseIntArray mUidsNetworkPermission = new SparseIntArray(); - private final SparseIntArray mAppIdsTrafficPermission = new SparseIntArray(); private static final int DOES_NOT_EXIST = -2; NetdMonitor(INetd mockNetd) throws Exception { @@ -545,16 +577,6 @@ public class PermissionMonitorTest { } return null; }).when(mockNetd).networkClearPermissionForUser(any(int[].class)); - - // Add hook to verify and track result of trafficSetNetPerm. - doAnswer((InvocationOnMock invocation) -> { - final Object[] args = invocation.getArguments(); - final int permission = (int) args[0]; - for (final int appId : (int[]) args[1]) { - mAppIdsTrafficPermission.put(appId, permission); - } - return null; - }).when(mockNetd).trafficSetNetPermForUids(anyInt(), any(int[].class)); } public void expectNetworkPerm(int permission, UserHandle[] users, int... appIds) { @@ -581,18 +603,6 @@ public class PermissionMonitorTest { } } } - - public void expectTrafficPerm(int permission, int... appIds) { - for (final int appId : appIds) { - if (mAppIdsTrafficPermission.get(appId, DOES_NOT_EXIST) == DOES_NOT_EXIST) { - fail("appId " + appId + " does not exist."); - } - if (mAppIdsTrafficPermission.get(appId) != permission) { - fail("appId " + appId + " has wrong permission: " - + mAppIdsTrafficPermission.get(appId)); - } - } - } } @Test @@ -702,30 +712,30 @@ public class PermissionMonitorTest { // When VPN is connected, expect a rule to be set up for user app MOCK_UID11 mPermissionMonitor.onVpnUidRangesAdded("tun0", vpnRange1, VPN_UID); - verify(mNetdService).firewallAddUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID11})); + verify(mBpfNetMaps).addUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID11})); - reset(mNetdService); + reset(mBpfNetMaps); // When MOCK_UID11 package is uninstalled and reinstalled, expect Netd to be updated mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); - verify(mNetdService).firewallRemoveUidInterfaceRules(aryEq(new int[]{MOCK_UID11})); + verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[]{MOCK_UID11})); mPermissionMonitor.onPackageAdded(MOCK_PACKAGE1, MOCK_UID11); - verify(mNetdService).firewallAddUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID11})); + verify(mBpfNetMaps).addUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID11})); - reset(mNetdService); + reset(mBpfNetMaps); // During VPN uid update (vpnRange1 -> vpnRange2), ConnectivityService first deletes the // old UID rules then adds the new ones. Expect netd to be updated mPermissionMonitor.onVpnUidRangesRemoved("tun0", vpnRange1, VPN_UID); - verify(mNetdService).firewallRemoveUidInterfaceRules(aryEq(new int[] {MOCK_UID11})); + verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[] {MOCK_UID11})); mPermissionMonitor.onVpnUidRangesAdded("tun0", vpnRange2, VPN_UID); - verify(mNetdService).firewallAddUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID12})); + verify(mBpfNetMaps).addUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID12})); - reset(mNetdService); + reset(mBpfNetMaps); // When VPN is disconnected, expect rules to be torn down mPermissionMonitor.onVpnUidRangesRemoved("tun0", vpnRange2, VPN_UID); - verify(mNetdService).firewallRemoveUidInterfaceRules(aryEq(new int[] {MOCK_UID12})); + verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[] {MOCK_UID12})); assertNull(mPermissionMonitor.getVpnUidRanges("tun0")); } @@ -744,13 +754,13 @@ public class PermissionMonitorTest { // Newly-installed package should have uid rules added addPackageForUsers(new UserHandle[]{MOCK_USER1, MOCK_USER2}, MOCK_PACKAGE1, MOCK_APPID1); - verify(mNetdService).firewallAddUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID11})); - verify(mNetdService).firewallAddUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID21})); + verify(mBpfNetMaps).addUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID11})); + verify(mBpfNetMaps).addUidInterfaceRules(eq("tun0"), aryEq(new int[]{MOCK_UID21})); // Removed package should have its uid rules removed mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); - verify(mNetdService).firewallRemoveUidInterfaceRules(aryEq(new int[]{MOCK_UID11})); - verify(mNetdService, never()).firewallRemoveUidInterfaceRules(aryEq(new int[]{MOCK_UID21})); + verify(mBpfNetMaps).removeUidInterfaceRules(aryEq(new int[]{MOCK_UID11})); + verify(mBpfNetMaps, never()).removeUidInterfaceRules(aryEq(new int[]{MOCK_UID21})); } @@ -784,91 +794,91 @@ public class PermissionMonitorTest { // Send the permission information to netd, expect permission updated. mPermissionMonitor.sendAppIdsTrafficPermission(netdPermissionsAppIds); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID2); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, SYSTEM_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, SYSTEM_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, SYSTEM_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, SYSTEM_APPID2); // Update permission of MOCK_APPID1, expect new permission show up. mPermissionMonitor.sendPackagePermissionsForAppId(MOCK_APPID1, PERMISSION_TRAFFIC_ALL); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); // Change permissions of SYSTEM_APPID2, expect new permission show up and old permission // revoked. mPermissionMonitor.sendPackagePermissionsForAppId(SYSTEM_APPID2, PERMISSION_INTERNET); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, SYSTEM_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, SYSTEM_APPID2); // Revoke permission from SYSTEM_APPID1, expect no permission stored. mPermissionMonitor.sendPackagePermissionsForAppId(SYSTEM_APPID1, PERMISSION_NONE); - mNetdMonitor.expectTrafficPerm(PERMISSION_NONE, SYSTEM_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, SYSTEM_APPID1); } @Test public void testPackageInstall() throws Exception { addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET, UPDATE_DEVICE_STATS); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); addPackage(MOCK_PACKAGE2, MOCK_UID12, INTERNET); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID2); } @Test public void testPackageInstallSharedUid() throws Exception { addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET, UPDATE_DEVICE_STATS); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); // Install another package with the same uid and no permissions should not cause the app id // to lose permissions. addPackage(MOCK_PACKAGE2, MOCK_UID11); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); } @Test public void testPackageUninstallBasic() throws Exception { addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET, UPDATE_DEVICE_STATS); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); when(mPackageManager.getPackagesForUid(MOCK_UID11)).thenReturn(new String[]{}); mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); - mNetdMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); } @Test public void testPackageRemoveThenAdd() throws Exception { addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET, UPDATE_DEVICE_STATS); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); when(mPackageManager.getPackagesForUid(MOCK_UID11)).thenReturn(new String[]{}); mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); - mNetdMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); } @Test public void testPackageUpdate() throws Exception { addPackage(MOCK_PACKAGE1, MOCK_UID11); - mNetdMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1); addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); } @Test public void testPackageUninstallWithMultiplePackages() throws Exception { addPackage(MOCK_PACKAGE1, MOCK_UID11, INTERNET, UPDATE_DEVICE_STATS); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); // Install another package with the same uid but different permissions. addPackage(MOCK_PACKAGE2, MOCK_UID11, INTERNET); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_UID11); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_UID11); // Uninstall MOCK_PACKAGE1 and expect only INTERNET permission left. when(mPackageManager.getPackagesForUid(eq(MOCK_UID11))) .thenReturn(new String[]{MOCK_PACKAGE2}); mPermissionMonitor.onPackageRemoved(MOCK_PACKAGE1, MOCK_UID11); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); } @Test @@ -876,7 +886,8 @@ public class PermissionMonitorTest { // Use the real context as this test must ensure the *real* system package holds the // necessary permission. final Context realContext = InstrumentationRegistry.getContext(); - final PermissionMonitor monitor = new PermissionMonitor(realContext, mNetdService); + final PermissionMonitor monitor = new PermissionMonitor(realContext, mNetdService, + mBpfNetMaps); final PackageManager manager = realContext.getPackageManager(); final PackageInfo systemInfo = manager.getPackageInfo(REAL_SYSTEM_PACKAGE_NAME, GET_PERMISSIONS | MATCH_ANY_USER); @@ -891,8 +902,8 @@ public class PermissionMonitorTest { .thenReturn(new int[]{ MOCK_UID12 }); mPermissionMonitor.startMonitoring(); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID2); } private BroadcastReceiver expectBroadcastReceiver(String... actions) { @@ -923,7 +934,7 @@ public class PermissionMonitorTest { buildAndMockPackageInfoWithPermissions(MOCK_PACKAGE1, MOCK_UID11, INTERNET, UPDATE_DEVICE_STATS); receiver.onReceive(mContext, addedIntent); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); // Verify receiving PACKAGE_REMOVED intent. when(mPackageManager.getPackagesForUid(MOCK_UID11)).thenReturn(new String[]{}); @@ -931,7 +942,7 @@ public class PermissionMonitorTest { Uri.fromParts("package", MOCK_PACKAGE1, null /* fragment */)); removedIntent.putExtra(Intent.EXTRA_UID, MOCK_UID11); receiver.onReceive(mContext, removedIntent); - mNetdMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_UNINSTALLED, MOCK_APPID1); } private ContentObserver expectRegisterContentObserver(Uri expectedUri) { @@ -1070,7 +1081,7 @@ public class PermissionMonitorTest { .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring(); mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER1}, MOCK_APPID1, MOCK_APPID2); - mNetdMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1, MOCK_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1, MOCK_APPID2); final BroadcastReceiver receiver = expectBroadcastReceiver( Intent.ACTION_EXTERNAL_APPLICATIONS_AVAILABLE); @@ -1087,8 +1098,8 @@ public class PermissionMonitorTest { MOCK_APPID1); mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER1}, MOCK_APPID2); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, MOCK_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, MOCK_APPID2); } @Test @@ -1114,8 +1125,8 @@ public class PermissionMonitorTest { MOCK_APPID1); mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER1}, MOCK_APPID2); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, MOCK_APPID2); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, MOCK_APPID2); } @Test @@ -1128,7 +1139,7 @@ public class PermissionMonitorTest { .when(mPackageManager).getInstalledPackagesAsUser(eq(GET_PERMISSIONS), anyInt()); mPermissionMonitor.startMonitoring(); mNetdMonitor.expectNoNetworkPerm(new UserHandle[]{MOCK_USER1}, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_NONE, MOCK_APPID1); final BroadcastReceiver receiver = expectBroadcastReceiver( Intent.ACTION_EXTERNAL_APPLICATIONS_AVAILABLE); @@ -1140,7 +1151,7 @@ public class PermissionMonitorTest { receiver.onReceive(mContext, externalIntent); mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER1}, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_UPDATE_DEVICE_STATS, MOCK_APPID1); } @Test @@ -1155,7 +1166,7 @@ public class PermissionMonitorTest { mPermissionMonitor.startMonitoring(); mNetdMonitor.expectNetworkPerm(PERMISSION_NETWORK, new UserHandle[]{MOCK_USER1}, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_INTERNET, MOCK_APPID1); final BroadcastReceiver receiver = expectBroadcastReceiver( Intent.ACTION_EXTERNAL_APPLICATIONS_AVAILABLE); @@ -1169,7 +1180,7 @@ public class PermissionMonitorTest { receiver.onReceive(mContext, externalIntent); mNetdMonitor.expectNetworkPerm(PERMISSION_SYSTEM, new UserHandle[]{MOCK_USER1}, MOCK_APPID1); - mNetdMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); + mBpfMapMonitor.expectTrafficPerm(PERMISSION_TRAFFIC_ALL, MOCK_APPID1); } @Test From 361b859b003bbff625223bb478d958ed8a395efc Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 28 Jan 2022 15:39:17 +0100 Subject: [PATCH 2/4] Merge libtraffic_controller_jni into libservice-connectivity There is not really a point to separate these two as they are all used in ConnectivityService. As a future TODO, we might want to rename libservice-connectivity to libcom_android_server_jni to be more inline with the jarjar'ed jni lib versions. Test: atest FrameworksNetTests:ConnectivityServiceTest Change-Id: I1b9f4fde345038bda6b3ffcf8e3f47cf9262e052 --- Tethering/apex/Android.bp | 1 - service/Android.bp | 8 +++- .../jni/com_android_server_BpfNetMaps.cpp | 0 .../com_android_server_TestNetworkService.cpp | 2 +- ...id_server_connectivity_ClatCoordinator.cpp | 2 +- service/jni/onload.cpp | 13 ++++-- service/native/Android.bp | 31 -------------- service/native/jni/onload.cpp | 41 ------------------- 8 files changed, 18 insertions(+), 80 deletions(-) rename service/{native => }/jni/com_android_server_BpfNetMaps.cpp (100%) delete mode 100644 service/native/jni/onload.cpp diff --git a/Tethering/apex/Android.bp b/Tethering/apex/Android.bp index 53a14ad9db..c0f68fde2c 100644 --- a/Tethering/apex/Android.bp +++ b/Tethering/apex/Android.bp @@ -53,7 +53,6 @@ apex { jni_libs: [ "libservice-connectivity", "libcom_android_connectivity_com_android_net_module_util_jni", - "libtraffic_controller_jni", ], }, both: { diff --git a/service/Android.bp b/service/Android.bp index 33d4b662a3..d940003318 100644 --- a/service/Android.bp +++ b/service/Android.bp @@ -57,23 +57,29 @@ cc_library_shared { "-Wthread-safety", ], srcs: [ - "jni/com_android_server_TestNetworkService.cpp", + "jni/com_android_server_BpfNetMaps.cpp", "jni/com_android_server_connectivity_ClatCoordinator.cpp", + "jni/com_android_server_TestNetworkService.cpp", "jni/onload.cpp", ], stl: "libc++_static", header_libs: [ + "bpf_connectivity_headers", "libbase_headers", ], static_libs: [ "libbase", "libclat", "libip_checksum", + "libnetdutils", "libnetjniutils", + "libtraffic_controller", + "netd_aidl_interface-lateststable-ndk", ], shared_libs: [ "liblog", "libnativehelper", + "libutils", ], apex_available: [ "com.android.tethering", diff --git a/service/native/jni/com_android_server_BpfNetMaps.cpp b/service/jni/com_android_server_BpfNetMaps.cpp similarity index 100% rename from service/native/jni/com_android_server_BpfNetMaps.cpp rename to service/jni/com_android_server_BpfNetMaps.cpp diff --git a/service/jni/com_android_server_TestNetworkService.cpp b/service/jni/com_android_server_TestNetworkService.cpp index 1a0de32ae9..4efd0e1a53 100644 --- a/service/jni/com_android_server_TestNetworkService.cpp +++ b/service/jni/com_android_server_TestNetworkService.cpp @@ -98,7 +98,7 @@ static const JNINativeMethod gMethods[] = { {"jniCreateTunTap", "(ZLjava/lang/String;)I", (void*)create}, }; -int register_android_server_TestNetworkService(JNIEnv* env) { +int register_com_android_server_TestNetworkService(JNIEnv* env) { return jniRegisterNativeMethods(env, "com/android/server/TestNetworkService", gMethods, NELEM(gMethods)); } diff --git a/service/jni/com_android_server_connectivity_ClatCoordinator.cpp b/service/jni/com_android_server_connectivity_ClatCoordinator.cpp index b445462a9a..ee512ec95f 100644 --- a/service/jni/com_android_server_connectivity_ClatCoordinator.cpp +++ b/service/jni/com_android_server_connectivity_ClatCoordinator.cpp @@ -519,7 +519,7 @@ static const JNINativeMethod gMethods[] = { (void*)com_android_server_connectivity_ClatCoordinator_stopClatd}, }; -int register_android_server_connectivity_ClatCoordinator(JNIEnv* env) { +int register_com_android_server_connectivity_ClatCoordinator(JNIEnv* env) { return jniRegisterNativeMethods(env, "com/android/server/connectivity/ClatCoordinator", gMethods, NELEM(gMethods)); } diff --git a/service/jni/onload.cpp b/service/jni/onload.cpp index 04d9671fad..facdad7041 100644 --- a/service/jni/onload.cpp +++ b/service/jni/onload.cpp @@ -19,8 +19,9 @@ namespace android { -int register_android_server_TestNetworkService(JNIEnv* env); -int register_android_server_connectivity_ClatCoordinator(JNIEnv* env); +int register_com_android_server_TestNetworkService(JNIEnv* env); +int register_com_android_server_connectivity_ClatCoordinator(JNIEnv* env); +int register_com_android_server_BpfNetMaps(JNIEnv* env); extern "C" jint JNI_OnLoad(JavaVM* vm, void*) { JNIEnv *env; @@ -29,11 +30,15 @@ extern "C" jint JNI_OnLoad(JavaVM* vm, void*) { return JNI_ERR; } - if (register_android_server_TestNetworkService(env) < 0) { + if (register_com_android_server_TestNetworkService(env) < 0) { return JNI_ERR; } - if (register_android_server_connectivity_ClatCoordinator(env) < 0) { + if (register_com_android_server_connectivity_ClatCoordinator(env) < 0) { + return JNI_ERR; + } + + if (register_com_android_server_BpfNetMaps(env) < 0) { return JNI_ERR; } diff --git a/service/native/Android.bp b/service/native/Android.bp index b49457da5b..0e805e2903 100644 --- a/service/native/Android.bp +++ b/service/native/Android.bp @@ -52,37 +52,6 @@ cc_library { min_sdk_version: "30", } -cc_library_shared { - name: "libtraffic_controller_jni", - cflags: [ - "-Wall", - "-Werror", - "-Wno-unused-parameter", - "-Wthread-safety", - ], - srcs: [ - "jni/*.cpp", - ], - header_libs: [ - "bpf_connectivity_headers", - ], - static_libs: [ - "libnetdutils", - "libtraffic_controller", - "netd_aidl_interface-lateststable-ndk", - ], - shared_libs: [ - "libbase", - "liblog", - "libutils", - "libnativehelper", - ], - apex_available: [ - "com.android.tethering", - ], - min_sdk_version: "30", -} - cc_test { name: "traffic_controller_unit_test", test_suites: ["general-tests"], diff --git a/service/native/jni/onload.cpp b/service/native/jni/onload.cpp deleted file mode 100644 index df7c77bfb6..0000000000 --- a/service/native/jni/onload.cpp +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright (C) 2022 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#define LOG_TAG "TrafficControllerJni" - -#include -#include - -#include "utils/Log.h" - -namespace android { - -int register_com_android_server_BpfNetMaps(JNIEnv* env); - -extern "C" jint JNI_OnLoad(JavaVM* vm, void*) { - JNIEnv *env; - if (vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6) != JNI_OK) { - ALOGE("%s: ERROR: GetEnv failed", __func__); - return JNI_ERR; - } - - if (register_com_android_server_BpfNetMaps(env) < 0) - return JNI_ERR; - - return JNI_VERSION_1_6; -} - -}; // namespace android From 313bc6c2d340bba4c027ea01cdee581543b0cded Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Mon, 31 Jan 2022 15:51:09 +0100 Subject: [PATCH 3/4] Remove libutils dependency from libservice-connectivity Remove libutils dependency. Test: m Change-Id: I857e40c984bcce2931b5068b3b96d2c9dd69693c --- service/Android.bp | 1 - service/jni/com_android_server_BpfNetMaps.cpp | 8 ++++---- service/native/TrafficController.cpp | 2 -- service/native/include/TrafficController.h | 4 ++-- 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/service/Android.bp b/service/Android.bp index d940003318..1e66c11b79 100644 --- a/service/Android.bp +++ b/service/Android.bp @@ -79,7 +79,6 @@ cc_library_shared { shared_libs: [ "liblog", "libnativehelper", - "libutils", ], apex_available: [ "com.android.tethering", diff --git a/service/jni/com_android_server_BpfNetMaps.cpp b/service/jni/com_android_server_BpfNetMaps.cpp index 7ab4d46ceb..113c449630 100644 --- a/service/jni/com_android_server_BpfNetMaps.cpp +++ b/service/jni/com_android_server_BpfNetMaps.cpp @@ -16,17 +16,17 @@ #define LOG_TAG "TrafficControllerJni" +#include "TrafficController.h" + +#include #include +#include #include #include #include #include #include -#include "TrafficController.h" -#include "android-base/logging.h" -#include "bpf_shared.h" -#include "utils/Log.h" using android::net::TrafficController; using android::netdutils::Status; diff --git a/service/native/TrafficController.cpp b/service/native/TrafficController.cpp index 97fcc43194..51dd502b21 100644 --- a/service/native/TrafficController.cpp +++ b/service/native/TrafficController.cpp @@ -683,8 +683,6 @@ void dumpBpfMap(const std::string& mapName, DumpWriter& dw, const std::string& h } } -const String16 TrafficController::DUMP_KEYWORD = String16("trafficcontroller"); - void TrafficController::dump(DumpWriter& dw, bool verbose) { std::lock_guard guard(mMutex); ScopedIndent indentTop(dw); diff --git a/service/native/include/TrafficController.h b/service/native/include/TrafficController.h index c0508719fe..ddcf4459fb 100644 --- a/service/native/include/TrafficController.h +++ b/service/native/include/TrafficController.h @@ -25,7 +25,6 @@ #include "netdutils/DumpWriter.h" #include "netdutils/NetlinkListener.h" #include "netdutils/StatusOr.h" -#include "utils/String16.h" namespace android { namespace net { @@ -34,6 +33,8 @@ using netdutils::StatusOr; class TrafficController { public: + static constexpr char DUMP_KEYWORD[] = "trafficcontroller"; + /* * Initialize the whole controller */ @@ -84,7 +85,6 @@ class TrafficController { netdutils::Status updateUidOwnerMap(const uint32_t uid, UidOwnerMatchType matchType, IptOp op) EXCLUDES(mMutex); - static const String16 DUMP_KEYWORD; int toggleUidOwnerMap(ChildChain chain, bool enable) EXCLUDES(mMutex); From c2984fdca83de11a6a4990576aab1596ef94909b Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Mon, 31 Jan 2022 18:06:19 +0100 Subject: [PATCH 4/4] Prevent native_init from starting TrafficController TrafficController is currently still started by netd, and this should not happen in two places. Instead, native_init should init (open) the maps. Test: atest FrameworksNetTests Change-Id: Ifd6be50aa5f62e59a5b1c5c0a97550389fd0e7e1 --- service/jni/com_android_server_BpfNetMaps.cpp | 3 ++- service/native/include/TrafficController.h | 16 +++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/service/jni/com_android_server_BpfNetMaps.cpp b/service/jni/com_android_server_BpfNetMaps.cpp index 113c449630..bde52a59b2 100644 --- a/service/jni/com_android_server_BpfNetMaps.cpp +++ b/service/jni/com_android_server_BpfNetMaps.cpp @@ -39,7 +39,8 @@ static android::net::TrafficController mTc; namespace android { static void native_init(JNIEnv* env, jobject clazz) { - Status status = mTc.start(); + // start is still being called by netd + Status status = mTc.initMaps(); if (!isOk(status)) { ALOGE("%s failed", __func__); } diff --git a/service/native/include/TrafficController.h b/service/native/include/TrafficController.h index ddcf4459fb..2d89344339 100644 --- a/service/native/include/TrafficController.h +++ b/service/native/include/TrafficController.h @@ -32,14 +32,22 @@ namespace net { using netdutils::StatusOr; class TrafficController { - public: - static constexpr char DUMP_KEYWORD[] = "trafficcontroller"; - + // TODO: marking this private for right now, as start is already called by + // netd. start() calls initMaps(), initPrograms(), and sets up the socket + // destroy listener. Both initPrograms() and setting up the socket destroy + // listener should only be done once. /* * Initialize the whole controller */ netdutils::Status start(); + public: + static constexpr char DUMP_KEYWORD[] = "trafficcontroller"; + + // TODO: marking this public for right now, as start() is already called by + // netd. + netdutils::Status initMaps() EXCLUDES(mMutex); + int setCounterSet(int counterSetNum, uid_t uid, uid_t callingUid) EXCLUDES(mMutex); /* @@ -187,8 +195,6 @@ class TrafficController { std::mutex mMutex; - netdutils::Status initMaps() EXCLUDES(mMutex); - // Keep track of uids that have permission UPDATE_DEVICE_STATS so we don't // need to call back to system server for permission check. std::set mPrivilegedUser GUARDED_BY(mMutex);