Merge changes I6e6b40b0,I6a0d93e0,Ie26f6e52 am: 1004c23b35

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/1772430

Change-Id: I5283354f545ef3f9b01f9327e2558314aab1f8f0

Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java

@@ -371,7 +371,6 @@ public class BpfCoordinatorShimImpl
         } catch (IllegalStateException e) {
             // Silent if the rule already exists. Note that the errno EEXIST was rethrown as
             // IllegalStateException. See BpfMap#insertEntry.
-            return false;
         }
         return true;
     }

Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java

@@ -141,11 +141,6 @@ public abstract class BpfCoordinatorShim {
 
     /**
      * Adds a tethering IPv4 offload rule to appropriate BPF map.
-     *
-     * @param downstream true if downstream, false if upstream.
-     * @param key the key to add.
-     * @param value the value to add.
-     * @return true iff the map was modified, false if the key exists or there was an error.
      */
     public abstract boolean tetherOffloadRuleAdd(boolean downstream, @NonNull Tether4Key key,
             @NonNull Tether4Value value);

Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java

@@ -124,21 +124,10 @@ public class BpfCoordinator {
         return makeMapPath((downstream ? "downstream" : "upstream") + ipVersion);
     }
 
-    // TODO: probably to remember what the timeout updated things to last is. But that requires
-    // either r/w map entries (which seems bad/racy) or a separate map to keep track of all flows
-    // and remember when they were updated and with what timeout.
     @VisibleForTesting
     static final int CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS = 60_000;
     @VisibleForTesting
-    static final int CONNTRACK_TIMEOUT_UPDATE_SLACK_MS = 20_000;
-
-    // Default timeouts sync from /proc/sys/net/netfilter/nf_conntrack_*
-    // See also kernel document nf_conntrack-sysctl.txt.
-    @VisibleForTesting
     static final int NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED = 432_000;
-    static final int NF_CONNTRACK_TCP_TIMEOUT_UNACKNOWLEDGED = 300;
-    // The default value is 120 for 5.10 and that thus the periodicity of the updates of 60s is
-    // low enough to support all ACK kernels.
     @VisibleForTesting
     static final int NF_CONNTRACK_UDP_TIMEOUT_STREAM = 180;
 
@@ -1578,34 +1567,8 @@ public class BpfCoordinator {
             final Tether4Key downstream4Key = makeTetherDownstream4Key(e, tetherClient,
                     upstreamIndex);
 
-            final boolean isConntrackEventDelete =
+            if (e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8
-                    e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8
+                    | NetlinkConstants.IPCTNL_MSG_CT_DELETE)) {
-                    | NetlinkConstants.IPCTNL_MSG_CT_DELETE);
-
-            // Using the timeout to distinguish tcp state is not a decent way. Need to fix.
-            // The received IPCTNL_MSG_CT_NEW must pass ConntrackMonitor#isEstablishedNatSession
-            // which checks CTA_STATUS. It implies that this entry has at least reached tcp
-            // state "established". For safety, treat any timeout which is equal or larger than 300
-            // seconds (UNACKNOWLEDGED, ESTABLISHED, ..) to be "established".
-            // TODO: parse tcp state in conntrack monitor.
-            final boolean isTcpEstablished =
-                    e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8
-                    | NetlinkConstants.IPCTNL_MSG_CT_NEW)
-                    && e.tupleOrig.protoNum == OsConstants.IPPROTO_TCP
-                    && (e.timeoutSec >= NF_CONNTRACK_TCP_TIMEOUT_UNACKNOWLEDGED);
-
-            final boolean isTcpNonEstablished =
-                    e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8
-                    | NetlinkConstants.IPCTNL_MSG_CT_NEW)
-                    && e.tupleOrig.protoNum == OsConstants.IPPROTO_TCP
-                    && (e.timeoutSec < NF_CONNTRACK_TCP_TIMEOUT_UNACKNOWLEDGED);
-
-            // Delete the BPF rules:
-            // 1. Contrack event IPCTNL_MSG_CT_DELETE received.
-            // 2. For TCP conntrack entry, the tcp state has left "established" and going to be
-            // closed.
-            // TODO: continue to offload half-closed tcp connections.
-            if (isConntrackEventDelete || isTcpNonEstablished) {
                 final boolean deletedUpstream = mBpfCoordinatorShim.tetherOffloadRuleRemove(
                         UPSTREAM, upstream4Key);
                 final boolean deletedDownstream = mBpfCoordinatorShim.tetherOffloadRuleRemove(
@@ -1620,7 +1583,6 @@ public class BpfCoordinator {
                     Log.wtf(TAG, "The bidirectional rules should be removed concurrently ("
                             + "upstream: " + deletedUpstream
                             + ", downstream: " + deletedDownstream + ")");
-                    // TODO: consider better error handling for the stubs {rule, limit, ..}.
                     return;
                 }
 
@@ -1631,41 +1593,11 @@ public class BpfCoordinator {
             final Tether4Value upstream4Value = makeTetherUpstream4Value(e, upstreamIndex);
             final Tether4Value downstream4Value = makeTetherDownstream4Value(e, tetherClient,
                     upstreamIndex);
+
             maybeAddDevMap(upstreamIndex, tetherClient.downstreamIfindex);
             maybeSetLimit(upstreamIndex);
-
+            mBpfCoordinatorShim.tetherOffloadRuleAdd(UPSTREAM, upstream4Key, upstream4Value);
-            final boolean upstreamAdded = mBpfCoordinatorShim.tetherOffloadRuleAdd(UPSTREAM,
+            mBpfCoordinatorShim.tetherOffloadRuleAdd(DOWNSTREAM, downstream4Key, downstream4Value);
-                    upstream4Key, upstream4Value);
-            final boolean downstreamAdded = mBpfCoordinatorShim.tetherOffloadRuleAdd(DOWNSTREAM,
-                    downstream4Key, downstream4Value);
-
-            if (upstreamAdded != downstreamAdded) {
-                mLog.e("The bidirectional rules should be added or not added concurrently ("
-                        + "upstream: " + upstreamAdded
-                        + ", downstream: " + downstreamAdded + "). "
-                        + "Remove the added rules.");
-                if (upstreamAdded) {
-                    mBpfCoordinatorShim.tetherOffloadRuleRemove(UPSTREAM, upstream4Key);
-                }
-                if (downstreamAdded) {
-                    mBpfCoordinatorShim.tetherOffloadRuleRemove(DOWNSTREAM, downstream4Key);
-                }
-                return;
-            }
-
-            // Update TCP timeout iif it is first time we're adding the rules. Needed because a
-            // payload data packet may have gone through non-offload path, before we added offload
-            // rules, and that this may result in in-kernel conntrack state being in ESTABLISHED
-            // but pending ACK (ie. UNACKED) state. But the in-kernel conntrack might never see the
-            // ACK because we just added offload rules. As such after adding the rules we need to
-            // force the timeout back to the normal ESTABLISHED timeout of 5 days. Note that
-            // updating the timeout will trigger another netlink event with the updated timeout.
-            // TODO: Remove this once the tcp state is parsed.
-            if (isTcpEstablished && upstreamAdded && downstreamAdded) {
-                updateConntrackTimeout((byte) upstream4Key.l4proto,
-                        parseIPv4Address(upstream4Key.src4), (short) upstream4Key.srcPort,
-                        parseIPv4Address(upstream4Key.dst4), (short) upstream4Key.dstPort);
-            }
         }
     }
 
@@ -1948,7 +1880,6 @@ public class BpfCoordinator {
         // - proc/sys/net/netfilter/nf_conntrack_tcp_timeout_established
         // - proc/sys/net/netfilter/nf_conntrack_udp_timeout_stream
         // See kernel document nf_conntrack-sysctl.txt.
-        // TODO: we should account for the fact that lastUsed is in the past and not exactly now.
         final int timeoutSec = (proto == OsConstants.IPPROTO_TCP)
                 ? NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED
                 : NF_CONNTRACK_UDP_TIMEOUT_STREAM;
@@ -1979,23 +1910,6 @@ public class BpfCoordinator {
         }
     }
 
-    boolean requireConntrackTimeoutUpdate(long nowNs, long lastUsedNs, int proto) {
-        // Refreshing tcp timeout without checking tcp state may make the conntrack entry live
-        // 5 days (432000s) even while the session is being closed. Its BPF rule may not be
-        // deleted for 5 days because the tcp state gets stuck and conntrack delete message is
-        // not sent. Note that both the conntrack monitor and refreshing timeout updater are
-        // in the same thread. Beware while the tcp status may be changed running in refreshing
-        // timeout updater and may read out-of-date tcp stats.
-        // See nf_conntrack_tcp_timeout_established in kernel document.
-        // TODO: support refreshing TCP conntrack timeout.
-        if (proto == OsConstants.IPPROTO_TCP) return false;
-
-        // The timeout requirement check needs the slack time because the scheduled timer may
-        // be not precise. The timeout update has a chance to be missed.
-        return (nowNs - lastUsedNs) < (long) (CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS
-                + CONNTRACK_TIMEOUT_UPDATE_SLACK_MS) * 1_000_000;
-    }
-
     private void refreshAllConntrackTimeouts() {
         final long now = mDeps.elapsedRealtimeNanos();
 
@@ -2003,7 +1917,7 @@ public class BpfCoordinator {
         // because TCP is a bidirectional traffic. Probably don't need to extend timeout by
         // both directions for TCP.
         mBpfCoordinatorShim.tetherOffloadRuleForEach(UPSTREAM, (k, v) -> {
-            if (requireConntrackTimeoutUpdate(now, v.lastUsed, k.l4proto)) {
+            if ((now - v.lastUsed) / 1_000_000 < CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS) {
                 updateConntrackTimeout((byte) k.l4proto,
                         parseIPv4Address(k.src4), (short) k.srcPort,
                         parseIPv4Address(k.dst4), (short) k.dstPort);
@@ -2011,10 +1925,10 @@ public class BpfCoordinator {
         });
 
         // Reverse the source and destination {address, port} from downstream value because
-        // #updateConntrackTimeout refreshes the timeout of netlink attribute CTA_TUPLE_ORIG
+        // #updateConntrackTimeout refresh the timeout of netlink attribute CTA_TUPLE_ORIG
         // which is opposite direction for downstream map value.
         mBpfCoordinatorShim.tetherOffloadRuleForEach(DOWNSTREAM, (k, v) -> {
-            if (requireConntrackTimeoutUpdate(now, v.lastUsed, k.l4proto)) {
+            if ((now - v.lastUsed) / 1_000_000 < CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS) {
                 updateConntrackTimeout((byte) k.l4proto,
                         parseIPv4Address(v.dst46), (short) v.dstPort,
                         parseIPv4Address(v.src46), (short) v.srcPort);

Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java

@@ -41,7 +41,6 @@ import static android.system.OsConstants.NETLINK_NETFILTER;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.staticMockMarker;
 import static com.android.networkstack.tethering.BpfCoordinator.CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS;
-import static com.android.networkstack.tethering.BpfCoordinator.CONNTRACK_TIMEOUT_UPDATE_SLACK_MS;
 import static com.android.networkstack.tethering.BpfCoordinator.NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED;
 import static com.android.networkstack.tethering.BpfCoordinator.NF_CONNTRACK_UDP_TIMEOUT_STREAM;
 import static com.android.networkstack.tethering.BpfCoordinator.StatsType;
@@ -1379,14 +1378,8 @@ public class BpfCoordinatorTest {
         }
 
         final int status = (msgType == IPCTNL_MSG_CT_NEW) ? ESTABLISHED_MASK : DYING_MASK;
-        int timeoutSec;
+        final int timeoutSec = (msgType == IPCTNL_MSG_CT_NEW) ? 100 /* nonzero, new */
-        if (msgType == IPCTNL_MSG_CT_NEW) {
+                : 0 /* unused, delete */;
-            timeoutSec = (proto == IPPROTO_TCP)
-                    ? NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED
-                    : NF_CONNTRACK_UDP_TIMEOUT_STREAM;
-        } else {
-            timeoutSec = 0; /* unused, CT_DELETE */
-        }
         return new ConntrackEvent(
                 (short) (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8 | msgType),
                 new Tuple(new TupleIpv4(PRIVATE_ADDR, REMOTE_ADDR),
@@ -1542,27 +1535,23 @@ public class BpfCoordinatorTest {
     }
 
     private void checkRefreshConntrackTimeout(final TestBpfMap<Tether4Key, Tether4Value> bpfMap,
-            int proto, final Tether4Key key, final Tether4Value value) throws Exception {
+            final Tether4Key tcpKey, final Tether4Value tcpValue, final Tether4Key udpKey,
-        if (proto != IPPROTO_TCP && proto != IPPROTO_UDP) {
+            final Tether4Value udpValue) throws Exception {
-            fail("Not support protocol " + proto);
-        }
         // Both system elapsed time since boot and the rule last used time are used to measure
         // the rule expiration. In this test, all test rules are fixed the last used time to 0.
         // Set the different testing elapsed time to make the rule to be valid or expired.
         //
         // Timeline:
-        //                                    CONNTRACK_TIMEOUT_UPDATE_SLACK_MS
+        // 0                                       60 (seconds)
-        //                                               ->|    |<-
+        // +---+---+---+---+--...--+---+---+---+---+---+- ..
-        // +---+---+---+---+---+--...--+---+---+---+---+---+-..-+---+-- ..
+        // | CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS  |
-        // |     CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS      |
+        // +---+---+---+---+--...--+---+---+---+---+---+- ..
-        // +---+---+---+---+---+--...--+---+---+---+---+---+-..-+---+-- ..
+        // |<-          valid diff           ->|
-        // |<-                valid diff                 ->|
+        // |<-          expired diff                 ->|
-        // |<-                expired diff                        ->|
+        // ^                                   ^       ^
-        // ^                                               ^        ^
+        // last used time      elapsed time (valid)    elapsed time (expired)
-        // last used time               elapsed time (valid)    elapsed time (expired)
+        final long validTime = (CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS - 1) * 1_000_000L;
-        final long validTimeNs = CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS * 1_000_000L;
+        final long expiredTime = (CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS + 1) * 1_000_000L;
-        final long expiredTimeNs = (CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS
-                + CONNTRACK_TIMEOUT_UPDATE_SLACK_MS + 1) * 1_000_000L;
 
         // Static mocking for NetlinkSocket.
         MockitoSession mockSession = ExtendedMockito.mockitoSession()
@@ -1571,27 +1560,30 @@ public class BpfCoordinatorTest {
         try {
             final BpfCoordinator coordinator = makeBpfCoordinator();
             coordinator.startPolling();
-            bpfMap.insertEntry(key, value);
+            bpfMap.insertEntry(tcpKey, tcpValue);
+            bpfMap.insertEntry(udpKey, udpValue);
 
             // [1] Don't refresh contrack timeout.
-            setElapsedRealtimeNanos(expiredTimeNs);
+            setElapsedRealtimeNanos(expiredTime);
             mTestLooper.moveTimeForward(CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS);
             waitForIdle();
             ExtendedMockito.verifyNoMoreInteractions(staticMockMarker(NetlinkSocket.class));
             ExtendedMockito.clearInvocations(staticMockMarker(NetlinkSocket.class));
 
-            // [2] Refresh contrack timeout. UDP Only.
+            // [2] Refresh contrack timeout.
-            setElapsedRealtimeNanos(validTimeNs);
+            setElapsedRealtimeNanos(validTime);
             mTestLooper.moveTimeForward(CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS);
             waitForIdle();
-
+            final byte[] expectedNetlinkTcp = ConntrackMessage.newIPv4TimeoutUpdateRequest(
-            if (proto == IPPROTO_UDP) {
+                    IPPROTO_TCP, PRIVATE_ADDR, (int) PRIVATE_PORT, REMOTE_ADDR,
-                final byte[] expectedNetlinkUdp = ConntrackMessage.newIPv4TimeoutUpdateRequest(
+                    (int) REMOTE_PORT, NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED);
-                        IPPROTO_UDP, PRIVATE_ADDR, (int) PRIVATE_PORT, REMOTE_ADDR,
+            final byte[] expectedNetlinkUdp = ConntrackMessage.newIPv4TimeoutUpdateRequest(
-                        (int) REMOTE_PORT, NF_CONNTRACK_UDP_TIMEOUT_STREAM);
+                    IPPROTO_UDP, PRIVATE_ADDR, (int) PRIVATE_PORT, REMOTE_ADDR,
-                ExtendedMockito.verify(() -> NetlinkSocket.sendOneShotKernelMessage(
+                    (int) REMOTE_PORT, NF_CONNTRACK_UDP_TIMEOUT_STREAM);
-                        eq(NETLINK_NETFILTER), eq(expectedNetlinkUdp)));
+            ExtendedMockito.verify(() -> NetlinkSocket.sendOneShotKernelMessage(
-            }
+                    eq(NETLINK_NETFILTER), eq(expectedNetlinkTcp)));
+            ExtendedMockito.verify(() -> NetlinkSocket.sendOneShotKernelMessage(
+                    eq(NETLINK_NETFILTER), eq(expectedNetlinkUdp)));
             ExtendedMockito.verifyNoMoreInteractions(staticMockMarker(NetlinkSocket.class));
             ExtendedMockito.clearInvocations(staticMockMarker(NetlinkSocket.class));
 
@@ -1608,57 +1600,33 @@ public class BpfCoordinatorTest {
 
     @Test
     @IgnoreUpTo(Build.VERSION_CODES.R)
-    public void testRefreshConntrackTimeout_Upstream4MapTcp() throws Exception {
+    public void testRefreshConntrackTimeout_Upstream4Map() throws Exception {
         // TODO: Replace the dependencies BPF map with a non-mocked TestBpfMap object.
         final TestBpfMap<Tether4Key, Tether4Value> bpfUpstream4Map =
                 new TestBpfMap<>(Tether4Key.class, Tether4Value.class);
         doReturn(bpfUpstream4Map).when(mDeps).getBpfUpstream4Map();
 
-        final Tether4Key key = makeUpstream4Key(IPPROTO_TCP);
+        final Tether4Key tcpKey = makeUpstream4Key(IPPROTO_TCP);
-        final Tether4Value value = makeUpstream4Value();
+        final Tether4Key udpKey = makeUpstream4Key(IPPROTO_UDP);
+        final Tether4Value tcpValue = makeUpstream4Value();
+        final Tether4Value udpValue = makeUpstream4Value();
 
-        checkRefreshConntrackTimeout(bpfUpstream4Map, IPPROTO_TCP, key, value);
+        checkRefreshConntrackTimeout(bpfUpstream4Map, tcpKey, tcpValue, udpKey, udpValue);
     }
 
     @Test
     @IgnoreUpTo(Build.VERSION_CODES.R)
-    public void testRefreshConntrackTimeout_Upstream4MapUdp() throws Exception {
+    public void testRefreshConntrackTimeout_Downstream4Map() throws Exception {
-        // TODO: Replace the dependencies BPF map with a non-mocked TestBpfMap object.
-        final TestBpfMap<Tether4Key, Tether4Value> bpfUpstream4Map =
-                new TestBpfMap<>(Tether4Key.class, Tether4Value.class);
-        doReturn(bpfUpstream4Map).when(mDeps).getBpfUpstream4Map();
-
-        final Tether4Key key = makeUpstream4Key(IPPROTO_UDP);
-        final Tether4Value value = makeUpstream4Value();
-
-        checkRefreshConntrackTimeout(bpfUpstream4Map, IPPROTO_UDP, key, value);
-    }
-
-    @Test
-    @IgnoreUpTo(Build.VERSION_CODES.R)
-    public void testRefreshConntrackTimeout_Downstream4MapTcp() throws Exception {
         // TODO: Replace the dependencies BPF map with a non-mocked TestBpfMap object.
         final TestBpfMap<Tether4Key, Tether4Value> bpfDownstream4Map =
                 new TestBpfMap<>(Tether4Key.class, Tether4Value.class);
         doReturn(bpfDownstream4Map).when(mDeps).getBpfDownstream4Map();
 
-        final Tether4Key key = makeDownstream4Key(IPPROTO_TCP);
+        final Tether4Key tcpKey = makeDownstream4Key(IPPROTO_TCP);
-        final Tether4Value value = makeDownstream4Value();
+        final Tether4Key udpKey = makeDownstream4Key(IPPROTO_UDP);
+        final Tether4Value tcpValue = makeDownstream4Value();
+        final Tether4Value udpValue = makeDownstream4Value();
 
-        checkRefreshConntrackTimeout(bpfDownstream4Map, IPPROTO_TCP, key, value);
+        checkRefreshConntrackTimeout(bpfDownstream4Map, tcpKey, tcpValue, udpKey, udpValue);
-    }
-
-    @Test
-    @IgnoreUpTo(Build.VERSION_CODES.R)
-    public void testRefreshConntrackTimeout_Downstream4MapUdp() throws Exception {
-        // TODO: Replace the dependencies BPF map with a non-mocked TestBpfMap object.
-        final TestBpfMap<Tether4Key, Tether4Value> bpfDownstream4Map =
-                new TestBpfMap<>(Tether4Key.class, Tether4Value.class);
-        doReturn(bpfDownstream4Map).when(mDeps).getBpfDownstream4Map();
-
-        final Tether4Key key = makeDownstream4Key(IPPROTO_UDP);
-        final Tether4Value value = makeDownstream4Value();
-
-        checkRefreshConntrackTimeout(bpfDownstream4Map, IPPROTO_UDP, key, value);
     }
 }