From c8b197598fbf5a73822dff10bcc7de7923a95e21 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Tue, 28 May 2019 17:29:11 +0900 Subject: [PATCH 1/3] Proper handling of NetworkStack crash Instead of always crashing on userdebug builds, do the following on all builds: - If the device did not observe a NetworkStack crash in the last 6h crash the system server. This is to handle spurious crashes of the NetworkStack, so that the system can recover instead of staying without connectivity. - Otherwise, the device has had a recent crash. Notify the listeners (watchdog listener to be added) that something is seriously wrong, but do not crash to avoid bootlooping the device. This allows the watchdog to do its job, and avoids bricking the device in situations where the user could need to make emergency calls. Bug: 133725814 Test: Killed the network stack, observe reboot of framework. Test: Kill network stack again, observe nothing. Test: Wipe data, install new network stack, kill it twice: rollback. (with patch on top applied) (Clean CP from If1fa00bed769eb60ca4832609006bdf15ceddb80) Merged-In: Ic6b2bb13b488f46cd4b8d87caa78342f622181c3 Change-Id: Iad90c7a4e416257dfe63d215274866bb25cf3d85 --- .../java/android/net/NetworkStackClient.java | 129 ++++++++++++++++-- 1 file changed, 115 insertions(+), 14 deletions(-) diff --git a/services/net/java/android/net/NetworkStackClient.java b/services/net/java/android/net/NetworkStackClient.java index 6b5842ff90656..09c9b6d360a9d 100644 --- a/services/net/java/android/net/NetworkStackClient.java +++ b/services/net/java/android/net/NetworkStackClient.java @@ -26,22 +26,27 @@ import android.content.ComponentName; import android.content.Context; import android.content.Intent; import android.content.ServiceConnection; +import android.content.SharedPreferences; import android.content.pm.PackageManager; import android.net.dhcp.DhcpServingParamsParcel; import android.net.dhcp.IDhcpServerCallbacks; import android.net.ip.IIpClientCallbacks; import android.net.util.SharedLog; import android.os.Binder; -import android.os.Build; +import android.os.Environment; import android.os.IBinder; import android.os.Process; import android.os.RemoteException; import android.os.ServiceManager; +import android.os.SystemClock; import android.os.UserHandle; +import android.provider.DeviceConfig; +import android.util.ArraySet; import android.util.Slog; import com.android.internal.annotations.GuardedBy; +import java.io.File; import java.io.PrintWriter; import java.util.ArrayList; @@ -54,6 +59,15 @@ public class NetworkStackClient { private static final int NETWORKSTACK_TIMEOUT_MS = 10_000; private static final String IN_PROCESS_SUFFIX = ".InProcess"; + private static final String PREFS_FILE = "NetworkStackClientPrefs.xml"; + private static final String PREF_KEY_LAST_CRASH_UPTIME = "lastcrash"; + private static final String CONFIG_MIN_CRASH_INTERVAL_MS = "min_crash_interval"; + + // Even if the network stack is lost, do not crash the system more often than this. + // Connectivity would be broken, but if the user needs the device for something urgent + // (like calling emergency services) we should not bootloop the device. + // This is the default value: the actual value can be adjusted via device config. + private static final long DEFAULT_MIN_CRASH_INTERVAL_MS = 6 * 3_600_000L; private static NetworkStackClient sInstance; @@ -67,12 +81,34 @@ public class NetworkStackClient { @GuardedBy("mLog") private final SharedLog mLog = new SharedLog(TAG); - private volatile boolean mNetworkStackStartRequested = false; + private volatile boolean mWasSystemServerInitialized = false; + + /** + * If non-zero, indicates that the last framework start happened after a crash of the + * NetworkStack which was at the specified uptime. + */ + private volatile long mLastCrashUptime = 0L; + + @GuardedBy("mHealthListeners") + private final ArraySet mHealthListeners = new ArraySet<>(); private interface NetworkStackCallback { void onNetworkStackConnected(INetworkStackConnector connector); } + /** + * Callback interface for severe failures of the NetworkStack. + * + *

Useful for health monitors such as PackageWatchdog. + */ + public interface NetworkStackHealthListener { + /** + * Called when there is a severe failure of the network stack. + * @param packageName Package name of the network stack. + */ + void onNetworkStackFailure(@NonNull String packageName); + } + private NetworkStackClient() { } /** @@ -85,6 +121,15 @@ public class NetworkStackClient { return sInstance; } + /** + * Add a {@link NetworkStackHealthListener} to listen to network stack health events. + */ + public void registerHealthListener(@NonNull NetworkStackHealthListener listener) { + synchronized (mHealthListeners) { + mHealthListeners.add(listener); + } + } + /** * Create a DHCP server according to the specified parameters. * @@ -147,6 +192,16 @@ public class NetworkStackClient { } private class NetworkStackConnection implements ServiceConnection { + @NonNull + private final Context mContext; + @NonNull + private final String mPackageName; + + private NetworkStackConnection(@NonNull Context context, @NonNull String packageName) { + mContext = context; + mPackageName = packageName; + } + @Override public void onServiceConnected(ComponentName name, IBinder service) { logi("Network stack service connected"); @@ -155,14 +210,14 @@ public class NetworkStackClient { @Override public void onServiceDisconnected(ComponentName name) { - // The system has lost its network stack (probably due to a crash in the - // network stack process): better crash rather than stay in a bad state where all - // networking is broken. // onServiceDisconnected is not being called on device shutdown, so this method being // called always indicates a bad state for the system server. - maybeCrashWithTerribleFailure("Lost network stack"); + // This code path is only run by the system server: only the system server binds + // to the NetworkStack as a service. Other processes get the NetworkStack from + // the ServiceManager. + maybeCrashWithTerribleFailure("Lost network stack", mContext, mPackageName); } - }; + } private void registerNetworkStackService(@NonNull IBinder service) { final INetworkStackConnector connector = INetworkStackConnector.Stub.asInterface(service); @@ -189,7 +244,7 @@ public class NetworkStackClient { */ public void init() { log("Network stack init"); - mNetworkStackStartRequested = true; + mWasSystemServerInitialized = true; } /** @@ -202,6 +257,13 @@ public class NetworkStackClient { */ public void start(Context context) { log("Starting network stack"); + + final SharedPreferences prefs = getSharedPreferences(context); + mLastCrashUptime = prefs.getLong(PREF_KEY_LAST_CRASH_UPTIME, 0L); + // Remove the preference after getting the last crash uptime, so mLastCrashUptime always + // indicates this is the first start since the last crash. + prefs.edit().remove(PREF_KEY_LAST_CRASH_UPTIME).commit(); + final PackageManager pm = context.getPackageManager(); // Try to bind in-process if the device was shipped with an in-process version @@ -216,16 +278,19 @@ public class NetworkStackClient { } if (intent == null) { - maybeCrashWithTerribleFailure("Could not resolve the network stack"); + maybeCrashWithTerribleFailure("Could not resolve the network stack", context, null); return; } + final String packageName = intent.getComponent().getPackageName(); + // Start the network stack. The service will be added to the service manager in // NetworkStackConnection.onServiceConnected(). - if (!context.bindServiceAsUser(intent, new NetworkStackConnection(), + if (!context.bindServiceAsUser(intent, new NetworkStackConnection(context, packageName), Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT, UserHandle.SYSTEM)) { maybeCrashWithTerribleFailure( - "Could not bind to network stack in-process, or in app with " + intent); + "Could not bind to network stack in-process, or in app with " + intent, + context, packageName); return; } @@ -274,11 +339,47 @@ public class NetworkStackClient { } } - private void maybeCrashWithTerribleFailure(@NonNull String message) { + private void maybeCrashWithTerribleFailure(@NonNull String message, + @NonNull Context context, @Nullable String packageName) { logWtf(message, null); - if (Build.IS_DEBUGGABLE) { + // uptime is monotonic even after a framework restart + final long uptime = SystemClock.elapsedRealtime(); + final long minCrashIntervalMs = DeviceConfig.getLong(DeviceConfig.NAMESPACE_CONNECTIVITY, + CONFIG_MIN_CRASH_INTERVAL_MS, DEFAULT_MIN_CRASH_INTERVAL_MS); + + // Either the framework was not restarted after a crash of the NetworkStack, or the min + // crash interval has passed since then. + if (mLastCrashUptime == 0L || uptime - mLastCrashUptime > minCrashIntervalMs) { + // The system is not bound to its network stack (for example due to a crash in the + // network stack process): better crash rather than stay in a bad state where all + // networking is broken. + // Using device-encrypted SharedPreferences as DeviceConfig does not have a synchronous + // API to persist settings before a crash. + final SharedPreferences prefs = getSharedPreferences(context); + if (!prefs.edit().putLong(PREF_KEY_LAST_CRASH_UPTIME, uptime).commit()) { + logWtf("Could not persist last crash uptime", null); + } throw new IllegalStateException(message); } + + // Here the system crashed recently already. Inform listeners that something is + // definitely wrong. + if (packageName != null) { + final ArraySet listeners; + synchronized (mHealthListeners) { + listeners = new ArraySet<>(mHealthListeners); + } + for (NetworkStackHealthListener listener : listeners) { + listener.onNetworkStackFailure(packageName); + } + } + } + + private SharedPreferences getSharedPreferences(Context context) { + final File prefsFile = new File( + Environment.getDataSystemDeDirectory(UserHandle.USER_SYSTEM), PREFS_FILE); + return context.createDeviceProtectedStorageContext() + .getSharedPreferences(prefsFile, Context.MODE_PRIVATE); } /** @@ -350,7 +451,7 @@ public class NetworkStackClient { "Only the system server should try to bind to the network stack."); } - if (!mNetworkStackStartRequested) { + if (!mWasSystemServerInitialized) { // The network stack is not being started in this process, e.g. this process is not // the system server. Get a remote connector registered by the system server. final INetworkStackConnector connector = getRemoteConnector(); From cfde948e4f28a42f58ba361b45d0756e6e92a5fa Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Fri, 31 May 2019 15:02:35 +0900 Subject: [PATCH 2/3] Simplify crash conditions in NetworkStackClient The previous model could have impact on boot time to read/write from/to disk, and could potentially fail in some scenarios where the device does full reboots instead of framework restarts. The current design most simply avoids crashing in the first 30mins after a full reboot, and optimistically checks the wall clock to rate-limit the crashes to every 6h. Test: manual as below, without IS_DEBUGGABLE condition Test: Install new NetworkStack, force crash, observe rollback Test: Set min_uptime_before_crash to 100, force crash, observe crash Test: min_uptime_before_crash still 100, install new NetworkStack, force crash: observe rollback (there was already a recent crash) Test: Set min_crash_interval to 10, force crash: observe crash Bug: 133725814 (Clean CP from I3fd5ba7047d7ac991cb62a7cab16a40f4ee731a3) Merged-In: Ic6b2bb13b488f46cd4b8d87caa78342f622181c3 Change-Id: Ia3fdd80c85cde45452c8e9b877836dfa4204e83d --- .../java/android/net/NetworkStackClient.java | 94 ++++++++++++++----- 1 file changed, 68 insertions(+), 26 deletions(-) diff --git a/services/net/java/android/net/NetworkStackClient.java b/services/net/java/android/net/NetworkStackClient.java index 09c9b6d360a9d..99da637416c30 100644 --- a/services/net/java/android/net/NetworkStackClient.java +++ b/services/net/java/android/net/NetworkStackClient.java @@ -33,6 +33,7 @@ import android.net.dhcp.IDhcpServerCallbacks; import android.net.ip.IIpClientCallbacks; import android.net.util.SharedLog; import android.os.Binder; +import android.os.Build; import android.os.Environment; import android.os.IBinder; import android.os.Process; @@ -41,6 +42,7 @@ import android.os.ServiceManager; import android.os.SystemClock; import android.os.UserHandle; import android.provider.DeviceConfig; +import android.text.format.DateUtils; import android.util.ArraySet; import android.util.Slog; @@ -60,14 +62,22 @@ public class NetworkStackClient { private static final int NETWORKSTACK_TIMEOUT_MS = 10_000; private static final String IN_PROCESS_SUFFIX = ".InProcess"; private static final String PREFS_FILE = "NetworkStackClientPrefs.xml"; - private static final String PREF_KEY_LAST_CRASH_UPTIME = "lastcrash"; + private static final String PREF_KEY_LAST_CRASH_TIME = "lastcrash_time"; private static final String CONFIG_MIN_CRASH_INTERVAL_MS = "min_crash_interval"; + private static final String CONFIG_MIN_UPTIME_BEFORE_CRASH_MS = "min_uptime_before_crash"; + private static final String CONFIG_ALWAYS_RATELIMIT_NETWORKSTACK_CRASH = + "always_ratelimit_networkstack_crash"; // Even if the network stack is lost, do not crash the system more often than this. // Connectivity would be broken, but if the user needs the device for something urgent // (like calling emergency services) we should not bootloop the device. // This is the default value: the actual value can be adjusted via device config. - private static final long DEFAULT_MIN_CRASH_INTERVAL_MS = 6 * 3_600_000L; + private static final long DEFAULT_MIN_CRASH_INTERVAL_MS = 6 * DateUtils.HOUR_IN_MILLIS; + + // Even if the network stack is lost, do not crash the system server if it was less than + // this much after boot. This avoids bootlooping the device, and crashes should address very + // infrequent failures, not failures on boot. + private static final long DEFAULT_MIN_UPTIME_BEFORE_CRASH_MS = 30 * DateUtils.MINUTE_IN_MILLIS; private static NetworkStackClient sInstance; @@ -83,12 +93,6 @@ public class NetworkStackClient { private volatile boolean mWasSystemServerInitialized = false; - /** - * If non-zero, indicates that the last framework start happened after a crash of the - * NetworkStack which was at the specified uptime. - */ - private volatile long mLastCrashUptime = 0L; - @GuardedBy("mHealthListeners") private final ArraySet mHealthListeners = new ArraySet<>(); @@ -258,12 +262,6 @@ public class NetworkStackClient { public void start(Context context) { log("Starting network stack"); - final SharedPreferences prefs = getSharedPreferences(context); - mLastCrashUptime = prefs.getLong(PREF_KEY_LAST_CRASH_UPTIME, 0L); - // Remove the preference after getting the last crash uptime, so mLastCrashUptime always - // indicates this is the first start since the last crash. - prefs.edit().remove(PREF_KEY_LAST_CRASH_UPTIME).commit(); - final PackageManager pm = context.getPackageManager(); // Try to bind in-process if the device was shipped with an in-process version @@ -344,21 +342,40 @@ public class NetworkStackClient { logWtf(message, null); // uptime is monotonic even after a framework restart final long uptime = SystemClock.elapsedRealtime(); + final long now = System.currentTimeMillis(); final long minCrashIntervalMs = DeviceConfig.getLong(DeviceConfig.NAMESPACE_CONNECTIVITY, CONFIG_MIN_CRASH_INTERVAL_MS, DEFAULT_MIN_CRASH_INTERVAL_MS); + final long minUptimeBeforeCrash = DeviceConfig.getLong(DeviceConfig.NAMESPACE_CONNECTIVITY, + CONFIG_MIN_UPTIME_BEFORE_CRASH_MS, DEFAULT_MIN_UPTIME_BEFORE_CRASH_MS); + final boolean alwaysRatelimit = DeviceConfig.getBoolean(DeviceConfig.NAMESPACE_CONNECTIVITY, + CONFIG_ALWAYS_RATELIMIT_NETWORKSTACK_CRASH, false); - // Either the framework was not restarted after a crash of the NetworkStack, or the min - // crash interval has passed since then. - if (mLastCrashUptime == 0L || uptime - mLastCrashUptime > minCrashIntervalMs) { + final SharedPreferences prefs = getSharedPreferences(context); + final long lastCrashTime = tryGetLastCrashTime(prefs); + + // Only crash if there was enough time since boot, and (if known) enough time passed since + // the last crash. + // time and lastCrashTime may be unreliable if devices have incorrect clock time, but they + // are only used to limit the number of crashes compared to only using the time since boot, + // which would also be OK behavior by itself. + // - If lastCrashTime is incorrectly more than the current time, only look at uptime + // - If it is much less than current time, only look at uptime + // - If current time is during the next few hours after last crash time, don't crash. + // Considering that this only matters if last boot was some time ago, it's likely that + // time will be set correctly. Otherwise, not crashing is not a big problem anyway. Being + // in this last state would also not last for long since the window is only a few hours. + final boolean alwaysCrash = Build.IS_DEBUGGABLE && !alwaysRatelimit; + final boolean justBooted = uptime < minUptimeBeforeCrash; + final boolean haveLastCrashTime = (lastCrashTime != 0) && (lastCrashTime < now); + final boolean haveKnownRecentCrash = + haveLastCrashTime && (now < lastCrashTime + minCrashIntervalMs); + if (alwaysCrash || (!justBooted && !haveKnownRecentCrash)) { // The system is not bound to its network stack (for example due to a crash in the // network stack process): better crash rather than stay in a bad state where all // networking is broken. // Using device-encrypted SharedPreferences as DeviceConfig does not have a synchronous // API to persist settings before a crash. - final SharedPreferences prefs = getSharedPreferences(context); - if (!prefs.edit().putLong(PREF_KEY_LAST_CRASH_UPTIME, uptime).commit()) { - logWtf("Could not persist last crash uptime", null); - } + tryWriteLastCrashTime(prefs, now); throw new IllegalStateException(message); } @@ -375,11 +392,36 @@ public class NetworkStackClient { } } - private SharedPreferences getSharedPreferences(Context context) { - final File prefsFile = new File( - Environment.getDataSystemDeDirectory(UserHandle.USER_SYSTEM), PREFS_FILE); - return context.createDeviceProtectedStorageContext() - .getSharedPreferences(prefsFile, Context.MODE_PRIVATE); + @Nullable + private SharedPreferences getSharedPreferences(@NonNull Context context) { + try { + final File prefsFile = new File( + Environment.getDataSystemDeDirectory(UserHandle.USER_SYSTEM), PREFS_FILE); + return context.createDeviceProtectedStorageContext() + .getSharedPreferences(prefsFile, Context.MODE_PRIVATE); + } catch (Throwable e) { + logWtf("Error loading shared preferences", e); + return null; + } + } + + private long tryGetLastCrashTime(@Nullable SharedPreferences prefs) { + if (prefs == null) return 0L; + try { + return prefs.getLong(PREF_KEY_LAST_CRASH_TIME, 0L); + } catch (Throwable e) { + logWtf("Error getting last crash time", e); + return 0L; + } + } + + private void tryWriteLastCrashTime(@Nullable SharedPreferences prefs, long value) { + if (prefs == null) return; + try { + prefs.edit().putLong(PREF_KEY_LAST_CRASH_TIME, value).commit(); + } catch (Throwable e) { + logWtf("Error writing last crash time", e); + } } /** From f75f9d792544f7169c176d0e06ac0366720becd2 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Wed, 5 Jun 2019 14:31:59 +0900 Subject: [PATCH 3/3] Remove DeviceConfig usage from NetworkStackClient DeviceConfig API is not yet submitted. Use Settings.Global instead, to still allow writing tests against AOSP code (a test would try to use adb shell device_config, and fallback to adb shell settings). This is not merged anywhere else, the merged-in is here to ensure this does not end up in branches that use DeviceConfig. This change should be lost when AOSP is updated. Test: flashed, force-crashed NetworkStack with different setting values: observe rate-limited crash Bug: 133725814 Merged-In: I423ca6ebb328f49b170baae0da9b8409a6429fcb Change-Id: I399d3e37f1faaecb8a30428c1989fac8821379d8 --- .../java/android/net/NetworkStackClient.java | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/services/net/java/android/net/NetworkStackClient.java b/services/net/java/android/net/NetworkStackClient.java index 99da637416c30..cf52574e83e38 100644 --- a/services/net/java/android/net/NetworkStackClient.java +++ b/services/net/java/android/net/NetworkStackClient.java @@ -41,7 +41,7 @@ import android.os.RemoteException; import android.os.ServiceManager; import android.os.SystemClock; import android.os.UserHandle; -import android.provider.DeviceConfig; +import android.provider.Settings; import android.text.format.DateUtils; import android.util.ArraySet; import android.util.Slog; @@ -340,6 +340,8 @@ public class NetworkStackClient { private void maybeCrashWithTerribleFailure(@NonNull String message, @NonNull Context context, @Nullable String packageName) { logWtf(message, null); + // Called DeviceConfig to minimize merge conflicts + final DeviceConfigStub DeviceConfig = new DeviceConfigStub(context); // uptime is monotonic even after a framework restart final long uptime = SystemClock.elapsedRealtime(); final long now = System.currentTimeMillis(); @@ -531,4 +533,36 @@ public class NetworkStackClient { pw.println(); pw.println("pendingNetStackRequests length: " + requestsQueueLength); } + + /** + * Stub class to replicate DeviceConfig behavior with minimal merge conflicts. + */ + private class DeviceConfigStub { + private final Context mContext; + + // Namespace is actually unused, but is here to replicate the final API. + private static final String NAMESPACE_CONNECTIVITY = "connectivity"; + + private DeviceConfigStub(Context context) { + mContext = context; + } + + private long getLong( + @NonNull String namespace, @NonNull String key, long defaultVal) { + // Temporary solution until DeviceConfig is available + try { + return Settings.Global.getLong( + mContext.getContentResolver(), TAG + "_" + key, defaultVal); + } catch (Throwable e) { + logWtf("Could not obtain setting " + key, e); + return defaultVal; + } + } + + private boolean getBoolean( + @NonNull String namespace, @NonNull String key, boolean defaultVal) { + // Temporary solution until DeviceConfig is available + return getLong(namespace, key, defaultVal ? 1 : 0) != 0; + } + } }