From f1fe8ee928f10e15a26059070fb8577fa5cc8c34 Mon Sep 17 00:00:00 2001
From: Patrick Rohr <prohr@google.com>
Date: Wed, 2 Mar 2022 15:14:07 +0100
Subject: [PATCH] Do not enable ingress rate limit until clsact qdisc exists

The tc police filter attaches to the clsact qdisc, so the rate limit
cannot be enabled before the qdisc is added to the interface.
The clsact qdisc is added as part of INetd#networkAddInterface, which is
called from inside updateLinkProperties.

Test: atest FrameworksNetTests:ConnectivityServiceTest
Change-Id: I0713605ff3684f8271eb3f0e29ab7116561963f1
---
 .../android/server/ConnectivityService.java   | 26 +++++++++----------
 .../server/ConnectivityServiceTest.java       |  7 +++++
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index c90fcd5adb..eabcd10c25 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -4242,7 +4242,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
             mDnsManager.removeNetwork(nai.network);
 
             // clean up tc police filters on interface.
-            if (canNetworkBeRateLimited(nai) && mIngressRateLimit >= 0) {
+            if (nai.everConnected && canNetworkBeRateLimited(nai) && mIngressRateLimit >= 0) {
                 mDeps.disableIngressRateLimit(nai.linkProperties.getInterfaceName());
             }
         }
@@ -9011,19 +9011,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
             // A network that has just connected has zero requests and is thus a foreground network.
             networkAgent.networkCapabilities.addCapability(NET_CAPABILITY_FOREGROUND);
 
-            // If a rate limit has been configured and is applicable to this network (network
-            // provides internet connectivity), apply it.
-            // Note: in case of a system server crash, there is a very small chance that this
-            // leaves some interfaces rate limited (i.e. if the rate limit had been changed just
-            // before the crash and was never applied). One solution would be to delete all
-            // potential tc police filters every time this is called. Since this is an unlikely
-            // scenario in the first place (and worst case, the interface stays rate limited until
-            // the device is rebooted), this seems a little overkill.
-            if (canNetworkBeRateLimited(networkAgent) && mIngressRateLimit >= 0) {
-                mDeps.enableIngressRateLimit(networkAgent.linkProperties.getInterfaceName(),
-                        mIngressRateLimit);
-            }
-
             if (!createNativeNetwork(networkAgent)) return;
             if (networkAgent.propagateUnderlyingCapabilities()) {
                 // Initialize the network's capabilities to their starting values according to the
@@ -9047,6 +9034,17 @@ public class ConnectivityService extends IConnectivityManager.Stub
             updateLinkProperties(networkAgent, new LinkProperties(networkAgent.linkProperties),
                     null);
 
+            // If a rate limit has been configured and is applicable to this network (network
+            // provides internet connectivity), apply it. The tc police filter cannot be attached
+            // before the clsact qdisc is added which happens as part of updateLinkProperties ->
+            // updateInterfaces -> INetd#networkAddInterface.
+            // Note: in case of a system server crash, the NetworkController constructor in netd
+            // (called when netd starts up) deletes the clsact qdisc of all interfaces.
+            if (canNetworkBeRateLimited(networkAgent) && mIngressRateLimit >= 0) {
+                mDeps.enableIngressRateLimit(networkAgent.linkProperties.getInterfaceName(),
+                        mIngressRateLimit);
+            }
+
             // Until parceled LinkProperties are sent directly to NetworkMonitor, the connect
             // command must be sent after updating LinkProperties to maximize chances of
             // NetworkMonitor seeing the correct LinkProperties when starting.
diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index 098d48a734..777da1714c 100644
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -1998,6 +1998,13 @@ public class ConnectivityServiceTest {
             // updated. Check that this happened.
             assertEquals(-1L, (long) mActiveRateLimit.getOrDefault(iface, -1L));
             mActiveRateLimit.put(iface, rateInBytesPerSecond);
+            // verify that clsact qdisc has already been created, otherwise attaching a tc police
+            // filter will fail.
+            try {
+                verify(mMockNetd).networkAddInterface(anyInt(), eq(iface));
+            } catch (RemoteException e) {
+                fail(e.getMessage());
+            }
         }
 
         @Override