Set the limit whenever any IPv4 or IPv6 rule exists.
Currently, BpfCoordinator only sets the data limit on a given
upstream whenever the first IPv6 rule is created on that
upstream, and clears it whenever the last rule is deleted on that
upstream. It never does this when adding or removing IPv4 rules.
This makes it impossible to offload traffic on IPv4-only
networks.
Fix this by setting the limit when IPv4 rules are created or
deleted as well.
Test: atest TetheringCoverageTests
Manual tests as the follows
Test {add, clear} limit with IPv6-only network [OK]
Test {add} limit with IPv4-only upstream [OK]
TODO:
Test {clear} limit with IPv4-only network. blocked by aosp/1579873
because the IPv4 rules have never deleted.
Change-Id: I5a29bdd18e564318759f617023163e23fb5a3ed0
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 f27c831..ce71f45 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
@@ -171,6 +171,12 @@
}
@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 4f7fe65..2bb9fd7 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.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 @@
@Nullable
private final BpfMap<TetherLimitKey, TetherLimitValue> 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<Integer> mRule4CountOnUpstream = new SparseArray<>();
+
public BpfCoordinatorShimImpl(@NonNull final Dependencies deps) {
mLog = deps.getSharedLog().forSubComponent(TAG);
@@ -324,18 +339,22 @@
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 @@
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 @@
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 b7b4c47..ccdf78c 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
@@ -145,6 +145,11 @@
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.
*
* TODO: consider using InterfaceParams to replace interface name.
diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java
index e5380e0..da15fa8 100644
--- a/Tethering/src/android/net/ip/IpServer.java
+++ b/Tethering/src/android/net/ip/IpServer.java
@@ -742,16 +742,14 @@
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 @@
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 74eb87b..6f54d10 100644
--- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
+++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
@@ -478,18 +478,7 @@
LinkedHashMap<Inet6Address, Ipv6ForwardingRule> 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 @@
}
// 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<TetherStatsValue> tetherStatsList = new SparseArray<TetherStatsValue>();
- 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 @@
// 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 @@
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 @@
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 @@
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<TetherStatsValue> tetherStatsList = new SparseArray<TetherStatsValue>();
+ 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<Inet6Address, Ipv6ForwardingRule> 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 adf1f67..1380bbf 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 @@
InOrder inOrder = inOrder(mNetd, mBpfCoordinator);
// Add the forwarding pair <IFACE_NAME, UPSTREAM_IFACE>.
+ 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 @@
inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE);
// Add the forwarding pair <IFACE_NAME, UPSTREAM_IFACE2>.
+ 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 @@
// Add the forwarding pair <IFACE_NAME, UPSTREAM_IFACE2> 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 @@
// Add the forwarding pair <IFACE_NAME, UPSTREAM_IFACE2> 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 0611086..1e23da1 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 @@
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);
}