From ddce7ee20fcad91bd45f77222d762136587bc192 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 21 Aug 2017 12:34:50 +0900 Subject: [PATCH] Don't completely stop offload if setting data limit fails. Currently, if setting a data limit fails, we completely stop offload in order to avoid data overages. However, the next thing we do is try to fetch the stats and crash, because once offload is stopped all our local state is cleared. Fix this by fetching stats before we stop offload. Bug: 29337859 Bug: 32163131 Bug: 64867836 Test: OffloadControllerTest passes Test: no crash when disabling wifi tethering with BT tethering active Merged-In: I7fc47e60b2da5f39c26fb22c1325618f9948dd38 Merged-In: I464dd2a6d1996b1cfb8bbf82b6ee453fd0747569 Change-Id: I260f5450f8b67f055983af68fb23a5f3cfc0bc69 (cherry picked from commit d743601a002ac12c02da58e92ebd0544ab0b77ea) --- .../connectivity/tethering/OffloadController.java | 24 ++++++++++++++++------ .../tethering/OffloadControllerTest.java | 10 +++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java index bcc74c0d49e4..ef18e4e0ba75 100644 --- a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java +++ b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java @@ -131,21 +131,25 @@ public class OffloadController { new OffloadHardwareInterface.ControlCallback() { @Override public void onStarted() { + if (!started()) return; mLog.log("onStarted"); } @Override public void onStoppedError() { + if (!started()) return; mLog.log("onStoppedError"); } @Override public void onStoppedUnsupported() { + if (!started()) return; mLog.log("onStoppedUnsupported"); } @Override public void onSupportAvailable() { + if (!started()) return; mLog.log("onSupportAvailable"); // [1] Poll for statistics and notify NetworkStats @@ -153,11 +157,12 @@ public class OffloadController { // [a] push local prefixes // [b] push downstreams // [c] push upstream parameters - pushUpstreamParameters(); + pushUpstreamParameters(null); } @Override public void onStoppedLimitReached() { + if (!started()) return; mLog.log("onStoppedLimitReached"); // We cannot reliably determine on which interface the limit was reached, @@ -185,6 +190,7 @@ public class OffloadController { public void onNatTimeoutUpdate(int proto, String srcAddr, int srcPort, String dstAddr, int dstPort) { + if (!started()) return; mLog.log(String.format("NAT timeout update: %s (%s,%s) -> (%s,%s)", proto, srcAddr, srcPort, dstAddr, dstPort)); } @@ -197,6 +203,9 @@ public class OffloadController { } public void stop() { + // Completely stops tethering offload. After this method is called, it is no longer safe to + // call any HAL method, no callbacks from the hardware will be delivered, and any in-flight + // callbacks must be ignored. Offload may be started again by calling start(). final boolean wasStarted = started(); updateStatsForCurrentUpstream(); mUpstreamLinkProperties = null; @@ -305,10 +314,7 @@ public class OffloadController { // onOffloadEvent() callback to tell us offload is available again and // then reapply all state). computeAndPushLocalPrefixes(); - pushUpstreamParameters(); - - // Update stats after we've told the hardware to change routing so we don't miss packets. - maybeUpdateStats(prevUpstream); + pushUpstreamParameters(prevUpstream); } public void setLocalPrefixes(Set localPrefixes) { @@ -342,8 +348,9 @@ public class OffloadController { return mConfigInitialized && mControlInitialized; } - private boolean pushUpstreamParameters() { + private boolean pushUpstreamParameters(String prevUpstream) { if (mUpstreamLinkProperties == null) { + maybeUpdateStats(prevUpstream); return mHwInterface.setUpstreamParameters(null, null, null, null); } @@ -382,9 +389,14 @@ public class OffloadController { return success; } + // Update stats after we've told the hardware to change routing so we don't miss packets. + maybeUpdateStats(prevUpstream); + // Data limits can only be set once offload is running on the upstream. success = maybeUpdateDataLimit(iface); if (!success) { + // If we failed to set a data limit, don't use this upstream, because we don't want to + // blow through the data limit that we were told to apply. mLog.log("Setting data limit for " + iface + " failed, disabling offload."); stop(); } diff --git a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java index 3f17b9eb9817..c769492424a4 100644 --- a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java +++ b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java @@ -112,7 +112,9 @@ public class OffloadControllerTest { when(mHardware.initOffloadConfig()).thenReturn(true); when(mHardware.initOffloadControl(mControlCallbackCaptor.capture())) .thenReturn(true); + when(mHardware.setUpstreamParameters(anyString(), any(), any(), any())).thenReturn(true); when(mHardware.getForwardedStats(any())).thenReturn(new ForwardedStats()); + when(mHardware.setDataLimit(anyString(), anyLong())).thenReturn(true); } private void enableOffload() { @@ -260,6 +262,7 @@ public class OffloadControllerTest { inOrder.verify(mHardware, never()).setLocalPrefixes(mStringArrayCaptor.capture()); inOrder.verify(mHardware, times(1)).setUpstreamParameters( eq(testIfName), eq(null), eq(null), eq(null)); + inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE)); inOrder.verifyNoMoreInteractions(); final String ipv4Addr = "192.0.2.5"; @@ -277,6 +280,7 @@ public class OffloadControllerTest { inOrder.verify(mHardware, times(1)).setUpstreamParameters( eq(testIfName), eq(ipv4Addr), eq(null), eq(null)); inOrder.verify(mHardware, times(1)).getForwardedStats(eq(testIfName)); + inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE)); inOrder.verifyNoMoreInteractions(); final String ipv4Gateway = "192.0.2.1"; @@ -287,6 +291,7 @@ public class OffloadControllerTest { inOrder.verify(mHardware, times(1)).setUpstreamParameters( eq(testIfName), eq(ipv4Addr), eq(ipv4Gateway), eq(null)); inOrder.verify(mHardware, times(1)).getForwardedStats(eq(testIfName)); + inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE)); inOrder.verifyNoMoreInteractions(); final String ipv6Gw1 = "fe80::cafe"; @@ -300,6 +305,7 @@ public class OffloadControllerTest { ArrayList v6gws = mStringArrayCaptor.getValue(); assertEquals(1, v6gws.size()); assertTrue(v6gws.contains(ipv6Gw1)); + inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE)); inOrder.verifyNoMoreInteractions(); final String ipv6Gw2 = "fe80::d00d"; @@ -314,6 +320,7 @@ public class OffloadControllerTest { assertEquals(2, v6gws.size()); assertTrue(v6gws.contains(ipv6Gw1)); assertTrue(v6gws.contains(ipv6Gw2)); + inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE)); inOrder.verifyNoMoreInteractions(); final LinkProperties stacked = new LinkProperties(); @@ -332,6 +339,7 @@ public class OffloadControllerTest { assertEquals(2, v6gws.size()); assertTrue(v6gws.contains(ipv6Gw1)); assertTrue(v6gws.contains(ipv6Gw2)); + inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE)); inOrder.verifyNoMoreInteractions(); // Add in some IPv6 upstream info. When there is a tethered downstream @@ -363,6 +371,7 @@ public class OffloadControllerTest { assertTrue(v6gws.contains(ipv6Gw1)); assertTrue(v6gws.contains(ipv6Gw2)); inOrder.verify(mHardware, times(1)).getForwardedStats(eq(testIfName)); + inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE)); inOrder.verifyNoMoreInteractions(); // Completely identical LinkProperties updates are de-duped. @@ -524,6 +533,7 @@ public class OffloadControllerTest { offload.setUpstreamLinkProperties(lp); provider.setInterfaceQuota(mobileIface, mobileLimit); waitForIdle(); + inOrder.verify(mHardware).getForwardedStats(ethernetIface); inOrder.verify(mHardware).stopOffloadControl(); } -- 2.11.0