From 7c2b72901d05340b461e39e301b5a5eda125d7d0 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Mon, 2 Jun 2025 15:04:23 -0700 Subject: [PATCH 1/5] Test EthernetTracker#trackAvailableInterfaces Coverage for this code path was removed a while ago, because the old test relied on the old implementation of setEthernetEnabled(). This test adds a similar test based on setIncludeTestInterfaces() instead, which calls the trackAvailableInterfaces() code path. trackAvailableInterfaces() is used when the ethernet service first starts up (either at boot, or after a system server crash). Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I635031bb670ef62d4ccd0cb67d282ee907219089 --- .../android/net/cts/EthernetManagerTest.kt | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index b1b237ed1d..6b3bfdaaf9 100644 --- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt @@ -667,6 +667,36 @@ class EthernetManagerTest { listener.assertNoCallback() } + @Test + fun testCallbacks_withRunningInterfaces() { + // Do not run this test when unrestricted interfaces are present. Refer to testCallbacks. + assumeNoInterfaceForTetheringAvailable() + + // The interfaces must be created before setIncludeTestInterfaces is set to false, so the + // test can ensure that the RTM_NEWLINK has been processed before proceeding. + val iface1 = createInterface() + val iface2 = createInterface() + val iface3 = createInterface() + + // Prevent test interfaces from being tracked and wait for the call to be processed by + // calling setEthernetEnabled() which waits on a completion callback. + setIncludeTestInterfaces(false) + setEthernetEnabled(true) + + val listener = EthernetStateListener() + addInterfaceStateListener(listener) + listener.assertNoCallback() + + // Include test interfaces again, and ensure that they are tracked properly. + setIncludeTestInterfaces(true) + setEthernetEnabled(true) + + listener.expectCallback(iface1, STATE_LINK_UP, ROLE_CLIENT) + listener.expectCallback(iface2, STATE_LINK_UP, ROLE_CLIENT) + listener.expectCallback(iface3, STATE_LINK_UP, ROLE_CLIENT) + listener.assertNoCallback() + } + @Test fun testGetInterfaceList() { // Create two test interfaces and check the return list contains the interface names. From 50d7eee5a41fed72f9c05100304178668955dd9e Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Wed, 4 Jun 2025 10:41:42 -0700 Subject: [PATCH 2/5] Remove tests which are covered by EthernetManagerTest Test interface tracking is already covered by EthernetManagerTest (in fact, that test relies heavily on this functionality), so it does not need to be part of EthernetTrackerTest. Additionally, both isValidTestInterface() and setIncludeTestInterfaces() are trivial functions that do not improve coverage significantly. Long-term, the ethernet unit tests should be reimagined as more of a module-wide unit test, rather than testing individual classes. This will make the tests much more valuable and could allow testing scenarios that are not possible in a end-to-end integration test. Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I94540b6e7b94b1de00965f4e54393d573788c59a --- .../server/ethernet/EthernetTrackerTest.java | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java index c6ae03985c..def064a03f 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java @@ -259,37 +259,4 @@ public class EthernetTrackerTest { verify(mFactory).updateInterface( eq(TEST_IFACE), eq(ipConfig), eq(capabilities)); } - - @Test - public void testIsValidTestInterfaceIsFalseWhenTestInterfacesAreNotIncluded() { - final String validIfaceName = TEST_TAP_PREFIX + "123"; - tracker.setIncludeTestInterfaces(false); - waitForIdle(); - - final boolean isValidTestInterface = tracker.isValidTestInterface(validIfaceName); - - assertFalse(isValidTestInterface); - } - - @Test - public void testIsValidTestInterfaceIsFalseWhenTestInterfaceNameIsInvalid() { - final String invalidIfaceName = "123" + TEST_TAP_PREFIX; - tracker.setIncludeTestInterfaces(true); - waitForIdle(); - - final boolean isValidTestInterface = tracker.isValidTestInterface(invalidIfaceName); - - assertFalse(isValidTestInterface); - } - - @Test - public void testIsValidTestInterfaceIsTrueWhenTestInterfacesIncludedAndValidName() { - final String validIfaceName = TEST_TAP_PREFIX + "123"; - tracker.setIncludeTestInterfaces(true); - waitForIdle(); - - final boolean isValidTestInterface = tracker.isValidTestInterface(validIfaceName); - - assertTrue(isValidTestInterface); - } } From 5fd68b67854d3c84b7c81a609d05115af0698d7e Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Tue, 3 Jun 2025 14:37:16 -0700 Subject: [PATCH 3/5] Change trackAvailable to trigger GETLINK dump Instead of calling NetworkInterface#getNetworkInterfaces() (which calls getifaddrs() and triggers an RTM_GETLINK dump request), trigger the RTM_GETLINK dump request in EthernetTracker directly. The resulting RTM_NEWLINK messages will be processed by the NetlinkMonitor callback. This means that there will be a single entry point for link tracking which contains all required information (ifindex, ifname, mac). This change also makes the behavior of EthernetTracker more correct. Previously, the interface list was retrieved out-of-band from EthernetNetlinkMonitor. This means that it was possible for an RTM_NEWLINK to be queued at the same time as #getNetworkInterfaces() was called. In that case, #getNetworkInterfaces() returns up-to-date information and is processed before the queued RTM_NEWLINK event with the now outdated information. The new code retrieves all information on a single netlink socket and processes them in order on the same handler thread. Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I1e6e17fa4eb3dd8fe308fd54c2bb16a8ad36dc43 --- .../server/ethernet/EthernetTracker.java | 27 ++++++++++++++++--- .../net/module/util/FdEventsReader.java | 5 ++++ .../net/module/util/ip/NetlinkMonitor.java | 24 +++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index f3a0af3f81..18abff11ad 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -26,6 +26,7 @@ import static android.net.TestNetworkManager.TEST_TAP_PREFIX; import static com.android.internal.annotations.VisibleForTesting.Visibility.PACKAGE; import static com.android.net.module.util.netlink.NetlinkConstants.IFF_UP; +import static com.android.net.module.util.netlink.NetlinkConstants.RTM_GETLINK; import android.annotation.NonNull; import android.annotation.Nullable; @@ -64,12 +65,15 @@ import com.android.net.module.util.netlink.NetlinkMessage; import com.android.net.module.util.netlink.NetlinkUtils; import com.android.net.module.util.netlink.RtNetlinkLinkMessage; import com.android.net.module.util.netlink.StructIfinfoMsg; +import com.android.net.module.util.netlink.StructNlMsgHdr; import com.android.server.connectivity.ConnectivityResources; import java.io.FileDescriptor; import java.net.InetAddress; import java.net.NetworkInterface; import java.net.SocketException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; import java.util.ArrayList; import java.util.Enumeration; import java.util.Iterator; @@ -763,10 +767,25 @@ public class EthernetTracker { } private void trackAvailableInterfaces() { - final List ifaces = getEthernetInterfaceList(); - for (String iface : ifaces) { - maybeTrackInterface(iface); - } + // TODO: Clean up NetlinkMessage classes and add support for building a generic GETLINK + // message. + // Allocate enough space for struct nlmsghdr + struct rtgenmsg + final ByteBuffer buf = ByteBuffer.allocate(StructNlMsgHdr.STRUCT_SIZE + 1); + buf.order(ByteOrder.nativeOrder()); + + final StructNlMsgHdr nlmsghdr = new StructNlMsgHdr(); + nlmsghdr.nlmsg_len = buf.capacity(); + nlmsghdr.nlmsg_type = RTM_GETLINK; + nlmsghdr.nlmsg_flags = StructNlMsgHdr.NLM_F_DUMP | StructNlMsgHdr.NLM_F_REQUEST; + nlmsghdr.pack(buf); + + // struct rtgenmsg { + // unsigned char rtgen_family; + // } + buf.put((byte) OsConstants.AF_UNSPEC); + buf.flip(); + + mNetlinkMonitor.sendNetlinkMessage(buf); } private static class ListenerInfo { diff --git a/staticlibs/device/com/android/net/module/util/FdEventsReader.java b/staticlibs/device/com/android/net/module/util/FdEventsReader.java index f88883bf22..e6c01416b9 100644 --- a/staticlibs/device/com/android/net/module/util/FdEventsReader.java +++ b/staticlibs/device/com/android/net/module/util/FdEventsReader.java @@ -141,6 +141,11 @@ public abstract class FdEventsReader { return mPacketsReceived; } + /** Returns the FileDescriptor if the FdEventsReader is running, null otherwise. */ + protected FileDescriptor getFd() { + return isRunning() ? mFd : null; + } + /** * Subclasses MUST create the listening socket here, including setting all desired socket * options, interface or address/port binding, etc. The socket MUST be created nonblocking. diff --git a/staticlibs/device/com/android/net/module/util/ip/NetlinkMonitor.java b/staticlibs/device/com/android/net/module/util/ip/NetlinkMonitor.java index 15a46337a3..b4309e76ba 100644 --- a/staticlibs/device/com/android/net/module/util/ip/NetlinkMonitor.java +++ b/staticlibs/device/com/android/net/module/util/ip/NetlinkMonitor.java @@ -41,6 +41,7 @@ import com.android.net.module.util.netlink.NetlinkMessage; import com.android.net.module.util.netlink.NetlinkUtils; import java.io.FileDescriptor; +import java.io.InterruptedIOException; import java.net.SocketAddress; import java.net.SocketException; import java.nio.ByteBuffer; @@ -95,6 +96,29 @@ public class NetlinkMonitor extends PacketReader { this(h, log, tag, family, bindGroups, DEFAULT_SOCKET_RECV_BUFSIZE); } + /** + * Send a netlink message on the underlying netlink socket. + * + * Any responses will be processed by {@link #processNetlinkMessage}. This is particularly + * useful to request a dump of the current state when the {@link NetlinkMonitor} is first + * created. + * This prevents having an application to open multiple netlink sockets of the same family (for + * example by using a utility function in {@link NetlinkUtils} which can introduce races. + * + * TODO(b/422484024): add builder support to NetlinkMessage and accept a NetlinkMessage object + * rather than a ByteBuffer. + */ + public void sendNetlinkMessage(ByteBuffer msg) { + if (!isRunning()) throw new IllegalStateException("NetlinkMonitor must be running"); + + final FileDescriptor fd = getFd(); + try { + Os.write(fd, msg); + } catch (ErrnoException | InterruptedIOException e) { + Log.e(mTag, "Failed to send netlink message", e); + } + } + @Override protected FileDescriptor createFd() { FileDescriptor fd = null; From abab191ff392a2b2d012ae6877664056e2e9fcc9 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Tue, 3 Jun 2025 16:38:02 -0700 Subject: [PATCH 4/5] Move trackAvailableInterfaces into EthernetNetlinkMonitor This change moves trackAvailableInterfaces into EthernetNetlinkMonitor and renames it to requestLinkDump. Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I0d3173a6376207a5391deb696df666534e700276 --- .../server/ethernet/EthernetTracker.java | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 18abff11ad..cf77f4935f 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -149,7 +149,7 @@ public class EthernetTracker { private final Handler mHandler; private final EthernetNetworkFactory mFactory; private final EthernetConfigStore mConfigStore; - private final NetlinkMonitor mNetlinkMonitor; + private final EthernetNetlinkMonitor mNetlinkMonitor; private final Dependencies mDeps; private final RemoteCallbackList mListeners = @@ -203,6 +203,29 @@ public class EthernetTracker { OsConstants.NETLINK_ROUTE, NetlinkConstants.RTMGRP_LINK); } + /** Request RTM_GETLINK dump. Resulting callbacks are handled by #processNetlinkMessage. */ + public void requestLinkDump() { + // TODO(b/422484024): Clean up NetlinkMessage classes and add support for building a + // generic GETLINK message. + // Allocate enough space for struct nlmsghdr + struct rtgenmsg + final ByteBuffer buf = ByteBuffer.allocate(StructNlMsgHdr.STRUCT_SIZE + 1); + buf.order(ByteOrder.nativeOrder()); + + final StructNlMsgHdr nlmsghdr = new StructNlMsgHdr(); + nlmsghdr.nlmsg_len = buf.capacity(); + nlmsghdr.nlmsg_type = RTM_GETLINK; + nlmsghdr.nlmsg_flags = StructNlMsgHdr.NLM_F_DUMP | StructNlMsgHdr.NLM_F_REQUEST; + nlmsghdr.pack(buf); + + // struct rtgenmsg { + // unsigned char rtgen_family; + // } + buf.put((byte) OsConstants.AF_UNSPEC); + buf.flip(); + + sendNetlinkMessage(buf); + } + private void onNewLink(EthernetPort port, boolean linkUp) { final String ifname = port.getInterfaceName(); if (!mFactory.hasInterface(ifname) && !ifname.equals(mTetheringInterface)) { @@ -314,7 +337,7 @@ public class EthernetTracker { mHandler.post(() -> { mNetlinkMonitor.start(); - trackAvailableInterfaces(); + mNetlinkMonitor.requestLinkDump(); }); } @@ -507,7 +530,7 @@ public class EthernetTracker { mHandler.post(() -> { mIncludeTestInterfaces = include; if (include) { - trackAvailableInterfaces(); + mNetlinkMonitor.requestLinkDump(); } else { removeTestData(); // remove all test interfaces @@ -766,28 +789,6 @@ public class EthernetTracker { broadcastInterfaceStateChange(iface); } - private void trackAvailableInterfaces() { - // TODO: Clean up NetlinkMessage classes and add support for building a generic GETLINK - // message. - // Allocate enough space for struct nlmsghdr + struct rtgenmsg - final ByteBuffer buf = ByteBuffer.allocate(StructNlMsgHdr.STRUCT_SIZE + 1); - buf.order(ByteOrder.nativeOrder()); - - final StructNlMsgHdr nlmsghdr = new StructNlMsgHdr(); - nlmsghdr.nlmsg_len = buf.capacity(); - nlmsghdr.nlmsg_type = RTM_GETLINK; - nlmsghdr.nlmsg_flags = StructNlMsgHdr.NLM_F_DUMP | StructNlMsgHdr.NLM_F_REQUEST; - nlmsghdr.pack(buf); - - // struct rtgenmsg { - // unsigned char rtgen_family; - // } - buf.put((byte) OsConstants.AF_UNSPEC); - buf.flip(); - - mNetlinkMonitor.sendNetlinkMessage(buf); - } - private static class ListenerInfo { boolean canUseRestrictedNetworks = false; From 9a807952641729562bc0e4d5eb1533d68cfc5a87 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 29 May 2025 15:03:26 -0700 Subject: [PATCH 5/5] Pass EthernetPort into maybeTrackInterface Test: TH Flag: EXEMPT mainline Bug: 1000001 Change-Id: I5405d71d71a17eb6ee6ba4c7ccd309d49c503240 --- .../src/com/android/server/ethernet/EthernetTracker.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index cf77f4935f..6d3ca04692 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -230,7 +230,7 @@ public class EthernetTracker { final String ifname = port.getInterfaceName(); if (!mFactory.hasInterface(ifname) && !ifname.equals(mTetheringInterface)) { Log.i(TAG, "onInterfaceAdded: " + port); - maybeTrackInterface(ifname); + maybeTrackInterface(port); } Log.i(TAG, "interfaceLinkStateChanged: " + port + ", up: " + linkUp); updateInterfaceState(ifname, linkUp); @@ -770,14 +770,15 @@ public class EthernetTracker { mTetheredInterfaceWasAvailable = available; } - private void maybeTrackInterface(String iface) { + private void maybeTrackInterface(EthernetPort port) { + final String iface = port.getInterfaceName(); // If we don't already track this interface, and if this interface matches // our regex, start tracking it. if (mFactory.hasInterface(iface) || iface.equals(mTetheringInterface)) { - if (DBG) Log.w(TAG, "Ignoring already-tracked interface " + iface); + if (DBG) Log.w(TAG, "Ignoring already-tracked " + port); return; } - if (DBG) Log.i(TAG, "maybeTrackInterface: " + iface); + if (DBG) Log.i(TAG, "maybeTrackInterface: " + port); // Do not use an interface for tethering if it has configured NetworkCapabilities. if (mTetheringInterface == null && !mNetworkCapabilities.containsKey(iface)) {