From e4d549ab119d4a2c585010a43d37fd158bcdcde4 Mon Sep 17 00:00:00 2001 From: KH Shi Date: Mon, 28 Aug 2023 09:57:34 +0000 Subject: [PATCH] Remove duplicated bpf offload support check in IpServer In current code, IpServer will ensure bpf offload is supported before calling BpfCoordinator's APIs. But those APIs already have the same checks inside the function. This CL removed duplicated checks in IpServer. After this change, the BPF offload support status is only determined inside BpfCoordinator and it won't change during the life cycle of Tethering. Tethering initializes mBpfCoordinator just once in the constructor. After that point, the value of isUsingBpf can never change, because mIsBpfEnabled is final and mBpfCoordinatorShim.isInitialized() is either always true (api30) or can either never change (api31). Also fixed a bug in IpServerTest upon this change. Test: atest TetheringTests Bug: 261923493 Change-Id: I50f231a83bf678b90eae4fd9bfa6035e86978bb3 --- Tethering/src/android/net/ip/IpServer.java | 55 +++---------------- .../tethering/BpfCoordinator.java | 12 ++++ .../unit/src/android/net/ip/IpServerTest.java | 4 +- 3 files changed, 22 insertions(+), 49 deletions(-) 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();