LineageOS/android_packages_modules_Connectivity
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Change Nat464Xlat lifecycle.

Browse Source 
Currently, NetworkAgentInfo keeps a Nat464Xlat object only on
networks where we're starting clatd (i.e., IPv6-only networks).
Simplify this code by making the Nat464Xlat object final and
always non-null. This allows us to use Nat464Xlat to store
information, such as the NAT64 prefix, even if the clat daemon
has not been started yet.

Also, remove the STOPPING state which waits for the interface to
be removed. Instead, when stop() is called, immediately enter the
IDLE state.
- This is necessary in order to be able to call start() again
  before the interface removal notification has arrived.
- It's also arguably more correct than the current code,
  because when stop() returns clatd has already terminated
  (ClatdController::stopClatd calls waitpid), and thus the tun
  fd is already closed and the tun interface is gone.

Also, now that Nat464Xlat objects are reused after stop(), add
test coverage for calling start() after stop, in both cases:
- The notification that the previous interface was removed
  arrives before the second start().
- The notification that the previous interface was removed
  arrives after the second start() but before the notification
  that the second interface was added.

Also fix a couple of lint warnings.

Test: builds, boots
Test: atest FrameworksNetTests
Test: clatd stops when IPv4 address added
Test: clatd restarts after "adb shell killall clatd"
Change-Id: I3dc66d155aa27606681f3473daf2170434d8c6d0
This commit is contained in:
Lorenzo Colitti
2019-01-08 14:43:37 +09:00
parent c11592ce7c
commit 7b0732fff7
4 changed files with 143 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
services/core/java/com/android/server/ConnectivityService.java
View File

@@ -2877,7 +2877,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
e.rethrowFromSystemServer();
}
mNetworkAgentInfos.remove(nai.messenger);
nai.maybeStopClat();
nai.updateClat();
synchronized (mNetworkForNetId) {
// Remove the NetworkAgent, but don't mark the netId as
// available until we've told netd to delete it below.
@@ -5185,11 +5185,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
LinkProperties oldLp) {
int netId = networkAgent.network.netId;
// The NetworkAgentInfo does not know whether clatd is running on its network or not. Before
// we do anything else, make sure its LinkProperties are accurate.
if (networkAgent.clatd != null) {
// The NetworkAgentInfo does not know whether clatd is running on its network or not, or
// whether there is a NAT64 prefix. Before we do anything else, make sure its LinkProperties
// are accurate.
networkAgent.clatd.fixupLinkProperties(oldLp, newLp);
}
updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities);
updateMtu(newLp, oldLp);
@@ -5220,7 +5219,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
networkAgent.linkProperties = newLp;
}
// Start or stop clat accordingly to network state.
networkAgent.updateClat(mNMS);
networkAgent.updateClat();
notifyIfacesChangedForNetworkStats();
if (networkAgent.everConnected) {
try {

66
services/core/java/com/android/server/connectivity/Nat464Xlat.java
View File

@@ -70,8 +70,6 @@ public class Nat464Xlat extends BaseNetworkObserver {
IDLE, // start() not called. Base iface and stacked iface names are null.
STARTING, // start() called. Base iface and stacked iface names are known.
RUNNING, // start() called, and the stacked iface is known to be up.
STOPPING; // stop() called, this Nat464Xlat is still registered as a network observer for
// the stacked interface.
}
private String mBaseIface;
@@ -123,13 +121,6 @@ public class Nat464Xlat extends BaseNetworkObserver {
return mState == State.RUNNING;
}
/**
* @return true if clatd has been stopped.
*/
public boolean isStopping() {
return mState == State.STOPPING;
}
/**
* Start clatd, register this Nat464Xlat as a network observer for the stacked interface,
* and set internal state.
@@ -138,8 +129,7 @@ public class Nat464Xlat extends BaseNetworkObserver {
try {
mNMService.registerObserver(this);
} catch (RemoteException e) {
Slog.e(TAG,
"startClat: Can't register interface observer for clat on " + mNetwork.name());
Slog.e(TAG, "Can't register interface observer for clat on " + mNetwork.name());
return;
}
try {
@@ -160,19 +150,6 @@ public class Nat464Xlat extends BaseNetworkObserver {
mState = State.RUNNING;
}
/**
* Stop clatd, and turn ND offload on if it had been turned off.
*/
private void enterStoppingState() {
try {
mNetd.clatdStop(mBaseIface);
} catch(RemoteException|IllegalStateException e) {
Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e);
}
mState = State.STOPPING;
}
/**
* Unregister as a base observer for the stacked interface, and clear internal state.
*/
@@ -182,7 +159,6 @@ public class Nat464Xlat extends BaseNetworkObserver {
} catch (RemoteException | IllegalStateException e) {
Slog.e(TAG, "Error unregistering clatd observer on " + mBaseIface, e);
}
mIface = null;
mBaseIface = null;
mState = State.IDLE;
@@ -217,14 +193,30 @@ public class Nat464Xlat extends BaseNetworkObserver {
*/
public void stop() {
if (!isStarted()) {
Slog.e(TAG, "stopClat: already stopped");
return;
}
Slog.i(TAG, "Stopping clatd on " + mBaseIface);
boolean wasStarting = isStarting();
enterStoppingState();
if (wasStarting) {
Slog.i(TAG, "Stopping clatd on " + mBaseIface);
try {
mNetd.clatdStop(mBaseIface);
} catch (RemoteException | IllegalStateException e) {
Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e);
}
String iface = mIface;
boolean wasRunning = isRunning();
// Enter IDLE state before updating LinkProperties. handleUpdateLinkProperties ends up
// calling fixupLinkProperties, and if at that time the state is still RUNNING,
// fixupLinkProperties would wrongly inform ConnectivityService that there is still a
// stacked interface.
enterIdleState();
if (wasRunning) {
LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
lp.removeStackedLink(iface);
mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp);
}
}
@@ -307,20 +299,16 @@ public class Nat464Xlat extends BaseNetworkObserver {
if (!Objects.equals(mIface, iface)) {
return;
}
if (!isRunning() && !isStopping()) {
if (!isRunning()) {
return;
}
Slog.i(TAG, "interface " + iface + " removed");
if (!isStopping()) {
// Ensure clatd is stopped if stop() has not been called: this likely means that clatd
// has crashed.
enterStoppingState();
}
enterIdleState();
LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
lp.removeStackedLink(iface);
mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp);
// If we're running, and the interface was removed, then we didn't call stop(), and it's
// likely that clatd crashed. Ensure we call stop() so we can start clatd again. Calling
// stop() will also update LinkProperties, and if clatd crashed, the LinkProperties update
// will cause ConnectivityService to call start() again.
stop();
}
@Override

34
services/core/java/com/android/server/connectivity/NetworkAgentInfo.java
View File

@@ -236,7 +236,7 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo> {
public final AsyncChannel asyncChannel;
// Used by ConnectivityService to keep track of 464xlat.
public Nat464Xlat clatd;
public final Nat464Xlat clatd;
// Set after asynchronous creation of the NetworkMonitor.
private volatile INetworkMonitor mNetworkMonitor;
@@ -244,8 +244,6 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo> {
private static final String TAG = ConnectivityService.class.getSimpleName();
private static final boolean VDBG = false;
private final ConnectivityService mConnService;
private final INetd mNetd;
private final INetworkManagementService mNMS;
private final Context mContext;
private final Handler mHandler;
@@ -260,9 +258,8 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo> {
linkProperties = lp;
networkCapabilities = nc;
currentScore = score;
clatd = new Nat464Xlat(this, netd, nms);
mConnService = connService;
mNetd = netd;
mNMS = nms;
mContext = context;
mHandler = handler;
networkMisc = misc;
@@ -595,30 +592,13 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo> {
for (LingerTimer timer : mLingerTimers) { pw.println(timer); }
}
public void updateClat(INetworkManagementService netd) {
if (Nat464Xlat.requiresClat(this)) {
maybeStartClat();
} else {
maybeStopClat();
}
}
/** Ensure clat has started for this network. */
public void maybeStartClat() {
if (clatd != null && clatd.isStarted()) {
return;
}
clatd = new Nat464Xlat(this, mNetd, mNMS);
/** Starts or stops clatd as required. */
public void updateClat() {
if (!clatd.isStarted() && Nat464Xlat.requiresClat(this)) {
clatd.start();
}
/** Ensure clat has stopped for this network. */
public void maybeStopClat() {
if (clatd == null) {
return;
}
} else if (clatd.isStarted() && !Nat464Xlat.requiresClat(this)) {
clatd.stop();
clatd = null;
}
}
public String toString() {

106
tests/net/java/com/android/server/connectivity/Nat464XlatTest.java
View File

@@ -19,6 +19,7 @@ package com.android.server.connectivity;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -43,6 +44,7 @@ import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.InOrder;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
@@ -145,20 +147,106 @@ public class Nat464XlatTest {
nat.stop();
verify(mNetd).clatdStop(eq(BASE_IFACE));
// Stacked interface removed notification arrives.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
verify(mNms).unregisterObserver(eq(nat));
verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
verify(mNms).unregisterObserver(eq(nat));
assertTrue(c.getValue().getStackedLinks().isEmpty());
assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertIdle(nat);
// Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
verifyNoMoreInteractions(mNetd, mNms, mConnectivity);
}
private void checkStartStopStart(boolean interfaceRemovedFirst) throws Exception {
Nat464Xlat nat = makeNat464Xlat();
ArgumentCaptor<LinkProperties> c = ArgumentCaptor.forClass(LinkProperties.class);
InOrder inOrder = inOrder(mNetd, mConnectivity);
// ConnectivityService starts clat.
nat.start();
inOrder.verify(mNetd).clatdStart(eq(BASE_IFACE));
// Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty());
assertTrue(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertRunning(nat);
// ConnectivityService stops clat (Network disconnects, IPv4 addr appears, ...).
nat.stop();
inOrder.verify(mNetd).clatdStop(eq(BASE_IFACE));
inOrder.verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
assertTrue(c.getValue().getStackedLinks().isEmpty());
assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertIdle(nat);
if (interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
nat.interfaceLinkStateChanged(STACKED_IFACE, false);
mLooper.dispatchNext();
}
assertTrue(c.getValue().getStackedLinks().isEmpty());
assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertIdle(nat);
inOrder.verifyNoMoreInteractions();
// ConnectivityService starts clatd again.
nat.start();
inOrder.verify(mNetd).clatdStart(eq(BASE_IFACE));
if (!interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
nat.interfaceLinkStateChanged(STACKED_IFACE, false);
mLooper.dispatchNext();
}
// Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty());
assertTrue(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertRunning(nat);
// ConnectivityService stops clat again.
nat.stop();
inOrder.verify(mNetd).clatdStop(eq(BASE_IFACE));
inOrder.verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
assertTrue(c.getValue().getStackedLinks().isEmpty());
assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertIdle(nat);
inOrder.verifyNoMoreInteractions();
}
@Test
public void testStartStopStart() throws Exception {
checkStartStopStart(true);
}
@Test
public void testStartStopStartBeforeInterfaceRemoved() throws Exception {
checkStartStopStart(false);
}
@Test
public void testClatdCrashWhileRunning() throws Exception {
Nat464Xlat nat = makeNat464Xlat();
@@ -184,9 +272,9 @@ public class Nat464XlatTest {
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
verify(mNms).unregisterObserver(eq(nat));
verify(mNetd).clatdStop(eq(BASE_IFACE));
verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
verify(mNms).unregisterObserver(eq(nat));
assertTrue(c.getValue().getStackedLinks().isEmpty());
assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertIdle(nat);
@@ -210,8 +298,8 @@ public class Nat464XlatTest {
// ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...)
nat.stop();
verify(mNms).unregisterObserver(eq(nat));
verify(mNetd).clatdStop(eq(BASE_IFACE));
verify(mNms).unregisterObserver(eq(nat));
assertIdle(nat);
// In-flight interface up notification arrives: no-op
@@ -241,8 +329,8 @@ public class Nat464XlatTest {
// ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...)
nat.stop();
verify(mNms).unregisterObserver(eq(nat));
verify(mNetd).clatdStop(eq(BASE_IFACE));
verify(mNms).unregisterObserver(eq(nat));
assertIdle(nat);
verifyNoMoreInteractions(mNetd, mNms, mConnectivity);