From 8bf2e7e05b11e5cfe708feebb60fe91c4bf8ac1f Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Thu, 2 Apr 2020 18:47:19 +0800 Subject: [PATCH 1/2] Add tether BPF offload config to device config and resource The tether bpf offload can be enabled by resource config and device config. The device config has higher priority and it could override this config which is set by resource config. Bug: 149997301 Test: -build, flash, boot -atest TetheringConfigurationTest Change-Id: I9c26852d2c926786e141ece6da53df3801c049b2 --- Tethering/res/values/config.xml | 6 +++ Tethering/res/values/overlayable.xml | 5 ++ .../tethering/TetheringConfiguration.java | 42 +++++++++++++++-- .../tethering/TetheringConfigurationTest.java | 46 +++++++++++++++++++ 4 files changed, 95 insertions(+), 4 deletions(-) diff --git a/Tethering/res/values/config.xml b/Tethering/res/values/config.xml index c999221963..0ea459a6cf 100644 --- a/Tethering/res/values/config.xml +++ b/Tethering/res/values/config.xml @@ -55,6 +55,12 @@ "bt-pan" + + true + false diff --git a/Tethering/res/values/overlayable.xml b/Tethering/res/values/overlayable.xml index 4c78a74d53..288dd5ddf3 100644 --- a/Tethering/res/values/overlayable.xml +++ b/Tethering/res/values/overlayable.xml @@ -23,6 +23,11 @@ + + diff --git a/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java b/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java index 9d4e747327..91a6e29a05 100644 --- a/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java +++ b/Tethering/src/com/android/networkstack/tethering/TetheringConfiguration.java @@ -72,6 +72,12 @@ public class TetheringConfiguration { private static final String[] DEFAULT_IPV4_DNS = {"8.8.4.4", "8.8.8.8"}; + /** + * Override enabling BPF offload configuration for tethering. + */ + public static final String OVERRIDE_TETHER_ENABLE_BPF_OFFLOAD = + "override_tether_enable_bpf_offload"; + /** * Use the old dnsmasq DHCP server for tethering instead of the framework implementation. */ @@ -95,6 +101,8 @@ public class TetheringConfiguration { public final String[] legacyDhcpRanges; public final String[] defaultIPv4DNS; public final boolean enableLegacyDhcpServer; + // TODO: Add to TetheringConfigurationParcel if required. + public final boolean enableBpfOffload; public final String[] provisioningApp; public final String provisioningAppNoUi; @@ -124,11 +132,12 @@ public class TetheringConfiguration { isDunRequired = checkDunRequired(ctx); chooseUpstreamAutomatically = getResourceBoolean( - res, R.bool.config_tether_upstream_automatic); + res, R.bool.config_tether_upstream_automatic, false /** default value */); preferredUpstreamIfaceTypes = getUpstreamIfaceTypes(res, isDunRequired); legacyDhcpRanges = getLegacyDhcpRanges(res); defaultIPv4DNS = copy(DEFAULT_IPV4_DNS); + enableBpfOffload = getEnableBpfOffload(res); enableLegacyDhcpServer = getEnableLegacyDhcpServer(res); provisioningApp = getResourceStringArray(res, R.array.config_mobile_hotspot_provision_app); @@ -208,6 +217,9 @@ public class TetheringConfiguration { pw.print("provisioningAppNoUi: "); pw.println(provisioningAppNoUi); + pw.print("enableBpfOffload: "); + pw.println(enableBpfOffload); + pw.print("enableLegacyDhcpServer: "); pw.println(enableLegacyDhcpServer); } @@ -228,6 +240,7 @@ public class TetheringConfiguration { toIntArray(preferredUpstreamIfaceTypes))); sj.add(String.format("provisioningApp:%s", makeString(provisioningApp))); sj.add(String.format("provisioningAppNoUi:%s", provisioningAppNoUi)); + sj.add(String.format("enableBpfOffload:%s", enableBpfOffload)); sj.add(String.format("enableLegacyDhcpServer:%s", enableLegacyDhcpServer)); return String.format("TetheringConfiguration{%s}", sj.toString()); } @@ -332,11 +345,11 @@ public class TetheringConfiguration { } } - private static boolean getResourceBoolean(Resources res, int resId) { + private static boolean getResourceBoolean(Resources res, int resId, boolean defaultValue) { try { return res.getBoolean(resId); } catch (Resources.NotFoundException e404) { - return false; + return defaultValue; } } @@ -357,8 +370,29 @@ public class TetheringConfiguration { } } + private boolean getEnableBpfOffload(final Resources res) { + // Get BPF offload config + // Priority 1: Device config + // Priority 2: Resource config + // Priority 3: Default value + final boolean resourceValue = getResourceBoolean( + res, R.bool.config_tether_enable_bpf_offload, true /** default value */); + + // Due to the limitation of static mock for testing, using #getProperty directly instead + // of getDeviceConfigBoolean. getDeviceConfigBoolean is not invoked because it uses + // #getBoolean to get the boolean device config. The test can't know that the returned + // boolean value comes from device config or default value (because of null property + // string). Because the test would like to verify null property boolean string case, + // use DeviceConfig.getProperty here. See also the test case testBpfOffload{*} in + // TetheringConfigurationTest.java. + final String value = DeviceConfig.getProperty( + NAMESPACE_CONNECTIVITY, OVERRIDE_TETHER_ENABLE_BPF_OFFLOAD); + return (value != null) ? Boolean.parseBoolean(value) : resourceValue; + } + private boolean getEnableLegacyDhcpServer(final Resources res) { - return getResourceBoolean(res, R.bool.config_tether_enable_legacy_dhcp_server) + return getResourceBoolean( + res, R.bool.config_tether_enable_legacy_dhcp_server, false /** default value */) || getDeviceConfigBoolean(TETHER_ENABLE_LEGACY_DHCP_SERVER); } diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java index e8ba5b8168..fbfa871f76 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringConfigurationTest.java @@ -127,6 +127,8 @@ public class TetheringConfigurationTest { .thenReturn(new String[0]); when(mResources.getBoolean(R.bool.config_tether_enable_legacy_dhcp_server)).thenReturn( false); + initializeBpfOffloadConfiguration(true, null /* unset */); + mHasTelephonyManager = true; mMockContext = new MockContext(mContext); mEnableLegacyDhcpServer = false; @@ -278,6 +280,50 @@ public class TetheringConfigurationTest { assertFalse(upstreamIterator.hasNext()); } + private void initializeBpfOffloadConfiguration( + final boolean fromRes, final String fromDevConfig) { + when(mResources.getBoolean(R.bool.config_tether_enable_bpf_offload)).thenReturn(fromRes); + doReturn(fromDevConfig).when( + () -> DeviceConfig.getProperty(eq(NAMESPACE_CONNECTIVITY), + eq(TetheringConfiguration.OVERRIDE_TETHER_ENABLE_BPF_OFFLOAD))); + } + + @Test + public void testBpfOffloadEnabledByResource() { + initializeBpfOffloadConfiguration(true, null /* unset */); + final TetheringConfiguration enableByRes = + new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); + assertTrue(enableByRes.enableBpfOffload); + } + + @Test + public void testBpfOffloadEnabledByDeviceConfigOverride() { + for (boolean res : new boolean[]{true, false}) { + initializeBpfOffloadConfiguration(res, "true"); + final TetheringConfiguration enableByDevConOverride = + new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); + assertTrue(enableByDevConOverride.enableBpfOffload); + } + } + + @Test + public void testBpfOffloadDisabledByResource() { + initializeBpfOffloadConfiguration(false, null /* unset */); + final TetheringConfiguration disableByRes = + new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); + assertFalse(disableByRes.enableBpfOffload); + } + + @Test + public void testBpfOffloadDisabledByDeviceConfigOverride() { + for (boolean res : new boolean[]{true, false}) { + initializeBpfOffloadConfiguration(res, "false"); + final TetheringConfiguration disableByDevConOverride = + new TetheringConfiguration(mMockContext, mLog, INVALID_SUBSCRIPTION_ID); + assertFalse(disableByDevConOverride.enableBpfOffload); + } + } + @Test public void testNewDhcpServerDisabled() { when(mResources.getBoolean(R.bool.config_tether_enable_legacy_dhcp_server)).thenReturn( From 3d8fa889b42f56c3e858bccbbc545595df5ff5e2 Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Sun, 12 Apr 2020 14:27:18 +0800 Subject: [PATCH 2/2] Use device option to control BPF offload features If BPF offload device config is not enabled: - Does not add/remove offload forwarding rules through disabling IP neighbor monitor. - Does not apply the RA MTU reduction. Bug: 149997301 Test: atest IpServerTest Change-Id: I2d6f80f0229f580c4b16243a064e889a6c37f77a --- Tethering/src/android/net/ip/IpServer.java | 34 +++++++-- .../networkstack/tethering/Tethering.java | 2 +- .../unit/src/android/net/ip/IpServerTest.java | 71 ++++++++++++++++--- 3 files changed, 90 insertions(+), 17 deletions(-) diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java index 83727bcdc6..ca485f5da5 100644 --- a/Tethering/src/android/net/ip/IpServer.java +++ b/Tethering/src/android/net/ip/IpServer.java @@ -227,6 +227,7 @@ public class IpServer extends StateMachine { private final int mInterfaceType; private final LinkProperties mLinkProperties; private final boolean mUsingLegacyDhcp; + private final boolean mUsingBpfOffload; private final Dependencies mDeps; @@ -304,7 +305,8 @@ public class IpServer extends StateMachine { public IpServer( String ifaceName, Looper looper, int interfaceType, SharedLog log, - INetd netd, Callback callback, boolean usingLegacyDhcp, Dependencies deps) { + INetd netd, Callback callback, boolean usingLegacyDhcp, boolean usingBpfOffload, + Dependencies deps) { super(ifaceName, looper); mLog = log.forSubComponent(ifaceName); mNetd = netd; @@ -314,6 +316,7 @@ public class IpServer extends StateMachine { mInterfaceType = interfaceType; mLinkProperties = new LinkProperties(); mUsingLegacyDhcp = usingLegacyDhcp; + mUsingBpfOffload = usingBpfOffload; mDeps = deps; resetLinkProperties(); mLastError = TetheringManager.TETHER_ERROR_NO_ERROR; @@ -321,8 +324,15 @@ public class IpServer extends StateMachine { mIpNeighborMonitor = mDeps.getIpNeighborMonitor(getHandler(), mLog, new MyNeighborEventConsumer()); - if (!mIpNeighborMonitor.start()) { - mLog.e("Failed to create IpNeighborMonitor on " + mIfaceName); + + // IP neighbor monitor monitors the neighbor event for adding/removing offload + // forwarding rules per client. If BPF offload is not supported, don't start listening + // neighbor events. See updateIpv6ForwardingRules, addIpv6ForwardingRule, + // removeIpv6ForwardingRule. + if (mUsingBpfOffload) { + if (!mIpNeighborMonitor.start()) { + mLog.e("Failed to create IpNeighborMonitor on " + mIfaceName); + } } mInitialState = new InitialState(); @@ -715,12 +725,12 @@ public class IpServer extends StateMachine { final String upstreamIface = v6only.getInterfaceName(); params = new RaParams(); - // We advertise an mtu lower by 16, which is the closest multiple of 8 >= 14, - // the ethernet header size. This makes kernel ebpf tethering offload happy. - // This hack should be reverted once we have the kernel fixed up. + // When BPF offload is enabled, we advertise an mtu lower by 16, which is the closest + // multiple of 8 >= 14, the ethernet header size. This makes kernel ebpf tethering + // offload happy. This hack should be reverted once we have the kernel fixed up. // Note: this will automatically clamp to at least 1280 (ipv6 minimum mtu) // see RouterAdvertisementDaemon.java putMtu() - params.mtu = v6only.getMtu() - 16; + params.mtu = mUsingBpfOffload ? v6only.getMtu() - 16 : v6only.getMtu(); params.hasDefaultRoute = v6only.hasIpv6DefaultRoute(); if (params.hasDefaultRoute) params.hopLimit = getHopLimit(upstreamIface); @@ -844,6 +854,11 @@ public class IpServer extends StateMachine { } private void addIpv6ForwardingRule(Ipv6ForwardingRule 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; + try { mNetd.tetherOffloadRuleAdd(rule.toTetherOffloadRuleParcel()); mIpv6ForwardingRules.put(rule.address, rule); @@ -853,6 +868,11 @@ public class IpServer extends StateMachine { } private void removeIpv6ForwardingRule(Ipv6ForwardingRule rule, boolean removeFromMap) { + // 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; + try { mNetd.tetherOffloadRuleRemove(rule.toTetherOffloadRuleParcel()); if (removeFromMap) { diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 00b94a8bbf..a31d5478ee 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -2289,7 +2289,7 @@ public class Tethering { final TetherState tetherState = new TetherState( new IpServer(iface, mLooper, interfaceType, mLog, mNetd, makeControlCallback(), mConfig.enableLegacyDhcpServer, - mDeps.getIpServerDependencies())); + mConfig.enableBpfOffload, mDeps.getIpServerDependencies())); mTetherStates.put(iface, tetherState); tetherState.ipServer.start(); } diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index f9be7b9d36..b9622da9d2 100644 --- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -106,6 +106,7 @@ public class IpServerTest { private static final String BLUETOOTH_IFACE_ADDR = "192.168.42.1"; private static final int BLUETOOTH_DHCP_PREFIX_LENGTH = 24; private static final int DHCP_LEASE_TIME_SECS = 3600; + private static final boolean DEFAULT_USING_BPF_OFFLOAD = true; private static final InterfaceParams TEST_IFACE_PARAMS = new InterfaceParams( IFACE_NAME, 42 /* index */, MacAddress.ALL_ZEROS_ADDRESS, 1500 /* defaultMtu */); @@ -130,10 +131,11 @@ public class IpServerTest { private NeighborEventConsumer mNeighborEventConsumer; private void initStateMachine(int interfaceType) throws Exception { - initStateMachine(interfaceType, false /* usingLegacyDhcp */); + initStateMachine(interfaceType, false /* usingLegacyDhcp */, DEFAULT_USING_BPF_OFFLOAD); } - private void initStateMachine(int interfaceType, boolean usingLegacyDhcp) throws Exception { + private void initStateMachine(int interfaceType, boolean usingLegacyDhcp, + boolean usingBpfOffload) throws Exception { doAnswer(inv -> { final IDhcpServerCallbacks cb = inv.getArgument(2); new Thread(() -> { @@ -165,7 +167,7 @@ public class IpServerTest { mIpServer = new IpServer( IFACE_NAME, mLooper.getLooper(), interfaceType, mSharedLog, mNetd, - mCallback, usingLegacyDhcp, mDependencies); + mCallback, usingLegacyDhcp, usingBpfOffload, mDependencies); mIpServer.start(); mNeighborEventConsumer = neighborCaptor.getValue(); @@ -179,12 +181,13 @@ public class IpServerTest { private void initTetheredStateMachine(int interfaceType, String upstreamIface) throws Exception { - initTetheredStateMachine(interfaceType, upstreamIface, false); + initTetheredStateMachine(interfaceType, upstreamIface, false, + DEFAULT_USING_BPF_OFFLOAD); } private void initTetheredStateMachine(int interfaceType, String upstreamIface, - boolean usingLegacyDhcp) throws Exception { - initStateMachine(interfaceType, usingLegacyDhcp); + boolean usingLegacyDhcp, boolean usingBpfOffload) throws Exception { + initStateMachine(interfaceType, usingLegacyDhcp, usingBpfOffload); dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED); if (upstreamIface != null) { LinkProperties lp = new LinkProperties(); @@ -204,7 +207,8 @@ public class IpServerTest { when(mDependencies.getIpNeighborMonitor(any(), any(), any())) .thenReturn(mIpNeighborMonitor); mIpServer = new IpServer(IFACE_NAME, mLooper.getLooper(), TETHERING_BLUETOOTH, mSharedLog, - mNetd, mCallback, false /* usingLegacyDhcp */, mDependencies); + mNetd, mCallback, false /* usingLegacyDhcp */, DEFAULT_USING_BPF_OFFLOAD, + mDependencies); mIpServer.start(); mLooper.dispatchAll(); verify(mCallback).updateInterfaceState( @@ -494,7 +498,8 @@ public class IpServerTest { @Test public void doesNotStartDhcpServerIfDisabled() throws Exception { - initTetheredStateMachine(TETHERING_WIFI, UPSTREAM_IFACE, true /* usingLegacyDhcp */); + initTetheredStateMachine(TETHERING_WIFI, UPSTREAM_IFACE, true /* usingLegacyDhcp */, + DEFAULT_USING_BPF_OFFLOAD); dispatchTetherConnectionChanged(UPSTREAM_IFACE); verify(mDependencies, never()).makeDhcpServer(any(), any(), any()); @@ -577,7 +582,8 @@ public class IpServerTest { @Test public void addRemoveipv6ForwardingRules() throws Exception { - initTetheredStateMachine(TETHERING_WIFI, UPSTREAM_IFACE, false /* usingLegacyDhcp */); + initTetheredStateMachine(TETHERING_WIFI, UPSTREAM_IFACE, false /* usingLegacyDhcp */, + DEFAULT_USING_BPF_OFFLOAD); final int myIfindex = TEST_IFACE_PARAMS.index; final int notMyIfindex = myIfindex - 1; @@ -678,6 +684,53 @@ public class IpServerTest { reset(mNetd); } + @Test + public void enableDisableUsingBpfOffload() throws Exception { + final int myIfindex = TEST_IFACE_PARAMS.index; + final InetAddress neigh = InetAddresses.parseNumericAddress("2001:db8::1"); + final MacAddress macA = MacAddress.fromString("00:00:00:00:00:0a"); + final MacAddress macNull = MacAddress.fromString("00:00:00:00:00:00"); + + reset(mNetd); + + // Expect that rules can be only added/removed when the BPF offload config is enabled. + // Note that the usingBpfOffload false case is not a realistic test case. Because IP + // neighbor monitor doesn't start if BPF offload is disabled, there should have no + // neighbor event listening. This is used for testing the protection check just in case. + // TODO: Perhaps remove this test once we don't need this check anymore. + for (boolean usingBpfOffload : new boolean[]{true, false}) { + initTetheredStateMachine(TETHERING_WIFI, UPSTREAM_IFACE, false /* usingLegacyDhcp */, + usingBpfOffload); + + // A neighbor is added. + recvNewNeigh(myIfindex, neigh, NUD_REACHABLE, macA); + if (usingBpfOffload) { + verify(mNetd).tetherOffloadRuleAdd(matches(UPSTREAM_IFINDEX, neigh, macA)); + } else { + verify(mNetd, never()).tetherOffloadRuleAdd(any()); + } + reset(mNetd); + + // A neighbor is deleted. + recvDelNeigh(myIfindex, neigh, NUD_STALE, macA); + if (usingBpfOffload) { + verify(mNetd).tetherOffloadRuleRemove(matches(UPSTREAM_IFINDEX, neigh, macNull)); + } else { + verify(mNetd, never()).tetherOffloadRuleRemove(any()); + } + reset(mNetd); + } + } + + @Test + public void doesNotStartIpNeighborMonitorIfBpfOffloadDisabled() throws Exception { + initTetheredStateMachine(TETHERING_WIFI, UPSTREAM_IFACE, false /* usingLegacyDhcp */, + false /* usingBpfOffload */); + + // IP neighbor monitor doesn't start if BPF offload is disabled. + verify(mIpNeighborMonitor, never()).start(); + } + private void assertDhcpStarted(IpPrefix expectedPrefix) throws Exception { verify(mDependencies, times(1)).makeDhcpServer(eq(IFACE_NAME), any(), any()); verify(mDhcpServer, timeout(MAKE_DHCPSERVER_TIMEOUT_MS).times(1)).startWithCallbacks(