[CTT-6] Update TCP conntrack entry timeout while adding 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.
Issue: the timeout is set to unacknowledged 300s (countdwon to 298s)
$ adb shell cat /proc/net/nf_conntrack
ipv4 2 tcp 6 298 ESTABLISHED src=192.168.244.128
dst=140.112.8.116 sport=45694 dport=443 ..
Test: atest TetheringCoverageTests
Manual check:
$ adb shell cat /proc/net/nf_conntrack
ipv4 2 tcp 6 431988 ESTABLISHED src=192.168.40.162
dst=140.112.8.116 sport=40774 dport=443 ..
Bug: 190783768
Bug: 192804833
Change-Id: I8c34e85e26c9d976e5e2b85473db75ff46d8abd4
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 611c828..f537e90 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,6 +371,7 @@
} 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 08ab9ca..3c2ce0f 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,6 +141,11 @@
/**
* 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 f8a9251..97ef380 100644
--- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
+++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
@@ -130,8 +130,14 @@
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;
@@ -1566,6 +1572,18 @@
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(
@@ -1592,11 +1610,41 @@
final Tether4Value upstream4Value = makeTetherUpstream4Value(e, upstreamIndex);
final Tether4Value downstream4Value = makeTetherDownstream4Value(e, tetherClient,
upstreamIndex);
-
maybeAddDevMap(upstreamIndex, tetherClient.downstreamIfindex);
maybeSetLimit(upstreamIndex);
- mBpfCoordinatorShim.tetherOffloadRuleAdd(UPSTREAM, upstream4Key, upstream4Value);
- mBpfCoordinatorShim.tetherOffloadRuleAdd(DOWNSTREAM, downstream4Key, downstream4Value);
+
+ 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);
+ }
}
}
@@ -1879,6 +1927,7 @@
// - 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 bcdedc7..32d7e5f 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java
@@ -42,6 +42,7 @@
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;
@@ -1369,8 +1370,14 @@
}
final int status = (msgType == IPCTNL_MSG_CT_NEW) ? ESTABLISHED_MASK : DYING_MASK;
- final int timeoutSec = (msgType == IPCTNL_MSG_CT_NEW) ? 100 /* nonzero, new */
- : 0 /* unused, delete */;
+ 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 */
+ }
return new ConntrackEvent(
(short) (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8 | msgType),
new Tuple(new TupleIpv4(PRIVATE_ADDR, REMOTE_ADDR),