Nat464Xlat: correct racefree teardown
This patch relays the NetworkBaseObserver notifications about nat
464xlat stacked interfaces onto the ConnectivityService handler.
This allows to process interface up and down notifications in the
same thread context and eliminates several races:
- NPE risk due to race between fixupLinkProperties called on
ConnectivityService thread and interfaceRemoved called on
NetworkManagementService thread.
- stale LinkProperties pointer reads in both NetworkBaseObserver
callbacks not called on ConnectivityService handler.
- removes the race between stop() and interfaceRemoved().
- removes superfluous LinkProperties notifications when stop() is
called before the stacked interface goes up.
The teardown procedure logic common to stop() and interfaceRemoved() is
put into enterStoppedState() and enterIdleState().
This allows to distinguish and correctly handle the following teardown
scenarios:
- an IPv4 appears -> ConnectivityService calls Nat464Xlat#stop()
-> Nat464Xlat calls stopClatd
-> clatd stops
-> if the stacked interface was up, it is removed
-> Nat464Xlat#interfaceRemoved() is triggered and
a LinkProperties update is sent.
- network disconnects -> ConnectivityService calls Nat464Xlat#stop()
-> Nat464Xlat calls stopClatd
-> clatd stops
-> if the stacked interface was up, it is removed
-> Nat464Xlat#interfaceRemoved() is triggered and
a LinkProperties update is sent.
- clatd crashes or exit -> Nat464Xlat#interfaceRemoved() is triggered
-> Nat464Xlat unregisters itself as a network
observer
-> ConnectivityService is updated about the
stacked interface missing, and restarts
Nat464Xlat if needed.
Note that the first two scenarios have two cases: stop() can be called
before the notification for the stacked interface going up (STARTED), or
after (RUNNING). In the first case, Nat464Xlat must unregister
immediately as a network observer to avoid leaks.
This patch also:
- removes/simplifies comments related to the threading model which
are no obsolete.
- extract clatd management logic from ConnectivityService into
NetworkAgentInfo
- add new unit tests where there was none before.
Bug: 62918393
Bug: 62997041
Bug: 64571917
Bug: 65225023
Test: runtest frameworks-net
Change-Id: I27221a8a60fd9760b567ed322cc79228df877e56
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index aa878ef..874303d 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -2023,16 +2023,7 @@
break;
}
case NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED: {
- if (VDBG) {
- log("Update of LinkProperties for " + nai.name() +
- "; created=" + nai.created +
- "; everConnected=" + nai.everConnected);
- }
- LinkProperties oldLp = nai.linkProperties;
- synchronized (nai) {
- nai.linkProperties = (LinkProperties)msg.obj;
- }
- if (nai.everConnected) updateLinkProperties(nai, oldLp);
+ handleUpdateLinkProperties(nai, (LinkProperties) msg.obj);
break;
}
case NetworkAgent.EVENT_NETWORK_INFO_CHANGED: {
@@ -2281,7 +2272,7 @@
}
nai.networkMonitor.sendMessage(NetworkMonitor.CMD_NETWORK_DISCONNECTED);
mNetworkAgentInfos.remove(msg.replyTo);
- maybeStopClat(nai);
+ nai.maybeStopClat();
synchronized (mNetworkForNetId) {
// Remove the NetworkAgent, but don't mark the netId as
// available until we've told netd to delete it below.
@@ -4375,7 +4366,7 @@
updateDnses(newLp, oldLp, netId);
// Start or stop clat accordingly to network state.
- updateClat(networkAgent);
+ networkAgent.updateClat(mNetd);
if (isDefaultNetwork(networkAgent)) {
handleApplyDefaultProxy(newLp.getHttpProxy());
} else {
@@ -4390,32 +4381,6 @@
mKeepaliveTracker.handleCheckKeepalivesStillValid(networkAgent);
}
- private void updateClat(NetworkAgentInfo nai) {
- if (Nat464Xlat.requiresClat(nai)) {
- maybeStartClat(nai);
- } else {
- maybeStopClat(nai);
- }
- }
-
- /** Ensure clat has started for this network. */
- private void maybeStartClat(NetworkAgentInfo nai) {
- if (nai.clatd != null && nai.clatd.isStarted()) {
- return;
- }
- nai.clatd = new Nat464Xlat(mNetd, mTrackerHandler, nai);
- nai.clatd.start();
- }
-
- /** Ensure clat has stopped for this network. */
- private void maybeStopClat(NetworkAgentInfo nai) {
- if (nai.clatd == null) {
- return;
- }
- nai.clatd.stop();
- nai.clatd = null;
- }
-
private void wakeupModifyInterface(String iface, NetworkCapabilities caps, boolean add) {
// Marks are only available on WiFi interaces. Checking for
// marks on unsupported interfaces is harmless.
@@ -4650,6 +4615,26 @@
}
}
+ public void handleUpdateLinkProperties(NetworkAgentInfo nai, LinkProperties newLp) {
+ if (mNetworkForNetId.get(nai.network.netId) != nai) {
+ // Ignore updates for disconnected networks
+ return;
+ }
+
+ if (VDBG) {
+ log("Update of LinkProperties for " + nai.name() +
+ "; created=" + nai.created +
+ "; everConnected=" + nai.everConnected);
+ }
+ LinkProperties oldLp = nai.linkProperties;
+ synchronized (nai) {
+ nai.linkProperties = newLp;
+ }
+ if (nai.everConnected) {
+ updateLinkProperties(nai, oldLp);
+ }
+ }
+
private void sendUpdatedScoreToFactories(NetworkAgentInfo nai) {
for (int i = 0; i < nai.numNetworkRequests(); i++) {
NetworkRequest nr = nai.requestAt(i);