From 299a81157c257a6b6bc9d41aef094fdfe8aadde7 Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Fri, 9 Jul 2021 13:15:37 +0800 Subject: [PATCH] [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 --- .../apishim/api31/BpfCoordinatorShimImpl.java | 1 + .../apishim/common/BpfCoordinatorShim.java | 5 ++ .../tethering/BpfCoordinator.java | 55 ++++++++++++++++++- .../tethering/BpfCoordinatorTest.java | 11 +++- 4 files changed, 67 insertions(+), 5 deletions(-) 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 611c828036..f537e90177 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 @@ 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; } 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 08ab9caa97..3c2ce0f607 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 @@ 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); diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index f8a9251099..97ef380670 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -130,8 +130,14 @@ public class BpfCoordinator { 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 @@ public class BpfCoordinator { 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 @@ 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); - 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 @@ 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; 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 bcdedc79aa..32d7e5f7a6 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.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; @@ -1369,8 +1370,14 @@ public class BpfCoordinatorTest { } 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),