From f360e458fc9b30c16437c3feacdfcd4fcde4d529 Mon Sep 17 00:00:00 2001
From: Patrick Rohr <prohr@google.com>
Date: Mon, 15 Aug 2022 20:37:30 -0700
Subject: [PATCH] ethernet: read running flag before bringing up interface

Previously, the "running" flag was checked right after calling
setInterfaceUp(), which caused an updateInterfaceState(UP) event to be
generated at the end of addInterface(). This "artificial" link up
notification is only needed for interfaces that already had link before
being added (as in this case, no RTM_NEWLINK callbacks are received, and
link state needs to "manually" be set to UP to start configuring the
interface). This could happen when system server crashes and gets
restarted, or when ethernet is temporarily disabled via the
EthernetManager API.

Reading the running flag before bringing up the interface ensures the
updateInterfaceState(UP) event is not generated for interfaces that were
just brought up.
This fix gets rid of the up/up/down/up callback when a new interface is
added.

Test: atest EthernetManagerTest
Change-Id: I122dc886a94222c575f44182b943e64b435c03e8
---
 .../com/android/server/ethernet/EthernetTracker.java   | 10 ++++++++--
 .../cts/net/src/android/net/cts/EthernetManagerTest.kt |  4 +---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java
index 77addcf6ff..00dff5b1d0 100644
--- a/service-t/src/com/android/server/ethernet/EthernetTracker.java
+++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java
@@ -578,8 +578,14 @@ public class EthernetTracker {
         // Bring up the interface so we get link status indications.
         try {
             PermissionUtils.enforceNetworkStackPermission(mContext);
-            NetdUtils.setInterfaceUp(mNetd, iface);
+            // Read the flags before attempting to bring up the interface. If the interface is
+            // already running an UP event is created after adding the interface.
             config = NetdUtils.getInterfaceConfigParcel(mNetd, iface);
+            if (NetdUtils.hasFlag(config, INetd.IF_STATE_DOWN)) {
+                // As a side-effect, NetdUtils#setInterfaceUp() also clears the interface's IPv4
+                // address and readds it which *could* lead to unexpected behavior in the future.
+                NetdUtils.setInterfaceUp(mNetd, iface);
+            }
         } catch (IllegalStateException e) {
             // Either the system is crashing or the interface has disappeared. Just ignore the
             // error; we haven't modified any state because we only do that if our calls succeed.
@@ -615,7 +621,7 @@ public class EthernetTracker {
         // Note: if the interface already has link (e.g., if we crashed and got
         // restarted while it was running), we need to fake a link up notification so we
         // start configuring it.
-        if (NetdUtils.hasFlag(config, "running")) {
+        if (NetdUtils.hasFlag(config, INetd.IF_FLAG_RUNNING)) {
             updateInterfaceState(iface, true);
         }
     }
diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt
index 57df3eb5b2..1c92e4e4a2 100644
--- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt
+++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt
@@ -497,9 +497,7 @@ class EthernetManagerTest {
 
         // If an interface appears, existing callbacks see it.
         val iface2 = createInterface()
-        // TODO: fix the up/up/down/up callbacks and only send down/up. Change to expectCallback
-        // once that is fixed.
-        listener1.eventuallyExpect(iface2, STATE_LINK_DOWN, ROLE_CLIENT)
+        listener1.expectCallback(iface2, STATE_LINK_DOWN, ROLE_CLIENT)
         listener1.expectCallback(iface2, STATE_LINK_UP, ROLE_CLIENT)
 
         // Register a new listener, it should see state of all existing interfaces immediately.