diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java index eadba58c22..a85141060c 100644 --- a/Tethering/src/android/net/ip/IpServer.java +++ b/Tethering/src/android/net/ip/IpServer.java @@ -252,7 +252,6 @@ public class IpServer extends StateMachine { private final int mInterfaceType; private final LinkProperties mLinkProperties; private final boolean mUsingLegacyDhcp; - private final boolean mUsingBpfOffload; private final int mP2pLeasesSubnetPrefixLength; private final Dependencies mDeps; @@ -314,7 +313,6 @@ public class IpServer extends StateMachine { mInterfaceType = interfaceType; mLinkProperties = new LinkProperties(); mUsingLegacyDhcp = config.useLegacyDhcpServer(); - mUsingBpfOffload = config.isBpfOffloadEnabled(); mP2pLeasesSubnetPrefixLength = config.getP2pLeasesSubnetPrefixLength(); mPrivateAddressCoordinator = addressCoordinator; mDeps = deps; @@ -326,11 +324,9 @@ public class IpServer extends StateMachine { mIpNeighborMonitor = mDeps.getIpNeighborMonitor(getHandler(), mLog, new MyNeighborEventConsumer()); - // IP neighbor monitor monitors the neighbor events for adding/removing offload - // forwarding rules per client. If BPF offload is not supported, don't start listening - // for neighbor events. See updateIpv6ForwardingRules, addIpv6DownstreamRule, - // removeIpv6DownstreamRule. - if (mUsingBpfOffload && !mIpNeighborMonitor.start()) { + // IP neighbor monitor monitors the neighbor events for adding/removing IPv6 downstream rule + // per client. If BPF offload is not supported, don't start listening for neighbor events. + if (mBpfCoordinator.isUsingBpfOffload() && !mIpNeighborMonitor.start()) { mLog.e("Failed to create IpNeighborMonitor on " + mIfaceName); } @@ -891,37 +887,6 @@ public class IpServer extends StateMachine { } } - private void addIpv6DownstreamRule(Ipv6DownstreamRule rule) { - // Theoretically, we don't need this check because IP neighbor monitor doesn't start if BPF - // offload is disabled. Add this check just in case. - // TODO: Perhaps remove this protection check. - if (!mUsingBpfOffload) return; - - mBpfCoordinator.addIpv6DownstreamRule(this, rule); - } - - private void removeIpv6DownstreamRule(Ipv6DownstreamRule rule) { - // TODO: Perhaps remove this protection check. - // See the related comment in #addIpv6DownstreamRule. - if (!mUsingBpfOffload) return; - - mBpfCoordinator.removeIpv6DownstreamRule(this, rule); - } - - private void clearIpv6ForwardingRules() { - if (!mUsingBpfOffload) return; - - mBpfCoordinator.tetherOffloadRuleClear(this); - } - - private void updateIpv6ForwardingRule(int newIfindex) { - // TODO: Perhaps remove this protection check. - // See the related comment in #addIpv6DownstreamRule. - if (!mUsingBpfOffload) return; - - mBpfCoordinator.tetherOffloadRuleUpdate(this, newIfindex); - } - // Handles all updates to IPv6 forwarding rules. These can currently change only if the upstream // changes or if a neighbor event is received. private void updateIpv6ForwardingRules(int prevUpstreamIfindex, int upstreamIfindex, @@ -930,14 +895,14 @@ public class IpServer extends StateMachine { // nothing else. // TODO: Rather than always clear rules, ensure whether ipv6 ever enable first. if (upstreamIfindex == 0 || !upstreamSupportsBpf) { - clearIpv6ForwardingRules(); + mBpfCoordinator.tetherOffloadRuleClear(this); return; } // If the upstream interface has changed, remove all rules and re-add them with the new // upstream interface. if (prevUpstreamIfindex != upstreamIfindex) { - updateIpv6ForwardingRule(upstreamIfindex); + mBpfCoordinator.tetherOffloadRuleUpdate(this, upstreamIfindex); } // If we're here to process a NeighborEvent, do so now. @@ -955,18 +920,14 @@ public class IpServer extends StateMachine { Ipv6DownstreamRule rule = new Ipv6DownstreamRule(upstreamIfindex, mInterfaceParams.index, (Inet6Address) e.ip, mInterfaceParams.macAddr, dstMac); if (e.isValid()) { - addIpv6DownstreamRule(rule); + mBpfCoordinator.addIpv6DownstreamRule(this, rule); } else { - removeIpv6DownstreamRule(rule); + mBpfCoordinator.removeIpv6DownstreamRule(this, rule); } } // TODO: consider moving into BpfCoordinator. private void updateClientInfoIpv4(NeighborEvent e) { - // TODO: Perhaps remove this protection check. - // See the related comment in #addIpv6DownstreamRule. - if (!mUsingBpfOffload) return; - if (e == null) return; if (!(e.ip instanceof Inet4Address) || e.ip.isMulticastAddress() || e.ip.isLoopbackAddress() || e.ip.isLinkLocalAddress()) { @@ -1342,7 +1303,7 @@ public class IpServer extends StateMachine { for (String ifname : mUpstreamIfaceSet.ifnames) cleanupUpstreamInterface(ifname); mUpstreamIfaceSet = null; - clearIpv6ForwardingRules(); + mBpfCoordinator.tetherOffloadRuleClear(IpServer.this); } private void cleanupUpstreamInterface(String upstreamIface) { diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index 7311125dcb..1b23a6c7f0 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -523,6 +523,18 @@ public class BpfCoordinator { mLog.i("Polling stopped"); } + /** + * Return whether BPF offload is supported + */ + public boolean isUsingBpfOffload() { + return isUsingBpf(); + } + + // This is identical to isUsingBpfOffload above but is only used internally. + // The reason for having two separate methods is that the code calls isUsingBpf + // very often. But the tests call verifyNoMoreInteractions, which will check all + // calls to public methods. If isUsingBpf were public, the test would need to + // verify all calls to it, which would clutter the test. private boolean isUsingBpf() { return mIsBpfEnabled && mBpfCoordinatorShim.isInitialized(); } diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index 19d70c614a..c0718d1b3e 100644 --- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -244,6 +244,8 @@ public class IpServerTest { when(mTetherConfig.isBpfOffloadEnabled()).thenReturn(usingBpfOffload); when(mTetherConfig.useLegacyDhcpServer()).thenReturn(usingLegacyDhcp); when(mTetherConfig.getP2pLeasesSubnetPrefixLength()).thenReturn(P2P_SUBNET_PREFIX_LENGTH); + // Recreate mBpfCoordinator again here because mTetherConfig has changed + mBpfCoordinator = spy(new BpfCoordinator(mBpfDeps)); mIpServer = new IpServer( IFACE_NAME, mLooper.getLooper(), interfaceType, mSharedLog, mNetd, mBpfCoordinator, mCallback, mTetherConfig, mAddressCoordinator, mTetheringMetrics, mDependencies); @@ -1244,13 +1246,11 @@ public class IpServerTest { resetNetdBpfMapAndCoordinator(); recvNewNeigh(myIfindex, neigh, NUD_REACHABLE, macA); - verify(mBpfCoordinator, never()).addIpv6DownstreamRule(any(), any()); verifyNeverTetherOffloadRuleAdd(); verifyNoUpstreamIpv6ForwardingChange(null); resetNetdBpfMapAndCoordinator(); recvDelNeigh(myIfindex, neigh, NUD_STALE, macA); - verify(mBpfCoordinator, never()).removeIpv6DownstreamRule(any(), any()); verifyNeverTetherOffloadRuleRemove(); verifyNoUpstreamIpv6ForwardingChange(null); resetNetdBpfMapAndCoordinator();