ethernet: replace netd link observer with NetlinkMonitor
The netd based InterfaceObserver uses two different netlink sockets for
updates on interface added / removed and link up / down events. This
means that these events are not ordered, which causes lots of
interesting issues in the ethernet service. Netd does not use
RTM_DELLINK at all, so this code has to use NetlinkMonitor to do
implement its own link tracking.
The EthernetManagerTests are currently disabled. There are still a
couple of flakes: one due to some link state issue which stops occurring
when bringing up the interface in TestNetworkService (weird -- I don't
yet know why this happens). Another due to the maybeTrackInterface
looking at the running flag (which races with the bringup code).
Test: atest EthernetManagerTest
Bug: 218785176
Bug: 234314411
Change-Id: I92b737f693402c1a8fd0a864736673de94904f2d
diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java
index be9beed..f3e49f8 100644
--- a/service-t/src/com/android/server/ethernet/EthernetTracker.java
+++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java
@@ -43,15 +43,21 @@
import android.os.RemoteCallbackList;
import android.os.RemoteException;
import android.os.ServiceSpecificException;
+import android.system.OsConstants;
import android.text.TextUtils;
import android.util.ArrayMap;
import android.util.Log;
import com.android.internal.annotations.VisibleForTesting;
import com.android.internal.util.IndentingPrintWriter;
-import com.android.net.module.util.BaseNetdUnsolicitedEventListener;
import com.android.net.module.util.NetdUtils;
import com.android.net.module.util.PermissionUtils;
+import com.android.net.module.util.SharedLog;
+import com.android.net.module.util.ip.NetlinkMonitor;
+import com.android.net.module.util.netlink.NetlinkConstants;
+import com.android.net.module.util.netlink.NetlinkMessage;
+import com.android.net.module.util.netlink.RtNetlinkLinkMessage;
+import com.android.net.module.util.netlink.StructIfinfoMsg;
import java.io.FileDescriptor;
import java.net.InetAddress;
@@ -86,6 +92,9 @@
private static final String TEST_IFACE_REGEXP = TEST_TAP_PREFIX + "\\d+";
+ // TODO: consider using SharedLog consistently across ethernet service.
+ private static final SharedLog sLog = new SharedLog(TAG);
+
/**
* Interface names we track. This is a product-dependent regular expression.
* Use isValidEthernetInterface to check if a interface name is a valid ethernet interface (this
@@ -110,6 +119,7 @@
private final Handler mHandler;
private final EthernetNetworkFactory mFactory;
private final EthernetConfigStore mConfigStore;
+ private final NetlinkMonitor mNetlinkMonitor;
private final Dependencies mDeps;
private final RemoteCallbackList<IEthernetServiceListener> mListeners =
@@ -148,6 +158,69 @@
}
}
+ private class EthernetNetlinkMonitor extends NetlinkMonitor {
+ EthernetNetlinkMonitor(Handler handler) {
+ super(handler, sLog, EthernetNetlinkMonitor.class.getSimpleName(),
+ OsConstants.NETLINK_ROUTE, NetlinkConstants.RTMGRP_LINK);
+ }
+
+ private void onNewLink(String ifname, boolean linkUp) {
+ if (!mFactory.hasInterface(ifname) && !ifname.equals(mDefaultInterface)) {
+ Log.i(TAG, "onInterfaceAdded, iface: " + ifname);
+ maybeTrackInterface(ifname);
+ }
+ Log.i(TAG, "interfaceLinkStateChanged, iface: " + ifname + ", up: " + linkUp);
+ updateInterfaceState(ifname, linkUp);
+ }
+
+ private void onDelLink(String ifname) {
+ Log.i(TAG, "onInterfaceRemoved, iface: " + ifname);
+ stopTrackingInterface(ifname);
+ }
+
+ private void processRtNetlinkLinkMessage(RtNetlinkLinkMessage msg) {
+ final StructIfinfoMsg ifinfomsg = msg.getIfinfoHeader();
+ // check if the message is valid
+ if (ifinfomsg.family != OsConstants.AF_UNSPEC) return;
+
+ // ignore messages for the loopback interface
+ if ((ifinfomsg.flags & OsConstants.IFF_LOOPBACK) != 0) return;
+
+ // check if the received message applies to an ethernet interface.
+ final String ifname = msg.getInterfaceName();
+ if (!isValidEthernetInterface(ifname)) return;
+
+ switch (msg.getHeader().nlmsg_type) {
+ case NetlinkConstants.RTM_NEWLINK:
+ final boolean linkUp = (ifinfomsg.flags & NetlinkConstants.IFF_LOWER_UP) != 0;
+ onNewLink(ifname, linkUp);
+ break;
+
+ case NetlinkConstants.RTM_DELLINK:
+ onDelLink(ifname);
+ break;
+
+ default:
+ Log.e(TAG, "Unknown rtnetlink link msg type: " + msg);
+ break;
+ }
+ }
+
+ // Note: processNetlinkMessage is called on the handler thread.
+ @Override
+ protected void processNetlinkMessage(NetlinkMessage nlMsg, long whenMs) {
+ // ignore all updates when ethernet is disabled.
+ if (mEthernetState == ETHERNET_STATE_DISABLED) return;
+
+ if (nlMsg instanceof RtNetlinkLinkMessage) {
+ processRtNetlinkLinkMessage((RtNetlinkLinkMessage) nlMsg);
+ } else {
+ Log.e(TAG, "Unknown netlink message: " + nlMsg);
+ }
+ }
+ }
+
+
EthernetTracker(@NonNull final Context context, @NonNull final Handler handler,
@NonNull final EthernetNetworkFactory factory, @NonNull final INetd netd) {
this(context, handler, factory, netd, new Dependencies());
@@ -173,6 +246,7 @@
}
mConfigStore = new EthernetConfigStore();
+ mNetlinkMonitor = new EthernetNetlinkMonitor(mHandler);
}
void start() {
@@ -186,14 +260,10 @@
mIpConfigurations.put(configs.keyAt(i), configs.valueAt(i));
}
- try {
- PermissionUtils.enforceNetworkStackPermission(mContext);
- mNetd.registerUnsolicitedEventListener(new InterfaceObserver());
- } catch (RemoteException | ServiceSpecificException e) {
- Log.e(TAG, "Could not register InterfaceObserver " + e);
- }
-
- mHandler.post(this::trackAvailableInterfaces);
+ mHandler.post(() -> {
+ mNetlinkMonitor.start();
+ trackAvailableInterfaces();
+ });
}
void updateIpConfiguration(String iface, IpConfiguration ipConfiguration) {
@@ -617,43 +687,6 @@
}
}
- @VisibleForTesting
- class InterfaceObserver extends BaseNetdUnsolicitedEventListener {
-
- @Override
- public void onInterfaceLinkStateChanged(String iface, boolean up) {
- if (DBG) {
- Log.i(TAG, "interfaceLinkStateChanged, iface: " + iface + ", up: " + up);
- }
- mHandler.post(() -> {
- if (mEthernetState == ETHERNET_STATE_DISABLED) return;
- updateInterfaceState(iface, up);
- });
- }
-
- @Override
- public void onInterfaceAdded(String iface) {
- if (DBG) {
- Log.i(TAG, "onInterfaceAdded, iface: " + iface);
- }
- mHandler.post(() -> {
- if (mEthernetState == ETHERNET_STATE_DISABLED) return;
- maybeTrackInterface(iface);
- });
- }
-
- @Override
- public void onInterfaceRemoved(String iface) {
- if (DBG) {
- Log.i(TAG, "onInterfaceRemoved, iface: " + iface);
- }
- mHandler.post(() -> {
- if (mEthernetState == ETHERNET_STATE_DISABLED) return;
- stopTrackingInterface(iface);
- });
- }
- }
-
private static class ListenerInfo {
boolean canUseRestrictedNetworks = false;
diff --git a/service/Android.bp b/service/Android.bp
index 7dcc888..1e70d68 100644
--- a/service/Android.bp
+++ b/service/Android.bp
@@ -164,6 +164,7 @@
"modules-utils-shell-command-handler",
"net-utils-device-common",
"net-utils-device-common-bpf",
+ "net-utils-device-common-ip",
"net-utils-device-common-netlink",
"net-utils-services-common",
"netd-client",
diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java
index 38094ae..0376a2a 100644
--- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java
+++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java
@@ -30,10 +30,8 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.timeout;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@@ -64,7 +62,6 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
@@ -476,43 +473,4 @@
verify(listener).onInterfaceStateChanged(eq(testIface), eq(EthernetManager.STATE_LINK_UP),
anyInt(), any());
}
-
- @Test
- public void testListenEthernetStateChange_unsolicitedEventListener() throws Exception {
- when(mNetd.interfaceGetList()).thenReturn(new String[] {});
- doReturn(new String[] {}).when(mFactory).getAvailableInterfaces(anyBoolean());
-
- tracker.setIncludeTestInterfaces(true);
- tracker.start();
-
- final ArgumentCaptor<EthernetTracker.InterfaceObserver> captor =
- ArgumentCaptor.forClass(EthernetTracker.InterfaceObserver.class);
- verify(mNetd, timeout(TIMEOUT_MS)).registerUnsolicitedEventListener(captor.capture());
- final EthernetTracker.InterfaceObserver observer = captor.getValue();
-
- tracker.setEthernetEnabled(false);
- waitForIdle();
- reset(mFactory);
- reset(mNetd);
-
- final String testIface = "testtap1";
- observer.onInterfaceAdded(testIface);
- verify(mFactory, never()).addInterface(eq(testIface), anyString(), any(), any());
- observer.onInterfaceRemoved(testIface);
- verify(mFactory, never()).removeInterface(eq(testIface));
-
- final String testHwAddr = "11:22:33:44:55:66";
- final InterfaceConfigurationParcel testIfaceParce =
- createMockedIfaceParcel(testIface, testHwAddr);
- when(mNetd.interfaceGetList()).thenReturn(new String[] {testIface});
- when(mNetd.interfaceGetCfg(eq(testIface))).thenReturn(testIfaceParce);
- doReturn(new String[] {testIface}).when(mFactory).getAvailableInterfaces(anyBoolean());
- tracker.setEthernetEnabled(true);
- waitForIdle();
- reset(mFactory);
-
- final String testIface2 = "testtap2";
- observer.onInterfaceRemoved(testIface2);
- verify(mFactory, timeout(TIMEOUT_MS)).removeInterface(eq(testIface2));
- }
}