diff --git a/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java b/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java index f27c831689..ce71f454de 100644 --- a/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java +++ b/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java @@ -170,6 +170,12 @@ public class BpfCoordinatorShimImpl return true; } + @Override + public boolean isAnyIpv4RuleOnUpstream(int ifIndex) { + /* no op */ + return false; + } + @Override public String toString() { return "Netd used"; 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 4f7fe65a6a..2bb9fd7e7c 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 @@ -23,6 +23,7 @@ import android.net.util.SharedLog; import android.system.ErrnoException; import android.system.Os; import android.system.OsConstants; +import android.util.Log; import android.util.SparseArray; import androidx.annotation.NonNull; @@ -84,6 +85,20 @@ public class BpfCoordinatorShimImpl @Nullable private final BpfMap mBpfLimitMap; + // Tracking IPv4 rule count while any rule is using the given upstream interfaces. Used for + // reducing the BPF map iteration query. The count is increased or decreased when the rule is + // added or removed successfully on mBpfDownstream4Map. Counting the rules on downstream4 map + // is because tetherOffloadRuleRemove can't get upstream interface index from upstream key, + // unless pass upstream value which is not required for deleting map entry. The upstream + // interface index is the same in Upstream4Value.oif and Downstream4Key.iif. For now, it is + // okay to count on Downstream4Key. See BpfConntrackEventConsumer#accept. + // Note that except the constructor, any calls to mBpfDownstream4Map.clear() need to clear + // this counter as well. + // TODO: Count the rule on upstream if multi-upstream is supported and the + // packet needs to be sent and responded on different upstream interfaces. + // TODO: Add IPv6 rule count. + private final SparseArray mRule4CountOnUpstream = new SparseArray<>(); + public BpfCoordinatorShimImpl(@NonNull final Dependencies deps) { mLog = deps.getSharedLog().forSubComponent(TAG); @@ -324,18 +339,22 @@ public class BpfCoordinatorShimImpl if (!isInitialized()) return false; try { - // The last used time field of the value is updated by the bpf program. Adding the same - // map pair twice causes the unexpected refresh. Must be fixed before starting the - // conntrack timeout extension implementation. - // TODO: consider using insertEntry. if (downstream) { - mBpfDownstream4Map.updateEntry(key, value); + mBpfDownstream4Map.insertEntry(key, value); + + // Increase the rule count while a adding rule is using a given upstream interface. + final int upstreamIfindex = (int) key.iif; + int count = mRule4CountOnUpstream.get(upstreamIfindex, 0 /* default */); + mRule4CountOnUpstream.put(upstreamIfindex, ++count); } else { - mBpfUpstream4Map.updateEntry(key, value); + mBpfUpstream4Map.insertEntry(key, value); } } catch (ErrnoException e) { - mLog.e("Could not update entry: ", e); + mLog.e("Could not insert entry (" + key + ", " + value + "): " + e); return false; + } catch (IllegalStateException e) { + // Silent if the rule already exists. Note that the errno EEXIST was rethrown as + // IllegalStateException. See BpfMap#insertEntry. } return true; } @@ -346,7 +365,26 @@ public class BpfCoordinatorShimImpl try { if (downstream) { - mBpfDownstream4Map.deleteEntry(key); + if (!mBpfDownstream4Map.deleteEntry(key)) { + mLog.e("Could not delete entry (key: " + key + ")"); + return false; + } + + // Decrease the rule count while a deleting rule is not using a given upstream + // interface anymore. + final int upstreamIfindex = (int) key.iif; + Integer count = mRule4CountOnUpstream.get(upstreamIfindex); + if (count == null) { + Log.wtf(TAG, "Could not delete count for interface " + upstreamIfindex); + return false; + } + + if (--count == 0) { + // Remove the entry if the count decreases to zero. + mRule4CountOnUpstream.remove(upstreamIfindex); + } else { + mRule4CountOnUpstream.put(upstreamIfindex, count); + } } else { mBpfUpstream4Map.deleteEntry(key); } @@ -386,6 +424,12 @@ public class BpfCoordinatorShimImpl return true; } + @Override + public boolean isAnyIpv4RuleOnUpstream(int ifIndex) { + // No entry means no rule for the given interface because 0 has never been stored. + return mRule4CountOnUpstream.get(ifIndex) != null; + } + private String mapStatus(BpfMap m, String name) { return name + "{" + (m != null ? "OK" : "ERROR") + "}"; } 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 b7b4c47cc3..ccdf78c228 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 @@ -144,6 +144,11 @@ public abstract class BpfCoordinatorShim { */ public abstract boolean tetherOffloadRuleRemove(boolean downstream, @NonNull Tether4Key key); + /** + * Whether there is currently any IPv4 rule on the specified upstream. + */ + public abstract boolean isAnyIpv4RuleOnUpstream(int ifIndex); + /** * Attach BPF program. * diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java index e5380e008d..da15fa8862 100644 --- a/Tethering/src/android/net/ip/IpServer.java +++ b/Tethering/src/android/net/ip/IpServer.java @@ -742,16 +742,14 @@ public class IpServer extends StateMachine { params.dnses.add(dnsServer); } } - - // Add upstream index to name mapping for the tether stats usage in the coordinator. - // Although this mapping could be added by both class Tethering and IpServer, adding - // mapping from IpServer guarantees that the mapping is added before the adding - // forwarding rules. That is because there are different state machines in both - // classes. It is hard to guarantee the link property update order between multiple - // state machines. - mBpfCoordinator.addUpstreamNameToLookupTable(upstreamIfIndex, upstreamIface); } + // Add upstream index to name mapping. See the comment of the interface mapping update in + // CMD_TETHER_CONNECTION_CHANGED. Adding the mapping update here to the avoid potential + // timing issue. It prevents that the IPv6 capability is updated later than + // CMD_TETHER_CONNECTION_CHANGED. + mBpfCoordinator.addUpstreamNameToLookupTable(upstreamIfIndex, upstreamIface); + // If v6only is null, we pass in null to setRaParams(), which handles // deprecation of any existing RA data. @@ -1335,6 +1333,26 @@ public class IpServer extends StateMachine { mUpstreamIfaceSet = newUpstreamIfaceSet; for (String ifname : added) { + // Add upstream index to name mapping for the tether stats usage in the + // coordinator. Although this mapping could be added by both class + // Tethering and IpServer, adding mapping from IpServer guarantees that + // the mapping is added before adding forwarding rules. That is because + // there are different state machines in both classes. It is hard to + // guarantee the link property update order between multiple state machines. + // Note that both IPv4 and IPv6 interface may be added because + // Tethering::setUpstreamNetwork calls getTetheringInterfaces which merges + // IPv4 and IPv6 interface name (if any) into an InterfaceSet. The IPv6 + // capability may be updated later. In that case, IPv6 interface mapping is + // updated in updateUpstreamIPv6LinkProperties. + if (!ifname.startsWith("v4-")) { // ignore clat interfaces + final InterfaceParams upstreamIfaceParams = + mDeps.getInterfaceParams(ifname); + if (upstreamIfaceParams != null) { + mBpfCoordinator.addUpstreamNameToLookupTable( + upstreamIfaceParams.index, ifname); + } + } + mBpfCoordinator.maybeAttachProgram(mIfaceName, ifname); try { mNetd.tetherAddForward(mIfaceName, ifname); diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index 74eb87b6b1..6f54d106df 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -478,18 +478,7 @@ public class BpfCoordinator { LinkedHashMap rules = mIpv6ForwardingRules.get(ipServer); // When the first rule is added to an upstream, setup upstream forwarding and data limit. - final int upstreamIfindex = rule.upstreamIfindex; - if (!isAnyRuleOnUpstream(upstreamIfindex)) { - // If failed to set a data limit, probably should not use this upstream, because - // the upstream may not want to blow through the data limit that was told to apply. - // TODO: Perhaps stop the coordinator. - boolean success = updateDataLimit(upstreamIfindex); - if (!success) { - final String iface = mInterfaceNames.get(upstreamIfindex); - mLog.e("Setting data limit for " + iface + " failed."); - } - - } + maybeSetLimit(rule.upstreamIfindex); if (!isAnyRuleFromDownstreamToUpstream(rule.downstreamIfindex, rule.upstreamIfindex)) { final int downstream = rule.downstreamIfindex; @@ -544,22 +533,7 @@ public class BpfCoordinator { } // Do cleanup functionality if there is no more rule on the given upstream. - final int upstreamIfindex = rule.upstreamIfindex; - if (!isAnyRuleOnUpstream(upstreamIfindex)) { - final TetherStatsValue statsValue = - mBpfCoordinatorShim.tetherOffloadGetAndClearStats(upstreamIfindex); - if (statsValue == null) { - Log.wtf(TAG, "Fail to cleanup tether stats for upstream index " + upstreamIfindex); - return; - } - - SparseArray tetherStatsList = new SparseArray(); - tetherStatsList.put(upstreamIfindex, statsValue); - - // Update the last stats delta and delete the local cache for a given upstream. - updateQuotaAndStatsFromSnapshot(tetherStatsList); - mStats.remove(upstreamIfindex); - } + maybeClearLimit(rule.upstreamIfindex); } /** @@ -1113,6 +1087,8 @@ public class BpfCoordinator { // Support raw ip only. // TODO: add ether ip support. + // TODO: parse CTA_PROTOINFO of conntrack event in ConntrackMonitor. For TCP, only add rules + // while TCP status is established. private class BpfConntrackEventConsumer implements ConntrackEventConsumer { @NonNull private Tether4Key makeTetherUpstream4Key( @@ -1178,8 +1154,9 @@ public class BpfCoordinator { if (e.msgType == (NetlinkConstants.NFNL_SUBSYS_CTNETLINK << 8 | NetlinkConstants.IPCTNL_MSG_CT_DELETE)) { - mBpfCoordinatorShim.tetherOffloadRuleRemove(false, upstream4Key); - mBpfCoordinatorShim.tetherOffloadRuleRemove(true, downstream4Key); + mBpfCoordinatorShim.tetherOffloadRuleRemove(UPSTREAM, upstream4Key); + mBpfCoordinatorShim.tetherOffloadRuleRemove(DOWNSTREAM, downstream4Key); + maybeClearLimit(upstreamIndex); return; } @@ -1187,8 +1164,9 @@ public class BpfCoordinator { final Tether4Value downstream4Value = makeTetherDownstream4Value(e, tetherClient, upstreamIndex); - mBpfCoordinatorShim.tetherOffloadRuleAdd(false, upstream4Key, upstream4Value); - mBpfCoordinatorShim.tetherOffloadRuleAdd(true, downstream4Key, downstream4Value); + maybeSetLimit(upstreamIndex); + mBpfCoordinatorShim.tetherOffloadRuleAdd(UPSTREAM, upstream4Key, upstream4Value); + mBpfCoordinatorShim.tetherOffloadRuleAdd(DOWNSTREAM, downstream4Key, downstream4Value); } } @@ -1250,6 +1228,47 @@ public class BpfCoordinator { return sendDataLimitToBpfMap(ifIndex, quotaBytes); } + private void maybeSetLimit(int upstreamIfindex) { + if (isAnyRuleOnUpstream(upstreamIfindex) + || mBpfCoordinatorShim.isAnyIpv4RuleOnUpstream(upstreamIfindex)) { + return; + } + + // If failed to set a data limit, probably should not use this upstream, because + // the upstream may not want to blow through the data limit that was told to apply. + // TODO: Perhaps stop the coordinator. + boolean success = updateDataLimit(upstreamIfindex); + if (!success) { + final String iface = mInterfaceNames.get(upstreamIfindex); + mLog.e("Setting data limit for " + iface + " failed."); + } + } + + // TODO: This should be also called while IpServer wants to clear all IPv4 rules. Relying on + // conntrack event can't cover this case. + private void maybeClearLimit(int upstreamIfindex) { + if (isAnyRuleOnUpstream(upstreamIfindex) + || mBpfCoordinatorShim.isAnyIpv4RuleOnUpstream(upstreamIfindex)) { + return; + } + + final TetherStatsValue statsValue = + mBpfCoordinatorShim.tetherOffloadGetAndClearStats(upstreamIfindex); + if (statsValue == null) { + Log.wtf(TAG, "Fail to cleanup tether stats for upstream index " + upstreamIfindex); + return; + } + + SparseArray tetherStatsList = new SparseArray(); + tetherStatsList.put(upstreamIfindex, statsValue); + + // Update the last stats delta and delete the local cache for a given upstream. + updateQuotaAndStatsFromSnapshot(tetherStatsList); + mStats.remove(upstreamIfindex); + } + + // TODO: Rename to isAnyIpv6RuleOnUpstream and define an isAnyRuleOnUpstream method that called + // both isAnyIpv6RuleOnUpstream and mBpfCoordinatorShim.isAnyIpv4RuleOnUpstream. private boolean isAnyRuleOnUpstream(int upstreamIfindex) { for (LinkedHashMap rules : mIpv6ForwardingRules .values()) { diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index adf1f671ca..1380bbf3c0 100644 --- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -474,6 +474,8 @@ public class IpServerTest { InOrder inOrder = inOrder(mNetd, mBpfCoordinator); // Add the forwarding pair . + inOrder.verify(mBpfCoordinator).addUpstreamNameToLookupTable(UPSTREAM_IFINDEX, + UPSTREAM_IFACE); inOrder.verify(mBpfCoordinator).maybeAttachProgram(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).ipfwdAddInterfaceForward(IFACE_NAME, UPSTREAM_IFACE); @@ -494,6 +496,8 @@ public class IpServerTest { inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE); // Add the forwarding pair . + inOrder.verify(mBpfCoordinator).addUpstreamNameToLookupTable(UPSTREAM_IFINDEX2, + UPSTREAM_IFACE2); inOrder.verify(mBpfCoordinator).maybeAttachProgram(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).ipfwdAddInterfaceForward(IFACE_NAME, UPSTREAM_IFACE2); @@ -517,6 +521,8 @@ public class IpServerTest { // Add the forwarding pair and expect that failed on // tetherAddForward. + inOrder.verify(mBpfCoordinator).addUpstreamNameToLookupTable(UPSTREAM_IFINDEX2, + UPSTREAM_IFACE2); inOrder.verify(mBpfCoordinator).maybeAttachProgram(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE2); @@ -543,6 +549,8 @@ public class IpServerTest { // Add the forwarding pair and expect that failed on // ipfwdAddInterfaceForward. + inOrder.verify(mBpfCoordinator).addUpstreamNameToLookupTable(UPSTREAM_IFINDEX2, + UPSTREAM_IFACE2); inOrder.verify(mBpfCoordinator).maybeAttachProgram(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).ipfwdAddInterfaceForward(IFACE_NAME, UPSTREAM_IFACE2); diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java index 0611086448..1e23da13c3 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -332,13 +332,14 @@ public class TetheringTest { assertTrue("Non-mocked interface " + ifName, ifName.equals(TEST_USB_IFNAME) || ifName.equals(TEST_WLAN_IFNAME) + || ifName.equals(TEST_WIFI_IFNAME) || ifName.equals(TEST_MOBILE_IFNAME) || ifName.equals(TEST_P2P_IFNAME) || ifName.equals(TEST_NCM_IFNAME) || ifName.equals(TEST_ETH_IFNAME)); final String[] ifaces = new String[] { - TEST_USB_IFNAME, TEST_WLAN_IFNAME, TEST_MOBILE_IFNAME, TEST_P2P_IFNAME, - TEST_NCM_IFNAME, TEST_ETH_IFNAME}; + TEST_USB_IFNAME, TEST_WLAN_IFNAME, TEST_WIFI_IFNAME, TEST_MOBILE_IFNAME, + TEST_P2P_IFNAME, TEST_NCM_IFNAME, TEST_ETH_IFNAME}; return new InterfaceParams(ifName, ArrayUtils.indexOf(ifaces, ifName) + IFINDEX_OFFSET, MacAddress.ALL_ZEROS_ADDRESS); }