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 f537e90177..611c828036 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 @@ 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 3c2ce0f607..08ab9caa97 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 @@ 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 f4a691612e..01be97a406 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -124,21 +124,10 @@ public class BpfCoordinator { 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; - - // 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; @@ -1578,34 +1567,8 @@ public class BpfCoordinator { final Tether4Key downstream4Key = makeTetherDownstream4Key(e, tetherClient, upstreamIndex); - final boolean isConntrackEventDelete = - e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8 - | NetlinkConstants.IPCTNL_MSG_CT_DELETE); - - // 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); - - final boolean isTcpNonEstablished = - 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); - - // Delete the BPF rules: - // 1. Contrack event IPCTNL_MSG_CT_DELETE received. - // 2. For TCP conntrack entry, the tcp state has left "established" and going to be - // closed. - // TODO: continue to offload half-closed tcp connections. - if (isConntrackEventDelete || isTcpNonEstablished) { + if (e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8 + | NetlinkConstants.IPCTNL_MSG_CT_DELETE)) { final boolean deletedUpstream = mBpfCoordinatorShim.tetherOffloadRuleRemove( UPSTREAM, upstream4Key); final boolean deletedDownstream = mBpfCoordinatorShim.tetherOffloadRuleRemove( @@ -1620,7 +1583,6 @@ public class BpfCoordinator { Log.wtf(TAG, "The bidirectional rules should be removed concurrently (" + "upstream: " + deletedUpstream + ", downstream: " + deletedDownstream + ")"); - // TODO: consider better error handling for the stubs {rule, limit, ..}. return; } @@ -1631,41 +1593,11 @@ public class BpfCoordinator { 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); } } @@ -1948,7 +1880,6 @@ 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; @@ -1979,23 +1910,6 @@ public class BpfCoordinator { } } - 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(); @@ -2003,7 +1917,7 @@ public class BpfCoordinator { // 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); @@ -2011,10 +1925,10 @@ public class BpfCoordinator { }); // 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 ab542d69dd..7e9e34fc6c 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,6 @@ import static android.system.OsConstants.NETLINK_NETFILTER; 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; @@ -1379,14 +1378,8 @@ public class BpfCoordinatorTest { } 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), @@ -1542,27 +1535,23 @@ public class BpfCoordinatorTest { } private void checkRefreshConntrackTimeout(final TestBpfMap 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() @@ -1571,27 +1560,30 @@ public class BpfCoordinatorTest { 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)); @@ -1608,57 +1600,33 @@ public class BpfCoordinatorTest { @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 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 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 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 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); } }