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

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 4393e3527e4d..22849e5f58c5 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<IpPrefix> 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 53a9b5a6befb..94b9b4dc9612 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<String> 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();
     }