Stop copying directly-connected routes to the main table, #2.

For a long time we have thought that copying directly-connected
routes to the main table was necessary to add gatewayed routes
to other routing tables. However, this is not necessary when the
directly-connected routes are properly created with "scope link"
as we do in http://ag/513100 .

Delete the copying code, but don't delete the rule that looks up
the main table or the code that dumps it. The main table is used
for things like cell networking, because the RIL emulates cell
networks, which are actually point-to-point, as directly
connected broadcast subnets (e.g., a /30 or a /27) with a fake
default gateway.  The directly-connected route that covers the
fake default gateway is implicitly created by adding the IP
address, but it's in the main table, so we can't add the default
route without looking up the main table.

Change-Id: I93bd4764ac75fdcc98fa4206c601524100d53fc3

diff --git a/server/RouteController.cpp b/server/RouteController.cpp
index 943a4c1..fca70a6 100644 (file)
--- a/server/RouteController.cpp
+++ b/server/RouteController.cpp
@@ -49,7 +49,7 @@ const uint32_t RULE_PRIORITY_BYPASSABLE_VPN      = 20000;
 const uint32_t RULE_PRIORITY_VPN_FALLTHROUGH     = 21000;
 const uint32_t RULE_PRIORITY_DEFAULT_NETWORK     = 22000;
 const uint32_t RULE_PRIORITY_DIRECTLY_CONNECTED  = 23000;
-const uint32_t RULE_PRIORITY_UNREACHABLE         = 24000;
+const uint32_t RULE_PRIORITY_UNREACHABLE         = 32000;
 
 const uint32_t ROUTE_TABLE_LOCAL_NETWORK  = 97;
 const uint32_t ROUTE_TABLE_LEGACY_NETWORK = 98;
@@ -639,10 +639,10 @@ WARN_UNUSED_RESULT int addLocalNetworkRules(unsigned localNetId) {
 }
 
 // Add a new rule to look up the 'main' table, with the same selectors as the "default network"
-// rule, but with a lower priority. Since the default network rule points to a table with a default
-// route, the rule we're adding will never be used for normal routing lookups. However, the kernel
-// may fall-through to it to find directly-connected routes when it validates that a nexthop (in a
-// route being added) is reachable.
+// rule, but with a lower priority. We will never create routes in the main table; it should only be
+// used for directly-connected routes implicitly created by the kernel when adding IP addresses.
+// This is necessary, for example, when adding a route through a directly-connected gateway: in
+// order to add the route, there must already be a directly-connected route that covers the gateway.
 WARN_UNUSED_RESULT int addDirectlyConnectedRule() {
     Fwmark fwmark;
     Fwmark mask;
@@ -654,8 +654,10 @@ WARN_UNUSED_RESULT int addDirectlyConnectedRule() {
                         fwmark.intValue, mask.intValue, IIF_NONE, OIF_NONE, UID_ROOT, UID_ROOT);
 }
 
-// Add a rule to preempt the pre-defined "from all lookup main" rule. Packets that reach this rule
-// will be null-routed, and won't fall-through to the main table.
+// Add an explicit unreachable rule close to the end of the prioriy list to make it clear that
+// relying on the kernel-default "from all lookup main" rule at priority 32766 is not intended
+// behaviour. We do flush the kernel-default rules at startup, but having an explicit unreachable
+// rule will hopefully make things even clearer.
 WARN_UNUSED_RESULT int addUnreachableRule() {
     return modifyIpRule(RTM_NEWRULE, RULE_PRIORITY_UNREACHABLE, RT_TABLE_UNSPEC, MARK_UNSET,
                         MARK_UNSET);
@@ -813,18 +815,6 @@ WARN_UNUSED_RESULT int modifyRoute(uint16_t action, const char* interface, const
         return ret;
     }
 
-    // If there's no nexthop, this is a directly connected route. Add it to the main table also, to
-    // let the kernel find it when validating nexthops when global routes are added.
-    if (!nexthop) {
-        ret = modifyIpRoute(action, RT_TABLE_MAIN, interface, destination, NULL);
-        // A failure with action == ADD && errno == EEXIST means that the route already exists in
-        // the main table, perhaps because the kernel added it automatically as part of adding the
-        // IP address to the interface. Ignore this, but complain about everything else.
-        if (ret && !(action == RTM_NEWROUTE && ret == -EEXIST)) {
-            return ret;
-        }
-    }
-
     return 0;
 }