Revert "Add forwarding methods to RoutingCoordinator"
This reverts commit 55ccfe19e24e0fdeab1c8c736a94da2478e65890.
Reason for revert: this must be introducing some sort of race
it appears to cause
atest EthernetTetheringTest 'NetdBinderTest#TetherForwardAddRemove'
to no longer reliably pass.
Change-Id: I5281ab3f42c5ce268d97a12db24a6768db3f4354
diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java
index 2a25a86..246e5bc 100644
--- a/Tethering/src/android/net/ip/IpServer.java
+++ b/Tethering/src/android/net/ip/IpServer.java
@@ -827,38 +827,7 @@
}
} catch (ServiceSpecificException | RemoteException e) {
mLog.e("Failed to add " + mIfaceName + " to local table: ", e);
- }
- }
-
- private void addInterfaceForward(@NonNull final String fromIface,
- @NonNull final String toIface) throws ServiceSpecificException, RemoteException {
- if (null != mRoutingCoordinator.value) {
- mRoutingCoordinator.value.addInterfaceForward(fromIface, toIface);
- } else {
- mNetd.tetherAddForward(fromIface, toIface);
- mNetd.ipfwdAddInterfaceForward(fromIface, toIface);
- }
- }
-
- private void removeInterfaceForward(@NonNull final String fromIface,
- @NonNull final String toIface) {
- if (null != mRoutingCoordinator.value) {
- try {
- mRoutingCoordinator.value.removeInterfaceForward(fromIface, toIface);
- } catch (ServiceSpecificException e) {
- mLog.e("Exception in removeInterfaceForward", e);
- }
- } else {
- try {
- mNetd.ipfwdRemoveInterfaceForward(fromIface, toIface);
- } catch (RemoteException | ServiceSpecificException e) {
- mLog.e("Exception in ipfwdRemoveInterfaceForward", e);
- }
- try {
- mNetd.tetherRemoveForward(fromIface, toIface);
- } catch (RemoteException | ServiceSpecificException e) {
- mLog.e("Exception in disableNat", e);
- }
+ return;
}
}
@@ -1368,7 +1337,16 @@
// to remove their rules, which generates errors.
// Just do the best we can.
mBpfCoordinator.maybeDetachProgram(mIfaceName, upstreamIface);
- removeInterfaceForward(mIfaceName, upstreamIface);
+ try {
+ mNetd.ipfwdRemoveInterfaceForward(mIfaceName, upstreamIface);
+ } catch (RemoteException | ServiceSpecificException e) {
+ mLog.e("Exception in ipfwdRemoveInterfaceForward: " + e.toString());
+ }
+ try {
+ mNetd.tetherRemoveForward(mIfaceName, upstreamIface);
+ } catch (RemoteException | ServiceSpecificException e) {
+ mLog.e("Exception in disableNat: " + e.toString());
+ }
}
@Override
@@ -1424,9 +1402,10 @@
mBpfCoordinator.maybeAttachProgram(mIfaceName, ifname);
try {
- addInterfaceForward(mIfaceName, ifname);
+ mNetd.tetherAddForward(mIfaceName, ifname);
+ mNetd.ipfwdAddInterfaceForward(mIfaceName, ifname);
} catch (RemoteException | ServiceSpecificException e) {
- mLog.e("Exception enabling iface forward", e);
+ mLog.e("Exception enabling NAT: " + e.toString());
cleanupUpstream();
mLastError = TETHER_ERROR_ENABLE_FORWARDING_ERROR;
transitionTo(mInitialState);
diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
index 15df3ba..fc9928d 100644
--- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
+++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
@@ -321,23 +321,6 @@
when(mTetherConfig.isBpfOffloadEnabled()).thenReturn(DEFAULT_USING_BPF_OFFLOAD);
when(mTetherConfig.useLegacyDhcpServer()).thenReturn(false /* default value */);
- // Simulate the behavior of RoutingCoordinator
- if (null != mRoutingCoordinatorManager.value) {
- doAnswer(it -> {
- final String fromIface = (String) it.getArguments()[0];
- final String toIface = (String) it.getArguments()[1];
- mNetd.tetherAddForward(fromIface, toIface);
- mNetd.ipfwdAddInterfaceForward(fromIface, toIface);
- return null;
- }).when(mRoutingCoordinatorManager.value).addInterfaceForward(any(), any());
- doAnswer(it -> {
- final String fromIface = (String) it.getArguments()[0];
- final String toIface = (String) it.getArguments()[1];
- mNetd.ipfwdRemoveInterfaceForward(fromIface, toIface);
- mNetd.tetherRemoveForward(fromIface, toIface);
- return null;
- }).when(mRoutingCoordinatorManager.value).removeInterfaceForward(any(), any());
- }
mBpfDeps = new BpfCoordinator.Dependencies() {
@NonNull
public Handler getHandler() {
diff --git a/framework/src/android/net/IRoutingCoordinator.aidl b/framework/src/android/net/IRoutingCoordinator.aidl
index cf02ec4..a5cda98 100644
--- a/framework/src/android/net/IRoutingCoordinator.aidl
+++ b/framework/src/android/net/IRoutingCoordinator.aidl
@@ -72,24 +72,4 @@
* unix errno.
*/
void removeInterfaceFromNetwork(int netId, in String iface);
-
- /**
- * Add forwarding ip rule
- *
- * @param fromIface interface name to add forwarding ip rule
- * @param toIface interface name to add forwarding ip rule
- * @throws ServiceSpecificException in case of failure, with an error code indicating the
- * cause of the failure.
- */
- void addInterfaceForward(in String fromIface, in String toIface);
-
- /**
- * Remove forwarding ip rule
- *
- * @param fromIface interface name to remove forwarding ip rule
- * @param toIface interface name to remove forwarding ip rule
- * @throws ServiceSpecificException in case of failure, with an error code indicating the
- * cause of the failure.
- */
- void removeInterfaceForward(in String fromIface, in String toIface);
}
diff --git a/framework/src/android/net/RoutingCoordinatorManager.java b/framework/src/android/net/RoutingCoordinatorManager.java
index a9e7eef..5576cb0 100644
--- a/framework/src/android/net/RoutingCoordinatorManager.java
+++ b/framework/src/android/net/RoutingCoordinatorManager.java
@@ -123,36 +123,4 @@
throw e.rethrowFromSystemServer();
}
}
-
- /**
- * Add forwarding ip rule
- *
- * @param fromIface interface name to add forwarding ip rule
- * @param toIface interface name to add forwarding ip rule
- * @throws ServiceSpecificException in case of failure, with an error code indicating the
- * cause of the failure.
- */
- public void addInterfaceForward(final String fromIface, final String toIface) {
- try {
- mService.addInterfaceForward(fromIface, toIface);
- } catch (RemoteException e) {
- throw e.rethrowFromSystemServer();
- }
- }
-
- /**
- * Remove forwarding ip rule
- *
- * @param fromIface interface name to remove forwarding ip rule
- * @param toIface interface name to remove forwarding ip rule
- * @throws ServiceSpecificException in case of failure, with an error code indicating the
- * cause of the failure.
- */
- public void removeInterfaceForward(final String fromIface, final String toIface) {
- try {
- mService.removeInterfaceForward(fromIface, toIface);
- } catch (RemoteException e) {
- throw e.rethrowFromSystemServer();
- }
- }
}
diff --git a/service/src/com/android/server/connectivity/RoutingCoordinatorService.java b/service/src/com/android/server/connectivity/RoutingCoordinatorService.java
index 92ea610..50e84d4 100644
--- a/service/src/com/android/server/connectivity/RoutingCoordinatorService.java
+++ b/service/src/com/android/server/connectivity/RoutingCoordinatorService.java
@@ -19,16 +19,12 @@
import static com.android.net.module.util.NetdUtils.toRouteInfoParcel;
import android.annotation.NonNull;
+import android.content.Context;
import android.net.INetd;
import android.net.IRoutingCoordinator;
import android.net.RouteInfo;
import android.os.RemoteException;
import android.os.ServiceSpecificException;
-import android.util.ArraySet;
-
-import com.android.internal.annotations.GuardedBy;
-
-import java.util.Objects;
/**
* Class to coordinate routing across multiple clients.
@@ -119,89 +115,4 @@
throws ServiceSpecificException, RemoteException {
mNetd.networkRemoveInterface(netId, iface);
}
-
- private final Object mIfacesLock = new Object();
- private static final class ForwardingPair {
- @NonNull public final String fromIface;
- @NonNull public final String toIface;
- ForwardingPair(@NonNull final String fromIface, @NonNull final String toIface) {
- this.fromIface = fromIface;
- this.toIface = toIface;
- }
-
- @Override
- public boolean equals(final Object o) {
- if (this == o) return true;
- if (!(o instanceof ForwardingPair)) return false;
-
- final ForwardingPair that = (ForwardingPair) o;
-
- return fromIface.equals(that.fromIface) && toIface.equals(that.toIface);
- }
-
- @Override
- public int hashCode() {
- int result = fromIface.hashCode();
- result = 2 * result + toIface.hashCode();
- return result;
- }
- }
-
- @GuardedBy("mIfacesLock")
- private final ArraySet<ForwardingPair> mForwardedInterfaces = new ArraySet<>();
-
- /**
- * Add forwarding ip rule
- *
- * @param fromIface interface name to add forwarding ip rule
- * @param toIface interface name to add forwarding ip rule
- * @throws ServiceSpecificException in case of failure, with an error code indicating the
- * cause of the failure.
- */
- public void addInterfaceForward(final String fromIface, final String toIface)
- throws ServiceSpecificException, RemoteException {
- Objects.requireNonNull(fromIface);
- Objects.requireNonNull(toIface);
- synchronized (mIfacesLock) {
- if (mForwardedInterfaces.size() == 0) {
- mNetd.ipfwdEnableForwarding("RoutingCoordinator");
- }
- final ForwardingPair fwp = new ForwardingPair(fromIface, toIface);
- if (mForwardedInterfaces.contains(fwp)) {
- throw new IllegalStateException("Forward already exists between ifaces "
- + fromIface + " → " + toIface);
- }
- mForwardedInterfaces.add(fwp);
- // Enables NAT for v4 and filters packets from unknown interfaces
- mNetd.tetherAddForward(fromIface, toIface);
- mNetd.ipfwdAddInterfaceForward(fromIface, toIface);
- }
- }
-
- /**
- * Remove forwarding ip rule
- *
- * @param fromIface interface name to remove forwarding ip rule
- * @param toIface interface name to remove forwarding ip rule
- * @throws ServiceSpecificException in case of failure, with an error code indicating the
- * cause of the failure.
- */
- public void removeInterfaceForward(final String fromIface, final String toIface)
- throws ServiceSpecificException, RemoteException {
- Objects.requireNonNull(fromIface);
- Objects.requireNonNull(toIface);
- synchronized (mIfacesLock) {
- final ForwardingPair fwp = new ForwardingPair(fromIface, toIface);
- if (!mForwardedInterfaces.contains(fwp)) {
- throw new IllegalStateException("No forward set up between interfaces "
- + fromIface + " → " + toIface);
- }
- mForwardedInterfaces.remove(fwp);
- mNetd.ipfwdRemoveInterfaceForward(fromIface, toIface);
- mNetd.tetherRemoveForward(fromIface, toIface);
- if (mForwardedInterfaces.size() == 0) {
- mNetd.ipfwdDisableForwarding("RoutingCoordinator");
- }
- }
- }
}
diff --git a/tests/unit/java/com/android/server/connectivity/RoutingCoordinatorServiceTest.kt b/tests/unit/java/com/android/server/connectivity/RoutingCoordinatorServiceTest.kt
deleted file mode 100644
index 8adf309..0000000
--- a/tests/unit/java/com/android/server/connectivity/RoutingCoordinatorServiceTest.kt
+++ /dev/null
@@ -1,65 +0,0 @@
-/*
- * Copyright (C) 2023 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package com.android.server.connectivity
-
-import android.net.INetd
-import android.os.Build
-import androidx.test.platform.app.InstrumentationRegistry
-import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo
-import com.android.testutils.DevSdkIgnoreRunner
-import org.junit.Test
-import org.junit.runner.RunWith
-import org.mockito.ArgumentMatchers.any
-import org.mockito.Mockito.inOrder
-import org.mockito.Mockito.mock
-import kotlin.test.assertFailsWith
-
-@RunWith(DevSdkIgnoreRunner::class)
-@IgnoreUpTo(Build.VERSION_CODES.TIRAMISU)
-class RoutingCoordinatorServiceTest {
- val mNetd = mock(INetd::class.java)
- val mService = RoutingCoordinatorService(mNetd)
-
- @Test
- fun testInterfaceForward() {
- val inOrder = inOrder(mNetd)
-
- mService.addInterfaceForward("from1", "to1")
- inOrder.verify(mNetd).ipfwdEnableForwarding(any())
- inOrder.verify(mNetd).tetherAddForward("from1", "to1")
- inOrder.verify(mNetd).ipfwdAddInterfaceForward("from1", "to1")
-
- mService.addInterfaceForward("from2", "to1")
- inOrder.verify(mNetd).tetherAddForward("from2", "to1")
- inOrder.verify(mNetd).ipfwdAddInterfaceForward("from2", "to1")
-
- assertFailsWith<IllegalStateException> {
- // Can't add the same pair again
- mService.addInterfaceForward("from2", "to1")
- }
-
- mService.removeInterfaceForward("from1", "to1")
- inOrder.verify(mNetd).ipfwdRemoveInterfaceForward("from1", "to1")
- inOrder.verify(mNetd).tetherRemoveForward("from1", "to1")
-
- mService.removeInterfaceForward("from2", "to1")
- inOrder.verify(mNetd).ipfwdRemoveInterfaceForward("from2", "to1")
- inOrder.verify(mNetd).tetherRemoveForward("from2", "to1")
-
- inOrder.verify(mNetd).ipfwdDisableForwarding(any())
- }
-}