From 305975ffb084e687665d18d3b43db97022f12557 Mon Sep 17 00:00:00 2001 From: Motomu Utsumi Date: Mon, 27 Jun 2022 09:24:32 +0000 Subject: [PATCH] Update error handling in BpfNetMaps Address review comments from aosp/2117045 Throw AssertionError if bpf map init get error Update tests to avoid getting AssertionError Bug: 217624062 Test: atest BpfNetMapsTest Change-Id: I2194b825aa73b88d5334077d17acb9abbd367c69 --- .../server/net/NetworkStatsFactory.java | 7 ++- .../src/com/android/server/BpfNetMaps.java | 59 ++++++++++--------- .../ConnectivityServiceIntegrationTest.kt | 2 + .../com/android/server/BpfNetMapsTest.java | 27 +++------ .../server/net/NetworkStatsFactoryTest.java | 4 +- 5 files changed, 47 insertions(+), 52 deletions(-) diff --git a/service-t/src/com/android/server/net/NetworkStatsFactory.java b/service-t/src/com/android/server/net/NetworkStatsFactory.java index 3b93f1a190..b628251037 100644 --- a/service-t/src/com/android/server/net/NetworkStatsFactory.java +++ b/service-t/src/com/android/server/net/NetworkStatsFactory.java @@ -164,16 +164,17 @@ public class NetworkStatsFactory { } public NetworkStatsFactory(@NonNull Context ctx) { - this(ctx, new File("/proc/"), true); + this(ctx, new File("/proc/"), true, new BpfNetMaps()); } @VisibleForTesting - public NetworkStatsFactory(@NonNull Context ctx, File procRoot, boolean useBpfStats) { + public NetworkStatsFactory(@NonNull Context ctx, File procRoot, boolean useBpfStats, + BpfNetMaps bpfNetMaps) { mStatsXtIfaceAll = new File(procRoot, "net/xt_qtaguid/iface_stat_all"); mStatsXtIfaceFmt = new File(procRoot, "net/xt_qtaguid/iface_stat_fmt"); mStatsXtUid = new File(procRoot, "net/xt_qtaguid/stats"); mUseBpfStats = useBpfStats; - mBpfNetMaps = new BpfNetMaps(); + mBpfNetMaps = bpfNetMaps; synchronized (mPersistentDataLock) { mPersistSnapshot = new NetworkStats(SystemClock.elapsedRealtime(), -1); mTunAnd464xlatAdjustedStats = new NetworkStats(SystemClock.elapsedRealtime(), -1); diff --git a/service/src/com/android/server/BpfNetMaps.java b/service/src/com/android/server/BpfNetMaps.java index 6e107c29fa..5650e31106 100644 --- a/service/src/com/android/server/BpfNetMaps.java +++ b/service/src/com/android/server/BpfNetMaps.java @@ -49,10 +49,16 @@ import java.io.IOException; * {@hide} */ public class BpfNetMaps { + private static final boolean PRE_T = !SdkLevel.isAtLeastT(); + static { + if (!PRE_T) { + System.loadLibrary("service-connectivity"); + } + } + private static final String TAG = "BpfNetMaps"; private final INetd mNetd; // Use legacy netd for releases before T. - private static final boolean PRE_T = !SdkLevel.isAtLeastT(); private static boolean sInitialized = false; // Lock for sConfigurationMap entry for UID_RULES_CONFIGURATION_KEY. @@ -98,11 +104,26 @@ public class BpfNetMaps { } /** - * Only tests or BpfNetMaps#ensureInitialized can call this function. + * Set configurationMap for test. */ @VisibleForTesting - public static void initialize(final Dependencies deps) { - sConfigurationMap = deps.getConfigurationMap(); + public static void setConfigurationMapForTest(BpfMap configurationMap) { + sConfigurationMap = configurationMap; + } + + private static BpfMap getConfigurationMap() { + try { + return new BpfMap<>( + CONFIGURATION_MAP_PATH, BpfMap.BPF_F_RDWR, U32.class, U32.class); + } catch (ErrnoException e) { + throw new IllegalStateException("Cannot open netd configuration map", e); + } + } + + private static void setBpfMaps() { + if (sConfigurationMap == null) { + sConfigurationMap = getConfigurationMap(); + } } /** @@ -111,33 +132,11 @@ public class BpfNetMaps { */ private static synchronized void ensureInitialized() { if (sInitialized) return; - if (!PRE_T) { - System.loadLibrary("service-connectivity"); - native_init(); - initialize(new Dependencies()); - } + setBpfMaps(); + native_init(); sInitialized = true; } - /** - * Dependencies of BpfNetMaps, for injection in tests. - */ - @VisibleForTesting - public static class Dependencies { - /** - * Get configuration BPF map. - */ - public BpfMap getConfigurationMap() { - try { - return new BpfMap<>( - CONFIGURATION_MAP_PATH, BpfMap.BPF_F_RDWR, U32.class, U32.class); - } catch (ErrnoException e) { - Log.e(TAG, "Cannot open netd configuration map: " + e); - return null; - } - } - } - /** Constructor used after T that doesn't need to use netd anymore. */ public BpfNetMaps() { this(null); @@ -146,7 +145,9 @@ public class BpfNetMaps { } public BpfNetMaps(final INetd netd) { - ensureInitialized(); + if (!PRE_T) { + ensureInitialized(); + } mNetd = netd; } diff --git a/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt b/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt index 80338aa2c0..efc24d32dc 100644 --- a/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt +++ b/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt @@ -47,6 +47,7 @@ import android.util.Log import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import com.android.connectivity.resources.R +import com.android.server.BpfNetMaps import com.android.server.ConnectivityService import com.android.server.NetworkAgentWrapper import com.android.server.TestNetIdManager @@ -208,6 +209,7 @@ class ConnectivityServiceIntegrationTest { doReturn(mock(ProxyTracker::class.java)).`when`(deps).makeProxyTracker(any(), any()) doReturn(mock(MockableSystemProperties::class.java)).`when`(deps).systemProperties doReturn(TestNetIdManager()).`when`(deps).makeNetIdManager() + doReturn(mock(BpfNetMaps::class.java)).`when`(deps).getBpfNetMaps(any()) doAnswer { inv -> object : MultinetworkPolicyTracker(inv.getArgument(0), inv.getArgument(1), inv.getArgument(2)) { diff --git a/tests/unit/java/com/android/server/BpfNetMapsTest.java b/tests/unit/java/com/android/server/BpfNetMapsTest.java index e9fad9bc54..689c148cb7 100644 --- a/tests/unit/java/com/android/server/BpfNetMapsTest.java +++ b/tests/unit/java/com/android/server/BpfNetMapsTest.java @@ -85,24 +85,13 @@ public final class BpfNetMapsTest { private BpfNetMaps mBpfNetMaps; @Mock INetd mNetd; - private static final TestBpfMap sConfigurationMap = - new TestBpfMap<>(U32.class, U32.class); + private final BpfMap mConfigurationMap = new TestBpfMap<>(U32.class, U32.class); @Before public void setUp() throws Exception { MockitoAnnotations.initMocks(this); + BpfNetMaps.setConfigurationMapForTest(mConfigurationMap); mBpfNetMaps = new BpfNetMaps(mNetd); - BpfNetMaps.initialize(makeDependencies()); - sConfigurationMap.clear(); - } - - private static BpfNetMaps.Dependencies makeDependencies() { - return new BpfNetMaps.Dependencies() { - @Override - public BpfMap getConfigurationMap() { - return sConfigurationMap; - } - }; } @Test @@ -121,7 +110,7 @@ public final class BpfNetMapsTest { for (final int chain: enableChains) { match |= mBpfNetMaps.getMatchByFirewallChain(chain); } - sConfigurationMap.updateEntry(UID_RULES_CONFIGURATION_KEY, new U32(match)); + mConfigurationMap.updateEntry(UID_RULES_CONFIGURATION_KEY, new U32(match)); for (final int chain: FIREWALL_CHAINS) { final String testCase = "EnabledChains: " + enableChains + " CheckedChain: " + chain; @@ -187,17 +176,17 @@ public final class BpfNetMapsTest { expectedMatch |= mBpfNetMaps.getMatchByFirewallChain(chain); } - assertEquals(0, sConfigurationMap.getValue(UID_RULES_CONFIGURATION_KEY).val); + assertEquals(0, mConfigurationMap.getValue(UID_RULES_CONFIGURATION_KEY).val); for (final int chain: testChains) { mBpfNetMaps.setChildChain(chain, true /* enable */); } - assertEquals(expectedMatch, sConfigurationMap.getValue(UID_RULES_CONFIGURATION_KEY).val); + assertEquals(expectedMatch, mConfigurationMap.getValue(UID_RULES_CONFIGURATION_KEY).val); for (final int chain: testChains) { mBpfNetMaps.setChildChain(chain, false /* enable */); } - assertEquals(0, sConfigurationMap.getValue(UID_RULES_CONFIGURATION_KEY).val); + assertEquals(0, mConfigurationMap.getValue(UID_RULES_CONFIGURATION_KEY).val); } private void doTestSetChildChain(final int testChain) throws Exception { @@ -207,7 +196,7 @@ public final class BpfNetMapsTest { @Test @IgnoreUpTo(Build.VERSION_CODES.S_V2) public void testSetChildChain() throws Exception { - sConfigurationMap.updateEntry(UID_RULES_CONFIGURATION_KEY, new U32(0)); + mConfigurationMap.updateEntry(UID_RULES_CONFIGURATION_KEY, new U32(0)); doTestSetChildChain(FIREWALL_CHAIN_DOZABLE); doTestSetChildChain(FIREWALL_CHAIN_STANDBY); doTestSetChildChain(FIREWALL_CHAIN_POWERSAVE); @@ -221,7 +210,7 @@ public final class BpfNetMapsTest { @Test @IgnoreUpTo(Build.VERSION_CODES.S_V2) public void testSetChildChainMultipleChain() throws Exception { - sConfigurationMap.updateEntry(UID_RULES_CONFIGURATION_KEY, new U32(0)); + mConfigurationMap.updateEntry(UID_RULES_CONFIGURATION_KEY, new U32(0)); doTestSetChildChain(List.of( FIREWALL_CHAIN_DOZABLE, FIREWALL_CHAIN_STANDBY)); diff --git a/tests/unit/java/com/android/server/net/NetworkStatsFactoryTest.java b/tests/unit/java/com/android/server/net/NetworkStatsFactoryTest.java index 5400a00d76..f6fb45ca11 100644 --- a/tests/unit/java/com/android/server/net/NetworkStatsFactoryTest.java +++ b/tests/unit/java/com/android/server/net/NetworkStatsFactoryTest.java @@ -44,6 +44,7 @@ import androidx.test.InstrumentationRegistry; import androidx.test.filters.SmallTest; import com.android.frameworks.tests.net.R; +import com.android.server.BpfNetMaps; import com.android.testutils.DevSdkIgnoreRule; import com.android.testutils.DevSdkIgnoreRunner; @@ -74,6 +75,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { private File mTestProc; private NetworkStatsFactory mFactory; @Mock private Context mContext; + @Mock private BpfNetMaps mBpfNetMaps; @Before public void setUp() throws Exception { @@ -84,7 +86,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { // applications. So in order to have a test support native library, the native code // related to networkStatsFactory is compiled to a minimal native library and loaded here. System.loadLibrary("networkstatsfactorytestjni"); - mFactory = new NetworkStatsFactory(mContext, mTestProc, false); + mFactory = new NetworkStatsFactory(mContext, mTestProc, false, mBpfNetMaps); mFactory.updateUnderlyingNetworkInfos(new UnderlyingNetworkInfo[0]); }