Revert "[CTT-5] Stop update TCP conntrack entry timeout"
This reverts commit a5c742be69c9a5e786b5fd6af60d84598196ae9a.
Reason for revert:
Stop releasing this commit because it needs more test coverage.
Bug: 190783768
Bug: 192804833
Change-Id: I6e6b40b0f6207ed197d01bfabb616f1afa31fb70
Test: atest TetheringCoverageTests
diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
index f8a9251..8952da9 100644
--- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
+++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
@@ -123,14 +123,9 @@
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;
@@ -1909,23 +1904,6 @@
}
}
- 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();
@@ -1933,7 +1911,7 @@
// 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);
@@ -1941,10 +1919,10 @@
});
// 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);
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 bcdedc7..8332854 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 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;
import static com.android.networkstack.tethering.BpfCoordinator.StatsType.STATS_PER_IFACE;
@@ -1526,27 +1526,23 @@
}
private void checkRefreshConntrackTimeout(final TestBpfMap<Tether4Key, Tether4Value> bpfMap,
- int proto, final Tether4Key key, final Tether4Value value) throws Exception {
- if (proto != IPPROTO_TCP && proto != IPPROTO_UDP) {
- fail("Not support protocol " + proto);
- }
+ final Tether4Key tcpKey, final Tether4Value tcpValue, final Tether4Key udpKey,
+ final Tether4Value udpValue) throws Exception {
// 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
- // ->| |<-
- // +---+---+---+---+---+--...--+---+---+---+---+---+-..-+---+-- ..
- // | 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;
+ // 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;
// Static mocking for NetlinkSocket.
MockitoSession mockSession = ExtendedMockito.mockitoSession()
@@ -1555,27 +1551,30 @@
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.
- setElapsedRealtimeNanos(validTimeNs);
+ // [2] Refresh contrack timeout.
+ setElapsedRealtimeNanos(validTime);
mTestLooper.moveTimeForward(CONNTRACK_TIMEOUT_UPDATE_INTERVAL_MS);
waitForIdle();
-
- 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)));
- }
+ 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)));
ExtendedMockito.verifyNoMoreInteractions(staticMockMarker(NetlinkSocket.class));
ExtendedMockito.clearInvocations(staticMockMarker(NetlinkSocket.class));
@@ -1592,57 +1591,33 @@
@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 Tether4Value value = makeUpstream4Value();
+ final Tether4Key tcpKey = makeUpstream4Key(IPPROTO_TCP);
+ 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 {
- // 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 {
+ public void testRefreshConntrackTimeout_Downstream4Map() 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 Tether4Value value = makeDownstream4Value();
+ final Tether4Key tcpKey = makeDownstream4Key(IPPROTO_TCP);
+ final Tether4Key udpKey = makeDownstream4Key(IPPROTO_UDP);
+ final Tether4Value tcpValue = makeDownstream4Value();
+ final Tether4Value udpValue = makeDownstream4Value();
- 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<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);
+ checkRefreshConntrackTimeout(bpfDownstream4Map, tcpKey, tcpValue, udpKey, udpValue);
}
}