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),