Revert "[CTT-6] Update TCP conntrack entry timeout while adding rules"
This reverts commit 299a81157c257a6b6bc9d41aef094fdfe8aadde7.
Reason for revert:
Stop releasing this commit because it needs more test coverage.
Bug: 190783768
Bug: 192804833
Change-Id: I6a0d93e04814ae73e1ec7d6fd4df19e1d2787207
Test: atest TetheringCoverageTests
diff --git a/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java b/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java
index f537e90..611c828 100644
--- a/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java
+++ b/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java
@@ -371,7 +371,6 @@
} 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;
}
diff --git a/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java b/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java
index 3c2ce0f..08ab9ca 100644
--- a/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java
+++ b/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java
@@ -141,11 +141,6 @@
/**
* 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);
diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
index 97ef380..f8a9251 100644
--- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
+++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
@@ -130,14 +130,8 @@
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;
@@ -1572,18 +1566,6 @@
final Tether4Key downstream4Key = makeTetherDownstream4Key(e, tetherClient,
upstreamIndex);
- // 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);
-
if (e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8
| NetlinkConstants.IPCTNL_MSG_CT_DELETE)) {
final boolean deletedUpstream = mBpfCoordinatorShim.tetherOffloadRuleRemove(
@@ -1610,41 +1592,11 @@
final Tether4Value upstream4Value = makeTetherUpstream4Value(e, upstreamIndex);
final Tether4Value downstream4Value = makeTetherDownstream4Value(e, tetherClient,
upstreamIndex);
+
maybeAddDevMap(upstreamIndex, tetherClient.downstreamIfindex);
maybeSetLimit(upstreamIndex);
-
- final boolean upstreamAdded = mBpfCoordinatorShim.tetherOffloadRuleAdd(UPSTREAM,
- 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);
- }
+ mBpfCoordinatorShim.tetherOffloadRuleAdd(UPSTREAM, upstream4Key, upstream4Value);
+ mBpfCoordinatorShim.tetherOffloadRuleAdd(DOWNSTREAM, downstream4Key, downstream4Value);
}
}
@@ -1927,7 +1879,6 @@
// - 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;
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 32d7e5f..bcdedc7 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java
@@ -42,7 +42,6 @@
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;
@@ -1370,14 +1369,8 @@
}
final int status = (msgType == IPCTNL_MSG_CT_NEW) ? ESTABLISHED_MASK : DYING_MASK;
- int timeoutSec;
- if (msgType == IPCTNL_MSG_CT_NEW) {
- timeoutSec = (proto == IPPROTO_TCP)
- ? NF_CONNTRACK_TCP_TIMEOUT_ESTABLISHED
- : NF_CONNTRACK_UDP_TIMEOUT_STREAM;
- } else {
- timeoutSec = 0; /* unused, CT_DELETE */
- }
+ final int timeoutSec = (msgType == IPCTNL_MSG_CT_NEW) ? 100 /* nonzero, new */
+ : 0 /* unused, delete */;
return new ConntrackEvent(
(short) (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8 | msgType),
new Tuple(new TupleIpv4(PRIVATE_ADDR, REMOTE_ADDR),