From 118b5b578a15dbb82146ae5d14d67a2cc79340cd Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Mon, 5 Jul 2021 15:55:23 +0800 Subject: [PATCH] [CTT-7] Delete the rules while half-closing tcp state entered This is fixing post-FIN state (by disabling offload post-FIN). Once the tcp state enters half-closing tcp state (fin wait, close wait), delete the offload rules. From this commit, we have done the short term solution for conntrack tcp timeout issue. Here is what we have done so far. - Stop updating tcp timeout to avoid updating wrong tcp state timeout. - Update the ESTABLISHED timeout nf_conntrack_tcp_timeout_established (432000) after adding bidirectional rules. - Delete the tcp rules when the tcp state has left "established". Here is the long term solution and need to be addressed in follow up commits. - Parse the tcp state from netlink conntrack event. - Build a mapping to trace the tcp state of the tcp conntrack event. - Update tcp state timeout for {ESTABLISHED (432000), FIN_WAIT (120), CLOSE_WAIT (60)}. Bug: 190783768 Bug: 192804833 Test: atest TetheringCoverageTests Manual test: 1. Browse on tethered device 2. Check conntrack tcp state is established. $ cat /proc/net/nf_conntrack ipv4 2 tcp 6 431995 ESTABLISHED src=192.168.207.9 dst=140.112.8.116 sport=50697 dport=443 .. 3. Check offload rules are added via dumpsys. Upstream: tcp .. 192.168.207.9:50697 -> 14(rmnet0) 10.224.1.247:50697 -> 140.112.8.116:443 .. Downstream: tcp .. 140.112.8.116:443 -> 30(30) 10.224.1.247:50697 -> 192.168.207.9:50697 .. 4. Stop browsing for a few seconds. 5. Check conntrack tcp state is half-closed. $ cat /proc/net/nf_conntrack ipv4 2 tcp 6 116 TIME_WAIT src=192.168.207.9 dst=140.112.8.116 sport=50697 dport=443 .. 5. Check offload rules are removed via dumpsys. Upstream: (not found) Downstream: (not found) Change-Id: I07e27230bf8952acd7828d1f605167758b3bc490 --- .../tethering/BpfCoordinator.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index 97ef380670..c99007e9ad 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -1572,6 +1572,10 @@ 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 @@ -1584,8 +1588,18 @@ public class BpfCoordinator { && 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 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) { final boolean deletedUpstream = mBpfCoordinatorShim.tetherOffloadRuleRemove( UPSTREAM, upstream4Key); final boolean deletedDownstream = mBpfCoordinatorShim.tetherOffloadRuleRemove( @@ -1600,6 +1614,7 @@ 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; }