From d743601a002ac12c02da58e92ebd0544ab0b77ea 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 Change-Id: I260f5450f8b67f055983af68fb23a5f3cfc0bc69 --- .../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 4393e3527e4d2..22849e5f58c54 100644 --- a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java +++ b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java @@ -127,21 +127,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 @@ -149,11 +153,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, @@ -181,6 +186,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)); } @@ -193,6 +199,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; @@ -288,10 +297,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) { @@ -325,8 +331,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); } @@ -365,9 +372,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 53a9b5a6befb6..94b9b4dc96121 100644 --- a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java +++ b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java @@ -110,7 +110,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() { @@ -256,6 +258,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"; @@ -273,6 +276,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"; @@ -283,6 +287,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"; @@ -296,6 +301,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"; @@ -310,6 +316,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(); @@ -328,6 +335,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 @@ -359,6 +367,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. @@ -504,6 +513,7 @@ public class OffloadControllerTest { offload.setUpstreamLinkProperties(lp); provider.setInterfaceQuota(mobileIface, mobileLimit); waitForIdle(); + inOrder.verify(mHardware).getForwardedStats(ethernetIface); inOrder.verify(mHardware).stopOffloadControl(); }