diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index 8952da9484..f8a9251099 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -123,9 +123,14 @@ 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; + @VisibleForTesting static final int NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED = 432_000; @VisibleForTesting static final int NF_CONNTRACK_UDP_TIMEOUT_STREAM = 180; @@ -1904,6 +1909,23 @@ 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(); @@ -1911,7 +1933,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 ((now - v.lastUsed) / 1_000_000 < CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS) { + if (requireConntrackTimeoutUpdate(now, v.lastUsed, k.l4proto)) { updateConntrackTimeout((byte) k.l4proto, parseIPv4Address(k.src4), (short) k.srcPort, parseIPv4Address(k.dst4), (short) k.dstPort); @@ -1919,10 +1941,10 @@ public class BpfCoordinator { }); // Reverse the source and destination {address, port} from downstream value because - // #updateConntrackTimeout refresh the timeout of netlink attribute CTA_TUPLE_ORIG + // #updateConntrackTimeout refreshes the timeout of netlink attribute CTA_TUPLE_ORIG // which is opposite direction for downstream map value. mBpfCoordinatorShim.tetherOffloadRuleForEach(DOWNSTREAM, (k, v) -> { - if ((now - v.lastUsed) / 1_000_000 < CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS) { + if (requireConntrackTimeoutUpdate(now, v.lastUsed, k.l4proto)) { updateConntrackTimeout((byte) k.l4proto, parseIPv4Address(v.dst46), (short) v.dstPort, parseIPv4Address(v.src46), (short) v.srcPort); diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java index 8332854f7e..bcdedc79aa 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java @@ -41,7 +41,7 @@ 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.NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED; +import static com.android.networkstack.tethering.BpfCoordinator.CONNTRACK_TIMEOUT_UPDATE_SLACK_MS; import static com.android.networkstack.tethering.BpfCoordinator.NF_CONNTRACK_UDP_TIMEOUT_STREAM; import static com.android.networkstack.tethering.BpfCoordinator.StatsType; import static com.android.networkstack.tethering.BpfCoordinator.StatsType.STATS_PER_IFACE; @@ -1526,23 +1526,27 @@ public class BpfCoordinatorTest { } private void checkRefreshConntrackTimeout(final TestBpfMap bpfMap, - final Tether4Key tcpKey, final Tether4Value tcpValue, final Tether4Key udpKey, - final Tether4Value udpValue) throws Exception { + int proto, final Tether4Key key, final Tether4Value value) throws Exception { + if (proto != IPPROTO_TCP && proto != IPPROTO_UDP) { + 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: - // 0 60 (seconds) - // +---+---+---+---+--...--+---+---+---+---+---+- .. - // | CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS | - // +---+---+---+---+--...--+---+---+---+---+---+- .. - // |<- valid diff ->| - // |<- expired diff ->| - // ^ ^ ^ - // last used time elapsed time (valid) elapsed time (expired) - final long validTime = (CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS - 1) * 1_000_000L; - final long expiredTime = (CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS + 1) * 1_000_000L; + // CONNTRACK_TIMEOUT_UPDATE_SLACK_MS + // ->| |<- + // +---+---+---+---+---+--...--+---+---+---+---+---+-..-+---+-- .. + // | CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS | + // +---+---+---+---+---+--...--+---+---+---+---+---+-..-+---+-- .. + // |<- valid diff ->| + // |<- expired diff ->| + // ^ ^ ^ + // last used time elapsed time (valid) elapsed time (expired) + final long validTimeNs = CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS * 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() @@ -1551,30 +1555,27 @@ public class BpfCoordinatorTest { try { final BpfCoordinator coordinator = makeBpfCoordinator(); coordinator.startPolling(); - bpfMap.insertEntry(tcpKey, tcpValue); - bpfMap.insertEntry(udpKey, udpValue); + bpfMap.insertEntry(key, value); // [1] Don't refresh contrack timeout. - setElapsedRealtimeNanos(expiredTime); + setElapsedRealtimeNanos(expiredTimeNs); mTestLooper.moveTimeForward(CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS); waitForIdle(); ExtendedMockito.verifyNoMoreInteractions(staticMockMarker(NetlinkSocket.class)); ExtendedMockito.clearInvocations(staticMockMarker(NetlinkSocket.class)); - // [2] Refresh contrack timeout. - setElapsedRealtimeNanos(validTime); + // [2] Refresh contrack timeout. UDP Only. + setElapsedRealtimeNanos(validTimeNs); mTestLooper.moveTimeForward(CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS); waitForIdle(); - final byte[] expectedNetlinkTcp = ConntrackMessage.newIPv4TimeoutUpdateRequest( - IPPROTO_TCP, PRIVATE_ADDR, (int) PRIVATE_PORT, REMOTE_ADDR, - (int) REMOTE_PORT, NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED); - final byte[] expectedNetlinkUdp = ConntrackMessage.newIPv4TimeoutUpdateRequest( - IPPROTO_UDP, PRIVATE_ADDR, (int) PRIVATE_PORT, REMOTE_ADDR, - (int) REMOTE_PORT, NF_CONNTRACK_UDP_TIMEOUT_STREAM); - ExtendedMockito.verify(() -> NetlinkSocket.sendOneShotKernelMessage( - eq(NETLINK_NETFILTER), eq(expectedNetlinkTcp))); - ExtendedMockito.verify(() -> NetlinkSocket.sendOneShotKernelMessage( - eq(NETLINK_NETFILTER), eq(expectedNetlinkUdp))); + + if (proto == IPPROTO_UDP) { + final byte[] expectedNetlinkUdp = ConntrackMessage.newIPv4TimeoutUpdateRequest( + IPPROTO_UDP, PRIVATE_ADDR, (int) PRIVATE_PORT, REMOTE_ADDR, + (int) REMOTE_PORT, NF_CONNTRACK_UDP_TIMEOUT_STREAM); + ExtendedMockito.verify(() -> NetlinkSocket.sendOneShotKernelMessage( + eq(NETLINK_NETFILTER), eq(expectedNetlinkUdp))); + } ExtendedMockito.verifyNoMoreInteractions(staticMockMarker(NetlinkSocket.class)); ExtendedMockito.clearInvocations(staticMockMarker(NetlinkSocket.class)); @@ -1591,33 +1592,57 @@ public class BpfCoordinatorTest { @Test @IgnoreUpTo(Build.VERSION_CODES.R) - public void testRefreshConntrackTimeout_Upstream4Map() throws Exception { + public void testRefreshConntrackTimeout_Upstream4MapTcp() throws Exception { // TODO: Replace the dependencies BPF map with a non-mocked TestBpfMap object. final TestBpfMap bpfUpstream4Map = new TestBpfMap<>(Tether4Key.class, Tether4Value.class); doReturn(bpfUpstream4Map).when(mDeps).getBpfUpstream4Map(); - final Tether4Key tcpKey = makeUpstream4Key(IPPROTO_TCP); - final Tether4Key udpKey = makeUpstream4Key(IPPROTO_UDP); - final Tether4Value tcpValue = makeUpstream4Value(); - final Tether4Value udpValue = makeUpstream4Value(); + final Tether4Key key = makeUpstream4Key(IPPROTO_TCP); + final Tether4Value value = makeUpstream4Value(); - checkRefreshConntrackTimeout(bpfUpstream4Map, tcpKey, tcpValue, udpKey, udpValue); + checkRefreshConntrackTimeout(bpfUpstream4Map, IPPROTO_TCP, key, value); } @Test @IgnoreUpTo(Build.VERSION_CODES.R) - public void testRefreshConntrackTimeout_Downstream4Map() throws Exception { + public void testRefreshConntrackTimeout_Upstream4MapUdp() throws Exception { + // TODO: Replace the dependencies BPF map with a non-mocked TestBpfMap object. + final TestBpfMap 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 bpfDownstream4Map = new TestBpfMap<>(Tether4Key.class, Tether4Value.class); doReturn(bpfDownstream4Map).when(mDeps).getBpfDownstream4Map(); - final Tether4Key tcpKey = makeDownstream4Key(IPPROTO_TCP); - final Tether4Key udpKey = makeDownstream4Key(IPPROTO_UDP); - final Tether4Value tcpValue = makeDownstream4Value(); - final Tether4Value udpValue = makeDownstream4Value(); + final Tether4Key key = makeDownstream4Key(IPPROTO_TCP); + final Tether4Value value = makeDownstream4Value(); - checkRefreshConntrackTimeout(bpfDownstream4Map, tcpKey, tcpValue, udpKey, udpValue); + checkRefreshConntrackTimeout(bpfDownstream4Map, IPPROTO_TCP, key, value); + } + + @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 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); } }