From eb11304b272ab5294e01b74f5eafef2d691cef27 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 19 Jul 2017 00:23:44 +0900 Subject: [PATCH] Minor fixes for netd restarts and StrictController. 1. Ensure we don't change strict mode network policy for a given UID from a non-accept policy to another non-accept policy, as netd does not support this. 2. Move the "strict enable" and "bandwidth enable" commands inside the lock. This improves correctness, and it is safe to do now that those commands now only take a few milliseconds, instead of several hundred milliseconds. 3. Fix an NPE in connectNativeNetdService which causes the system to crash when netd crashes. (cherry picked from commit 8c253ade20827e16720c4a0d58cddfe72350e677) Bug: 28362720 Test: bullhead builds, boots Test: "adb shell killall netd" no longer crashes the system Change-Id: Iac72cf0011a7d3be4f4b1a5f21dd8cfb519e2b89 Merged-In: I2ce3b63e4cc9bf0416f02f550f44cf188980af50 --- .../server/NetworkManagementService.java | 121 ++++++++++-------- 1 file changed, 66 insertions(+), 55 deletions(-) diff --git a/services/core/java/com/android/server/NetworkManagementService.java b/services/core/java/com/android/server/NetworkManagementService.java index 96275aba5a174..a678cb3681868 100644 --- a/services/core/java/com/android/server/NetworkManagementService.java +++ b/services/core/java/com/android/server/NetworkManagementService.java @@ -67,6 +67,7 @@ import android.net.NetworkStats; import android.net.NetworkUtils; import android.net.RouteInfo; import android.net.UidRange; +import android.net.util.NetdService; import android.net.wifi.WifiConfiguration; import android.net.wifi.WifiConfiguration.KeyMgmt; import android.os.BatteryStats; @@ -362,7 +363,9 @@ public class NetworkManagementService extends INetworkManagementService.Stub if (DBG) Slog.d(TAG, "Awaiting socket connection"); connectedSignal.await(); if (DBG) Slog.d(TAG, "Connected"); + if (DBG) Slog.d(TAG, "Connecting native netd service"); service.connectNativeNetdService(); + if (DBG) Slog.d(TAG, "Connected"); return service; } @@ -586,14 +589,7 @@ public class NetworkManagementService extends INetworkManagementService.Stub } private void connectNativeNetdService() { - boolean nativeServiceAvailable = false; - try { - mNetdService = INetd.Stub.asInterface(ServiceManager.getService(NETD_SERVICE_NAME)); - nativeServiceAvailable = mNetdService.isAlive(); - } catch (RemoteException e) {} - if (!nativeServiceAvailable) { - Slog.wtf(TAG, "Can't connect to NativeNetdService " + NETD_SERVICE_NAME); - } + mNetdService = NetdService.get(); } /** @@ -606,37 +602,31 @@ public class NetworkManagementService extends INetworkManagementService.Stub // only enable bandwidth control when support exists final boolean hasKernelSupport = new File("/proc/net/xt_qtaguid/ctrl").exists(); - if (hasKernelSupport) { - Slog.d(TAG, "enabling bandwidth control"); - try { - mConnector.execute("bandwidth", "enable"); - mBandwidthControlEnabled = true; - } catch (NativeDaemonConnectorException e) { - Log.wtf(TAG, "problem enabling bandwidth controls", e); - } - } else { - Slog.i(TAG, "not enabling bandwidth control"); - } - - SystemProperties.set(PROP_QTAGUID_ENABLED, mBandwidthControlEnabled ? "1" : "0"); - - if (mBandwidthControlEnabled) { - try { - getBatteryStats().noteNetworkStatsEnabled(); - } catch (RemoteException e) { - } - } - - try { - mConnector.execute("strict", "enable"); - mStrictEnabled = true; - } catch (NativeDaemonConnectorException e) { - Log.wtf(TAG, "Failed strict enable", e); - } // push any existing quota or UID rules synchronized (mQuotaLock) { + if (hasKernelSupport) { + Slog.d(TAG, "enabling bandwidth control"); + try { + mConnector.execute("bandwidth", "enable"); + mBandwidthControlEnabled = true; + } catch (NativeDaemonConnectorException e) { + Log.wtf(TAG, "problem enabling bandwidth controls", e); + } + } else { + Slog.i(TAG, "not enabling bandwidth control"); + } + + SystemProperties.set(PROP_QTAGUID_ENABLED, mBandwidthControlEnabled ? "1" : "0"); + + try { + mConnector.execute("strict", "enable"); + mStrictEnabled = true; + } catch (NativeDaemonConnectorException e) { + Log.wtf(TAG, "Failed strict enable", e); + } + setDataSaverModeEnabled(mDataSaverMode); int size = mActiveQuotas.size(); @@ -714,6 +704,14 @@ public class NetworkManagementService extends INetworkManagementService.Stub } } } + + if (mBandwidthControlEnabled) { + try { + getBatteryStats().noteNetworkStatsEnabled(); + } catch (RemoteException e) { + } + } + } /** @@ -1792,6 +1790,30 @@ public class NetworkManagementService extends INetworkManagementService.Stub } } + private void applyUidCleartextNetworkPolicy(int uid, int policy) { + final String policyString; + switch (policy) { + case StrictMode.NETWORK_POLICY_ACCEPT: + policyString = "accept"; + break; + case StrictMode.NETWORK_POLICY_LOG: + policyString = "log"; + break; + case StrictMode.NETWORK_POLICY_REJECT: + policyString = "reject"; + break; + default: + throw new IllegalArgumentException("Unknown policy " + policy); + } + + try { + mConnector.execute("strict", "set_uid_cleartext_policy", uid, policyString); + mUidCleartextPolicy.put(uid, policy); + } catch (NativeDaemonConnectorException e) { + throw e.rethrowAsParcelableException(); + } + } + @Override public void setUidCleartextNetworkPolicy(int uid, int policy) { if (Binder.getCallingUid() != uid) { @@ -1801,6 +1823,8 @@ public class NetworkManagementService extends INetworkManagementService.Stub synchronized (mQuotaLock) { final int oldPolicy = mUidCleartextPolicy.get(uid, StrictMode.NETWORK_POLICY_ACCEPT); if (oldPolicy == policy) { + // This also ensures we won't needlessly apply an ACCEPT policy if we've just + // enabled strict and the underlying iptables rules are empty. return; } @@ -1811,28 +1835,15 @@ public class NetworkManagementService extends INetworkManagementService.Stub return; } - final String policyString; - switch (policy) { - case StrictMode.NETWORK_POLICY_ACCEPT: - policyString = "accept"; - break; - case StrictMode.NETWORK_POLICY_LOG: - policyString = "log"; - break; - case StrictMode.NETWORK_POLICY_REJECT: - policyString = "reject"; - break; - default: - throw new IllegalArgumentException("Unknown policy " + policy); - } - - try { - mConnector.execute("strict", "set_uid_cleartext_policy", uid, policyString); - mUidCleartextPolicy.put(uid, policy); - } catch (NativeDaemonConnectorException e) { - throw e.rethrowAsParcelableException(); + // netd does not keep state on strict mode policies, and cannot replace a non-accept + // policy without deleting it first. Rather than add state to netd, just always send + // it an accept policy when switching between two non-accept policies. + if (oldPolicy != StrictMode.NETWORK_POLICY_ACCEPT && + policy != StrictMode.NETWORK_POLICY_ACCEPT) { + applyUidCleartextNetworkPolicy(uid, policy); } } + applyUidCleartextNetworkPolicy(uid, policy); } @Override