From 6ad7599b5c34c71962106269331ccc53848e878a Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Thu, 5 Dec 2024 14:45:32 +0800 Subject: [PATCH] Exclude NET_CAP_LOCAL_NETWORK when listening for upstream network events In the current design, the upstream monitor collects prefixes of all upstream networks to prevent prefix conflicts, which are coordinated by the PrivateAddressCoordinator. However, the upstream monitor does not exclude the newly added NET_CAP_LOCAL_NETWORK capability, which could be held by a downstream network. This could lead to a prefix conflict detection loop: 1. In UpstreamNetworkMonitor, a prefix is collected from a local network and sent to the PrivateAddressCoordinator. 2. PrivateAddressCoordinator detects a prefix conflict with an existing downstream prefix. 3. The conflict notification is sent to the local network provider, triggering a recovery. Test: atest TetheringTests:com.android.networkstack.tethering.UpstreamNetworkMonitorTest Bug: 349487600 Bug: 382397803 Change-Id: I743d569394e190c8620b283689d853cd2f5e3f28 --- .../networkstack/tethering/Tethering.java | 2 +- .../tethering/UpstreamNetworkMonitor.java | 55 ++++++++++++------- .../tethering/TestConnectivityManager.java | 14 +++-- .../networkstack/tethering/TetheringTest.java | 10 ++-- .../tethering/UpstreamNetworkMonitorTest.java | 29 +++++----- 5 files changed, 64 insertions(+), 46 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 254b60fda3..bdc9387ee0 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -2089,7 +2089,7 @@ public class Tethering { } mRoutingCoordinator.maybeRemoveDeprecatedUpstreams(); - mUpstreamNetworkMonitor.startObserveAllNetworks(); + mUpstreamNetworkMonitor.startObserveUpstreamNetworks(); // TODO: De-duplicate with updateUpstreamWanted() below. if (upstreamWanted()) { diff --git a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java index 7a05d749e7..9705d847db 100644 --- a/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java +++ b/Tethering/src/com/android/networkstack/tethering/UpstreamNetworkMonitor.java @@ -24,6 +24,7 @@ import static android.net.ConnectivityManager.TYPE_MOBILE_HIPRI; import static android.net.ConnectivityManager.TYPE_WIFI; import static android.net.NetworkCapabilities.NET_CAPABILITY_DUN; import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; +import static android.net.NetworkCapabilities.NET_CAPABILITY_LOCAL_NETWORK; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_RESTRICTED; import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VPN; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; @@ -44,6 +45,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.android.internal.annotations.VisibleForTesting; +import com.android.modules.utils.build.SdkLevel; import com.android.net.module.util.SharedLog; import com.android.networkstack.apishim.ConnectivityManagerShimImpl; import com.android.networkstack.apishim.common.ConnectivityManagerShim; @@ -62,9 +64,10 @@ import java.util.Set; * The owner of UNM gets it to register network callbacks by calling the * following methods : * Calling #startTrackDefaultNetwork() to track the system default network. - * Calling #startObserveAllNetworks() to observe all networks. Listening all - * networks is necessary while the expression of preferred upstreams remains - * a list of legacy connectivity types. In future, this can be revisited. + * Calling #startObserveUpstreamNetworks() to observe upstream networks. + * Listening all upstream networks is necessary while the expression of + * preferred upstreams remains a list of legacy connectivity types. + * In future, this can be revisited. * Calling #setTryCell() to request bringing up mobile DUN or HIPRI. * * The methods and data members of this class are only to be accessed and @@ -94,7 +97,7 @@ public class UpstreamNetworkMonitor { @VisibleForTesting public static final int TYPE_NONE = -1; - private static final int CALLBACK_LISTEN_ALL = 1; + private static final int CALLBACK_LISTEN_UPSTREAM = 1; private static final int CALLBACK_DEFAULT_INTERNET = 2; private static final int CALLBACK_MOBILE_REQUEST = 3; @@ -116,7 +119,7 @@ public class UpstreamNetworkMonitor { private HashSet mLocalPrefixes; private ConnectivityManager mCM; private EntitlementManager mEntitlementMgr; - private NetworkCallback mListenAllCallback; + private NetworkCallback mListenUpstreamCallback; private NetworkCallback mDefaultNetworkCallback; private NetworkCallback mMobileNetworkCallback; @@ -157,20 +160,29 @@ public class UpstreamNetworkMonitor { } ConnectivityManagerShim mCmShim = ConnectivityManagerShimImpl.newInstance(mContext); mDefaultNetworkCallback = new UpstreamNetworkCallback(CALLBACK_DEFAULT_INTERNET); + // TODO (b/382413665): By definition, a local network cannot be the system default, + // because it does not provide internet capability. Figure out whether this + // is enforced in ConnectivityService. Or what will happen for tethering if it happens. mCmShim.registerSystemDefaultNetworkCallback(mDefaultNetworkCallback, mHandler); if (mEntitlementMgr == null) { mEntitlementMgr = entitle; } } - /** Listen all networks. */ - public void startObserveAllNetworks() { + /** Listen upstream networks. */ + public void startObserveUpstreamNetworks() { stop(); - final NetworkRequest listenAllRequest = new NetworkRequest.Builder() - .clearCapabilities().build(); - mListenAllCallback = new UpstreamNetworkCallback(CALLBACK_LISTEN_ALL); - cm().registerNetworkCallback(listenAllRequest, mListenAllCallback, mHandler); + final NetworkRequest listenUpstreamRequest; + // Before V, only TV supports local agent on U, which doesn't support tethering. + if (SdkLevel.isAtLeastV()) { + listenUpstreamRequest = new NetworkRequest.Builder().clearCapabilities() + .addForbiddenCapability(NET_CAPABILITY_LOCAL_NETWORK).build(); + } else { + listenUpstreamRequest = new NetworkRequest.Builder().clearCapabilities().build(); + } + mListenUpstreamCallback = new UpstreamNetworkCallback(CALLBACK_LISTEN_UPSTREAM); + cm().registerNetworkCallback(listenUpstreamRequest, mListenUpstreamCallback, mHandler); } /** @@ -183,8 +195,8 @@ public class UpstreamNetworkMonitor { public void stop() { setTryCell(false); - releaseCallback(mListenAllCallback); - mListenAllCallback = null; + releaseCallback(mListenUpstreamCallback); + mListenUpstreamCallback = null; mNetworkMap.clear(); } @@ -535,10 +547,10 @@ public class UpstreamNetworkMonitor { return; } - // Any non-LISTEN_ALL callback will necessarily concern a network that will - // also match the LISTEN_ALL callback by construction of the LISTEN_ALL callback. - // So it's not useful to do this work for non-LISTEN_ALL callbacks. - if (mCallbackType == CALLBACK_LISTEN_ALL) { + // Any non-LISTEN_UPSTREAM callback will necessarily concern a network that will + // also match the LISTEN_UPSTREAM callback by construction of the LISTEN_UPSTREAM + // callback. So it's not useful to do this work for non-LISTEN_UPSTREAM callbacks. + if (mCallbackType == CALLBACK_LISTEN_UPSTREAM) { recomputeLocalPrefixes(); } } @@ -555,10 +567,11 @@ public class UpstreamNetworkMonitor { } handleLost(network); - // Any non-LISTEN_ALL callback will necessarily concern a network that will - // also match the LISTEN_ALL callback by construction of the LISTEN_ALL callback. - // So it's not useful to do this work for non-LISTEN_ALL callbacks. - if (mCallbackType == CALLBACK_LISTEN_ALL) { + // Any non-LISTEN_UPSTREAM callback will necessarily concern a network that will + // also match the LISTEN_UPSTREAM callback by construction of the + // LISTEN_UPSTREAM callback. So it's not useful to do this work for + // non-LISTEN_UPSTREAM callbacks. + if (mCallbackType == CALLBACK_LISTEN_UPSTREAM) { recomputeLocalPrefixes(); } } diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java index b2cbf7560e..51ba140b11 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java @@ -18,6 +18,7 @@ package com.android.networkstack.tethering; import static android.net.NetworkCapabilities.NET_CAPABILITY_DUN; import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; +import static android.net.NetworkCapabilities.NET_CAPABILITY_LOCAL_NETWORK; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static com.android.networkstack.apishim.common.ShimUtils.isAtLeastS; @@ -41,6 +42,8 @@ import android.util.ArrayMap; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import com.android.modules.utils.build.SdkLevel; + import java.util.Map; import java.util.Objects; @@ -119,12 +122,15 @@ public class TestConnectivityManager extends ConnectivityManager { && mLegacyTypeMap.isEmpty(); } - boolean isListeningForAll() { - final NetworkCapabilities empty = new NetworkCapabilities(); - empty.clearAll(); + boolean isListeningForUpstream() { + final NetworkCapabilities upstreamNc = new NetworkCapabilities(); + upstreamNc.clearAll(); + if (SdkLevel.isAtLeastV()) { + upstreamNc.addForbiddenCapability(NET_CAPABILITY_LOCAL_NETWORK); + } for (NetworkRequestInfo nri : mListening.values()) { - if (nri.request.networkCapabilities.equalRequestableCapabilities(empty)) { + if (nri.request.networkCapabilities.equalRequestableCapabilities(upstreamNc)) { return true; } } diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java index 0c6a95d3d1..32f0756bfc 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -1032,7 +1032,7 @@ public class TetheringTest { verify(mWifiManager).updateInterfaceIpState(TEST_WLAN_IFNAME, expectedState); verifyNoMoreInteractions(mWifiManager); - verify(mUpstreamNetworkMonitor).startObserveAllNetworks(); + verify(mUpstreamNetworkMonitor).startObserveUpstreamNetworks(); if (isLocalOnly) { // There are 2 IpServer state change events: STATE_AVAILABLE -> STATE_LOCAL_ONLY. verify(mNotificationUpdater, times(2)).onDownstreamChanged(DOWNSTREAM_NONE); @@ -1260,7 +1260,7 @@ public class TetheringTest { // Start USB tethering with no current upstream. prepareUsbTethering(); sendUsbBroadcast(true, true, TETHER_USB_RNDIS_FUNCTION); - inOrder.verify(mUpstreamNetworkMonitor).startObserveAllNetworks(); + inOrder.verify(mUpstreamNetworkMonitor).startObserveUpstreamNetworks(); inOrder.verify(mUpstreamNetworkMonitor).setTryCell(true); // Pretend cellular connected and expect the upstream to be set. @@ -1859,7 +1859,7 @@ public class TetheringTest { // Start USB tethering with no current upstream. prepareUsbTethering(); sendUsbBroadcast(true, true, TETHER_USB_RNDIS_FUNCTION); - inOrder.verify(mUpstreamNetworkMonitor).startObserveAllNetworks(); + inOrder.verify(mUpstreamNetworkMonitor).startObserveUpstreamNetworks(); inOrder.verify(mUpstreamNetworkMonitor).setTryCell(true); ArgumentCaptor captor = ArgumentCaptor.forClass(NetworkCallback.class); inOrder.verify(mCm).requestNetwork(any(), eq(0), eq(TYPE_MOBILE_DUN), any(), @@ -2587,7 +2587,7 @@ public class TetheringTest { verify(mNetd, times(1)).tetherStartWithConfiguration(any()); verifyNoMoreInteractions(mNetd); verifyTetheringBroadcast(TEST_P2P_IFNAME, EXTRA_ACTIVE_LOCAL_ONLY); - verify(mUpstreamNetworkMonitor, times(1)).startObserveAllNetworks(); + verify(mUpstreamNetworkMonitor, times(1)).startObserveUpstreamNetworks(); // There are 2 IpServer state change events: STATE_AVAILABLE -> STATE_LOCAL_ONLY verify(mNotificationUpdater, times(2)).onDownstreamChanged(DOWNSTREAM_NONE); @@ -3757,7 +3757,7 @@ public class TetheringTest { verifyInterfaceServingModeStarted(TEST_P2P_IFNAME); verifyTetheringBroadcast(TEST_P2P_IFNAME, EXTRA_AVAILABLE_TETHER); verifyTetheringBroadcast(TEST_P2P_IFNAME, EXTRA_ACTIVE_LOCAL_ONLY); - verify(mUpstreamNetworkMonitor).startObserveAllNetworks(); + verify(mUpstreamNetworkMonitor).startObserveUpstreamNetworks(); // Verify never enable upstream if only P2P active. verify(mUpstreamNetworkMonitor, never()).setTryCell(true); assertEquals(TETHER_ERROR_NO_ERROR, mTethering.getLastErrorForTest(TEST_P2P_IFNAME)); diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java index 90fd7099ef..f192492342 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/UpstreamNetworkMonitorTest.java @@ -36,7 +36,6 @@ import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.inOrder; -import static org.mockito.Mockito.reset; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -141,7 +140,7 @@ public class UpstreamNetworkMonitorTest { assertTrue(mCM.hasNoCallbacks()); mUNM.startTrackDefaultNetwork(mEntitleMgr); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); assertEquals(1, mCM.mTrackingDefault.size()); mUNM.stop(); @@ -149,13 +148,13 @@ public class UpstreamNetworkMonitorTest { } @Test - public void testListensForAllNetworks() throws Exception { + public void testListensForUpstreamNetworks() throws Exception { assertTrue(mCM.mListening.isEmpty()); mUNM.startTrackDefaultNetwork(mEntitleMgr); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); assertFalse(mCM.mListening.isEmpty()); - assertTrue(mCM.isListeningForAll()); + assertTrue(mCM.isListeningForUpstream()); mUNM.stop(); assertTrue(mCM.onlyHasDefaultCallbacks()); @@ -179,7 +178,7 @@ public class UpstreamNetworkMonitorTest { assertTrue(TestConnectivityManager.looksLikeDefaultRequest(requestCaptor.getValue())); } - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); verify(mCM, times(1)).registerNetworkCallback( any(NetworkRequest.class), any(NetworkCallback.class), any(Handler.class)); @@ -192,7 +191,7 @@ public class UpstreamNetworkMonitorTest { assertFalse(mUNM.mobileNetworkRequested()); assertEquals(0, mCM.mRequested.size()); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); assertFalse(mUNM.mobileNetworkRequested()); assertEquals(0, mCM.mRequested.size()); @@ -215,7 +214,7 @@ public class UpstreamNetworkMonitorTest { assertFalse(mUNM.mobileNetworkRequested()); assertEquals(0, mCM.mRequested.size()); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); verify(mCM, times(1)).registerNetworkCallback( any(NetworkRequest.class), any(NetworkCallback.class), any(Handler.class)); assertFalse(mUNM.mobileNetworkRequested()); @@ -251,7 +250,7 @@ public class UpstreamNetworkMonitorTest { assertFalse(mUNM.mobileNetworkRequested()); assertEquals(0, mCM.mRequested.size()); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); assertFalse(mUNM.mobileNetworkRequested()); assertEquals(0, mCM.mRequested.size()); @@ -271,7 +270,7 @@ public class UpstreamNetworkMonitorTest { @Test public void testUpdateMobileRequiresDun() throws Exception { - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); // Test going from no-DUN to DUN correctly re-registers callbacks. mUNM.setUpstreamConfig(false /* autoUpstream */, false /* dunRequired */); @@ -301,7 +300,7 @@ public class UpstreamNetworkMonitorTest { preferredTypes.add(TYPE_WIFI); mUNM.startTrackDefaultNetwork(mEntitleMgr); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); // There are no networks, so there is nothing to select. assertSatisfiesLegacyType(TYPE_NONE, mUNM.selectPreferredUpstreamType(preferredTypes)); @@ -374,7 +373,7 @@ public class UpstreamNetworkMonitorTest { @Test public void testGetCurrentPreferredUpstream() throws Exception { mUNM.startTrackDefaultNetwork(mEntitleMgr); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); mUNM.setUpstreamConfig(true /* autoUpstream */, false /* dunRequired */); mUNM.setTryCell(true); @@ -446,7 +445,7 @@ public class UpstreamNetworkMonitorTest { @Test public void testLocalPrefixes() throws Exception { mUNM.startTrackDefaultNetwork(mEntitleMgr); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); // [0] Test minimum set of local prefixes. Set local = mUNM.getLocalPrefixes(); @@ -558,7 +557,7 @@ public class UpstreamNetworkMonitorTest { preferredTypes.add(TYPE_MOBILE_HIPRI); preferredTypes.add(TYPE_WIFI); mUNM.startTrackDefaultNetwork(mEntitleMgr); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); // Setup wifi and make wifi as default network. final TestNetworkAgent wifiAgent = new TestNetworkAgent(mCM, WIFI_CAPABILITIES); wifiAgent.fakeConnect(); @@ -579,7 +578,7 @@ public class UpstreamNetworkMonitorTest { final String ipv6Addr1 = "2001:db8:4:fd00:827a:bfff:fe6f:374d/64"; final String ipv6Addr2 = "2003:aa8:3::123/64"; mUNM.startTrackDefaultNetwork(mEntitleMgr); - mUNM.startObserveAllNetworks(); + mUNM.startObserveUpstreamNetworks(); mUNM.setUpstreamConfig(true /* autoUpstream */, false /* dunRequired */); mUNM.setTryCell(true);