Rework the determination of a "valid network".
+ isNetIdValid() doesn't make much sense. What we want is whether the netId has
actually been created (via createNetwork()).
+ It isn't an error to call deleteNetwork() or setDefaultNetwork() even when
there are no interfaces assigned to the network.
+ Secure all accesses to the maps in PermissionsController with locks; they are
called from many threads (CommandListener, DnsProxyListener and FwmarkServer).
+ Remove the redundant mIfaceNetidMap.
+ Minor cosmetic changes to things such as #includes and log messages.
Change-Id: Ieb154589b24f00ba8067eaaec4def3534aec4923
diff --git a/server/CommandListener.cpp b/server/CommandListener.cpp
index 5455a8e..fbf5670 100644
--- a/server/CommandListener.cpp
+++ b/server/CommandListener.cpp
@@ -1594,9 +1594,6 @@
}
// strtoul() returns 0 on errors, which is fine because 0 is an invalid netId.
unsigned netId = strtoul(argv[2], NULL, 0);
- if (!sNetCtrl->isNetIdValid(netId)) {
- return paramError(client, "Invalid netId");
- }
int nextArg = 3;
Permission permission = parseMultiplePermissions(argc, argv, &nextArg);
if (nextArg != argc) {
@@ -1616,9 +1613,6 @@
}
// strtoul() returns 0 on errors, which is fine because 0 is an invalid netId.
unsigned netId = strtoul(argv[2], NULL, 0);
- if (!sNetCtrl->isNetIdValid(netId)) {
- return paramError(client, "Invalid netId");
- }
if (!sNetCtrl->destroyNetwork(netId)) {
return operationError(client, "destroyNetwork() failed");
}
@@ -1634,9 +1628,6 @@
}
// strtoul() returns 0 on errors, which is fine because 0 is an invalid netId.
unsigned netId = strtoul(argv[2], NULL, 0);
- if (!sNetCtrl->isNetIdValid(netId)) {
- return paramError(client, "Invalid netId");
- }
if (!strcmp(argv[1], "addiface")) {
if (!sNetCtrl->addInterfaceToNetwork(netId, argv[3])) {
return operationError(client, "addInterfaceToNetwork() failed");
@@ -1705,9 +1696,6 @@
}
// strtoul() returns 0 on errors, which is fine because 0 is an invalid netId.
netId = strtoul(argv[3], NULL, 0);
- if (!sNetCtrl->isNetIdValid(netId)) {
- return paramError(client, "Invalid netId");
- }
} else if (strcmp(argv[2], "clear")) {
return syntaxError(client, "Unknown argument");
}
@@ -1726,9 +1714,6 @@
}
// strtoul() returns 0 on errors, which is fine because 0 is an invalid netId.
unsigned netId = strtoul(argv[3], NULL, 0);
- if (!sNetCtrl->isNetIdValid(netId)) {
- return paramError(client, "Invalid netId");
- }
const char* interface = argv[4];
const char* destination = argv[5];
const char* nexthop = NULL;
diff --git a/server/NetworkController.cpp b/server/NetworkController.cpp
index a11fae8..9504dc0 100644
--- a/server/NetworkController.cpp
+++ b/server/NetworkController.cpp
@@ -14,16 +14,31 @@
* limitations under the License.
*/
-#include <resolv_netid.h>
-
-#define LOG_TAG "NetworkController"
-#include <cutils/log.h>
+// THREAD-SAFETY
+// -------------
+// The methods in this file are called from multiple threads (from CommandListener, FwmarkServer
+// and DnsProxyListener). So, all accesses to shared state are guarded by a lock.
+//
+// In some cases, a single non-const method acquires and releases the lock several times, like so:
+// if (isValidNetwork(...)) { // isValidNetwork() acquires and releases the lock.
+// setDefaultNetwork(...); // setDefaultNetwork() also acquires and releases the lock.
+//
+// It might seem that this allows races where the state changes between the two statements, but in
+// fact there are no races because:
+// 1. This pattern only occurs in non-const methods (i.e., those that mutate state).
+// 2. Only CommandListener calls these non-const methods. The others call only const methods.
+// 3. CommandListener only processes one command at a time. I.e., it's serialized.
+// Thus, no other mutation can occur in between the two statements above.
#include "NetworkController.h"
#include "PermissionsController.h"
#include "RouteController.h"
+#define LOG_TAG "NetworkController"
+#include "cutils/log.h"
+#include "resolv_netid.h"
+
namespace {
// Keep these in sync with ConnectivityService.java.
@@ -32,10 +47,6 @@
} // namespace
-bool NetworkController::isNetIdValid(unsigned netId) {
- return MIN_NET_ID <= netId && netId <= MAX_NET_ID;
-}
-
NetworkController::NetworkController(PermissionsController* permissionsController,
RouteController* routeController)
: mDefaultNetId(NETID_UNSET),
@@ -54,6 +65,13 @@
}
bool NetworkController::setDefaultNetwork(unsigned newNetId) {
+ // newNetId must be either NETID_UNSET or a valid network. If it's NETID_UNSET, the caller is
+ // asking for there to be no default network, which is a request we support.
+ if (newNetId != NETID_UNSET && !isValidNetwork(newNetId)) {
+ ALOGE("invalid netId %u", newNetId);
+ return false;
+ }
+
unsigned oldNetId;
{
android::RWLock::AutoWLock lock(mRWLock);
@@ -66,26 +84,27 @@
}
bool status = true;
+ Permission permission;
+ InterfaceRange range;
// Add default network rules for the new netId.
- if (isNetIdValid(newNetId)) {
- Permission permission = mPermissionsController->getPermissionForNetwork(newNetId);
- InterfaceRange range = interfacesForNetId(newNetId, &status);
- for (InterfaceIterator iter = range.first; iter != range.second; ++iter) {
- if (!mRouteController->addDefaultNetwork(iter->second.c_str(), permission)) {
- status = false;
- }
+ permission = mPermissionsController->getPermissionForNetwork(newNetId);
+ range = mNetIdToInterfaces.equal_range(newNetId);
+ for (InterfaceIteratorConst iter = range.first; iter != range.second; ++iter) {
+ if (!mRouteController->addToDefaultNetwork(iter->second.c_str(), permission)) {
+ ALOGE("failed to add interface %s to default netId %u", iter->second.c_str(), newNetId);
+ status = false;
}
}
// Remove the old default network rules.
- if (isNetIdValid(oldNetId)) {
- Permission permission = mPermissionsController->getPermissionForNetwork(oldNetId);
- InterfaceRange range = interfacesForNetId(oldNetId, &status);
- for (InterfaceIterator iter = range.first; iter != range.second; ++iter) {
- if (!mRouteController->removeDefaultNetwork(iter->second.c_str(), permission)) {
- status = false;
- }
+ permission = mPermissionsController->getPermissionForNetwork(oldNetId);
+ range = mNetIdToInterfaces.equal_range(oldNetId);
+ for (InterfaceIteratorConst iter = range.first; iter != range.second; ++iter) {
+ if (!mRouteController->removeFromDefaultNetwork(iter->second.c_str(), permission)) {
+ ALOGE("failed to remove interface %s from default netId %u", iter->second.c_str(),
+ oldNetId);
+ status = false;
}
}
@@ -93,11 +112,11 @@
}
bool NetworkController::setNetworkForUidRange(int uid_start, int uid_end, unsigned netId,
- bool forward_dns) {
- android::RWLock::AutoWLock lock(mRWLock);
- if (uid_start > uid_end || !isNetIdValid(netId))
+ bool forward_dns) {
+ if (uid_start > uid_end || !isValidNetwork(netId))
return false;
+ android::RWLock::AutoWLock lock(mRWLock);
for (std::list<UidEntry>::iterator it = mUidMap.begin(); it != mUidMap.end(); ++it) {
if (it->uid_start != uid_start || it->uid_end != uid_end || it->netId != netId)
continue;
@@ -110,10 +129,10 @@
}
bool NetworkController::clearNetworkForUidRange(int uid_start, int uid_end, unsigned netId) {
- android::RWLock::AutoWLock lock(mRWLock);
- if (uid_start > uid_end || !isNetIdValid(netId))
+ if (uid_start > uid_end || !isValidNetwork(netId))
return false;
+ android::RWLock::AutoWLock lock(mRWLock);
for (std::list<UidEntry>::iterator it = mUidMap.begin(); it != mUidMap.end(); ++it) {
if (it->uid_start != uid_start || it->uid_end != uid_end || it->netId != netId)
continue;
@@ -132,35 +151,46 @@
break;
return it->netId;
}
- if (isNetIdValid(requested_netId))
+ if (mValidNetworks.find(requested_netId) != mValidNetworks.end())
return requested_netId;
return mDefaultNetId;
}
-unsigned NetworkController::getNetworkId(const char* interface) {
- std::map<std::string, unsigned>::const_iterator it = mIfaceNetidMap.find(interface);
- if (it != mIfaceNetidMap.end())
- return it->second;
+unsigned NetworkController::getNetworkId(const char* interface) const {
+ for (InterfaceIteratorConst iter = mNetIdToInterfaces.begin(); iter != mNetIdToInterfaces.end();
+ ++iter) {
+ if (iter->second == interface) {
+ return iter->first;
+ }
+ }
return NETID_UNSET;
}
bool NetworkController::createNetwork(unsigned netId, Permission permission) {
- if (!isNetIdValid(netId)) {
+ if (netId < MIN_NET_ID || netId > MAX_NET_ID) {
ALOGE("invalid netId %u", netId);
return false;
}
+ {
+ android::RWLock::AutoWLock lock(mRWLock);
+ if (!mValidNetworks.insert(netId).second) {
+ ALOGE("duplicate netId %u", netId);
+ return false;
+ }
+ }
+
mPermissionsController->setPermissionForNetwork(permission, netId);
return true;
}
bool NetworkController::addInterfaceToNetwork(unsigned netId, const char* interface) {
- if (!isNetIdValid(netId) || !interface) {
+ if (!isValidNetwork(netId) || !interface) {
ALOGE("invalid netId %u or interface null", netId);
return false;
}
- unsigned existingNetId = netIdForInterface(interface);
+ unsigned existingNetId = getNetworkId(interface);
if (existingNetId != NETID_UNSET) {
ALOGE("interface %s already assigned to netId %u", interface, existingNetId);
return false;
@@ -168,16 +198,15 @@
Permission permission = mPermissionsController->getPermissionForNetwork(netId);
if (!mRouteController->addInterfaceToNetwork(netId, interface, permission)) {
- ALOGE("failed to add rules for interface %s to netId %u", interface, netId);
+ ALOGE("failed to add interface %s to netId %u", interface, netId);
return false;
}
mNetIdToInterfaces.insert(std::pair<unsigned, std::string>(netId, interface));
- mIfaceNetidMap[interface] = netId;
if (netId == getDefaultNetwork() &&
- !mRouteController->addDefaultNetwork(interface, permission)) {
- ALOGE("failed to add interface %s to default network %u", interface, netId);
+ !mRouteController->addToDefaultNetwork(interface, permission)) {
+ ALOGE("failed to add interface %s to default netId %u", interface, netId);
return false;
}
@@ -185,45 +214,41 @@
}
bool NetworkController::removeInterfaceFromNetwork(unsigned netId, const char* interface) {
- if (!isNetIdValid(netId) || !interface) {
+ if (!isValidNetwork(netId) || !interface) {
ALOGE("invalid netId %u or interface null", netId);
return false;
}
- bool status = true;
- bool found = false;
- InterfaceRange range = interfacesForNetId(netId, &status);
+ bool status = false;
+ InterfaceRange range = mNetIdToInterfaces.equal_range(netId);
for (InterfaceIterator iter = range.first; iter != range.second; ++iter) {
if (iter->second == interface) {
mNetIdToInterfaces.erase(iter);
- found = true;
+ status = true;
break;
}
}
- if (!found) {
- ALOGE("interface %s not a member of netId %u", interface, netId);
- status = false;
+ if (!status) {
+ ALOGE("interface %s not assigned to netId %u", interface, netId);
}
Permission permission = mPermissionsController->getPermissionForNetwork(netId);
if (!mRouteController->removeInterfaceFromNetwork(netId, interface, permission)) {
- ALOGE("failed to remove rules for interface %s from netId %u", interface, netId);
+ ALOGE("failed to remove interface %s from netId %u", interface, netId);
status = false;
}
if (netId == getDefaultNetwork() &&
- !mRouteController->removeDefaultNetwork(interface, permission)) {
- ALOGE("failed to remove interface %s from default network %u", interface, netId);
+ !mRouteController->removeFromDefaultNetwork(interface, permission)) {
+ ALOGE("failed to remove interface %s from default netId %u", interface, netId);
status = false;
}
- mIfaceNetidMap.erase(interface);
-
return status;
}
bool NetworkController::destroyNetwork(unsigned netId) {
- if (!isNetIdValid(netId)) {
+ if (!isValidNetwork(netId)) {
ALOGE("invalid netId %u", netId);
return false;
}
@@ -232,9 +257,9 @@
bool status = true;
- InterfaceRange range = interfacesForNetId(netId, &status);
- for (InterfaceIterator iter = range.first; iter != range.second; ) {
- InterfaceIterator toErase = iter;
+ InterfaceRange range = mNetIdToInterfaces.equal_range(netId);
+ for (InterfaceIteratorConst iter = range.first; iter != range.second; ) {
+ InterfaceIteratorConst toErase = iter;
++iter;
if (!removeInterfaceFromNetwork(netId, toErase->second.c_str())) {
status = false;
@@ -242,13 +267,14 @@
}
if (netId == getDefaultNetwork()) {
- // Could the default network have changed from below us, after we evaluated the 'if', making
- // it wrong to call setDefaultNetwork() now? No, because the default can only change due to
- // a command from CommandListener; but commands are serialized, I.e., we are processing the
- // destroyNetwork() command here, so a setDefaultNetwork() command can't happen in parallel.
setDefaultNetwork(NETID_UNSET);
}
+ {
+ android::RWLock::AutoWLock lock(mRWLock);
+ mValidNetworks.erase(netId);
+ }
+
mPermissionsController->setPermissionForNetwork(PERMISSION_NONE, netId);
_resolv_delete_cache_for_net(netId);
@@ -268,14 +294,12 @@
bool status = true;
for (size_t i = 0; i < netId.size(); ++i) {
- if (!isNetIdValid(netId[i])) {
+ if (!isValidNetwork(netId[i])) {
ALOGE("invalid netId %u", netId[i]);
status = false;
continue;
}
- InterfaceRange range = interfacesForNetId(netId[i], &status);
-
Permission oldPermission = mPermissionsController->getPermissionForNetwork(netId[i]);
if (oldPermission == newPermission) {
continue;
@@ -284,9 +308,12 @@
// TODO: ioctl(SIOCKILLADDR, ...) to kill sockets on the network that don't have
// newPermission.
- for (InterfaceIterator iter = range.first; iter != range.second; ++iter) {
+ InterfaceRange range = mNetIdToInterfaces.equal_range(netId[i]);
+ for (InterfaceIteratorConst iter = range.first; iter != range.second; ++iter) {
if (!mRouteController->modifyNetworkPermission(netId[i], iter->second.c_str(),
oldPermission, newPermission)) {
+ ALOGE("failed to change permission on interface %s of netId %u from %x to %x",
+ iter->second.c_str(), netId[i], oldPermission, newPermission);
status = false;
}
}
@@ -307,34 +334,23 @@
return modifyRoute(netId, interface, destination, nexthop, false);
}
-unsigned NetworkController::netIdForInterface(const char* interface) {
- for (InterfaceIterator iter = mNetIdToInterfaces.begin(); iter != mNetIdToInterfaces.end();
- ++iter) {
- if (iter->second == interface) {
- return iter->first;
- }
+bool NetworkController::isValidNetwork(unsigned netId) const {
+ if (netId == NETID_UNSET) {
+ return false;
}
- return NETID_UNSET;
-}
-NetworkController::InterfaceRange NetworkController::interfacesForNetId(unsigned netId,
- bool* status) {
- InterfaceRange range = mNetIdToInterfaces.equal_range(netId);
- if (range.first == range.second) {
- ALOGE("unknown netId %u", netId);
- *status = false;
- }
- return range;
+ android::RWLock::AutoRLock lock(mRWLock);
+ return mValidNetworks.find(netId) != mValidNetworks.end();
}
bool NetworkController::modifyRoute(unsigned netId, const char* interface, const char* destination,
const char* nexthop, bool add) {
- if (!isNetIdValid(netId)) {
+ if (!isValidNetwork(netId)) {
ALOGE("invalid netId %u", netId);
return false;
}
- if (netIdForInterface(interface) != netId) {
+ if (getNetworkId(interface) != netId) {
ALOGE("netId %u has no such interface %s", netId, interface);
return false;
}
@@ -343,10 +359,6 @@
mRouteController->removeRoute(interface, destination, nexthop);
}
-NetworkController::UidEntry::UidEntry(
- int start, int end, unsigned netId, bool forward_dns)
- : uid_start(start),
- uid_end(end),
- netId(netId),
- forward_dns(forward_dns) {
+NetworkController::UidEntry::UidEntry(int start, int end, unsigned netId, bool forward_dns)
+ : uid_start(start), uid_end(end), netId(netId), forward_dns(forward_dns) {
}
diff --git a/server/NetworkController.h b/server/NetworkController.h
index fff3289..27785d3 100644
--- a/server/NetworkController.h
+++ b/server/NetworkController.h
@@ -14,19 +14,20 @@
* limitations under the License.
*/
-#ifndef _NETD_NETWORKCONTROLLER_H
-#define _NETD_NETWORKCONTROLLER_H
+#ifndef NETD_SERVER_NETWORK_CONTROLLER_H
+#define NETD_SERVER_NETWORK_CONTROLLER_H
#include "Permission.h"
+#include "utils/RWLock.h"
#include <list>
#include <map>
-#include <string>
-#include <vector>
-
+#include <set>
#include <stddef.h>
#include <stdint.h>
-#include <utils/RWLock.h>
+#include <string>
+#include <utility>
+#include <vector>
class PermissionsController;
class RouteController;
@@ -39,8 +40,6 @@
*/
class NetworkController {
public:
- static bool isNetIdValid(unsigned netId);
-
NetworkController(PermissionsController* permissionsController,
RouteController* routeController);
@@ -56,7 +55,7 @@
// requests to VPNs without DNS servers.
unsigned getNetwork(int uid, unsigned requested_netId, bool for_dns) const;
- unsigned getNetworkId(const char* interface);
+ unsigned getNetworkId(const char* interface) const;
bool createNetwork(unsigned netId, Permission permission);
bool destroyNetwork(unsigned netId);
@@ -73,16 +72,13 @@
bool removeRoute(unsigned netId, const char* interface, const char* destination,
const char* nexthop);
+ bool isValidNetwork(unsigned netId) const;
+
private:
+ typedef std::multimap<unsigned, std::string>::const_iterator InterfaceIteratorConst;
typedef std::multimap<unsigned, std::string>::iterator InterfaceIterator;
typedef std::pair<InterfaceIterator, InterfaceIterator> InterfaceRange;
- // Returns the netId that |interface| belongs to, or NETID_UNSET if it doesn't belong to any.
- unsigned netIdForInterface(const char* interface);
-
- // Returns the interfaces assigned to |netId|. Sets |*status| to false if there are none.
- InterfaceRange interfacesForNetId(unsigned netId, bool* status);
-
bool modifyRoute(unsigned netId, const char* interface, const char* destination,
const char* nexthop, bool add);
@@ -94,11 +90,11 @@
UidEntry(int uid_start, int uid_end, unsigned netId, bool forward_dns);
};
+ // mRWLock guards all accesses to mUidMap, mDefaultNetId and mValidNetworks.
mutable android::RWLock mRWLock;
std::list<UidEntry> mUidMap;
unsigned mDefaultNetId;
-
- std::map<std::string, unsigned> mIfaceNetidMap;
+ std::set<unsigned> mValidNetworks;
PermissionsController* const mPermissionsController;
RouteController* const mRouteController;
@@ -112,4 +108,4 @@
std::multimap<unsigned, std::string> mNetIdToInterfaces;
};
-#endif
+#endif // NETD_SERVER_NETWORK_CONTROLLER_H
diff --git a/server/PermissionsController.cpp b/server/PermissionsController.cpp
index a747efc..63c80dc 100644
--- a/server/PermissionsController.cpp
+++ b/server/PermissionsController.cpp
@@ -34,23 +34,28 @@
} // namespace
Permission PermissionsController::getPermissionForUser(unsigned uid) const {
+ android::RWLock::AutoRLock lock(mRWLock);
return get(mUsers, uid);
}
void PermissionsController::setPermissionForUser(Permission permission, unsigned uid) {
+ android::RWLock::AutoWLock lock(mRWLock);
set(&mUsers, permission, uid);
}
Permission PermissionsController::getPermissionForNetwork(unsigned netId) const {
+ android::RWLock::AutoRLock lock(mRWLock);
return get(mNetworks, netId);
}
void PermissionsController::setPermissionForNetwork(Permission permission, unsigned netId) {
+ android::RWLock::AutoWLock lock(mRWLock);
set(&mNetworks, permission, netId);
}
bool PermissionsController::isUserPermittedOnNetwork(unsigned uid, unsigned netId) const {
- Permission userPermission = getPermissionForUser(uid);
- Permission networkPermission = getPermissionForNetwork(netId);
+ android::RWLock::AutoRLock lock(mRWLock);
+ Permission userPermission = get(mUsers, uid);
+ Permission networkPermission = get(mNetworks, netId);
return (userPermission & networkPermission) == networkPermission;
}
diff --git a/server/PermissionsController.h b/server/PermissionsController.h
index e259e6d..c2a014d 100644
--- a/server/PermissionsController.h
+++ b/server/PermissionsController.h
@@ -18,6 +18,7 @@
#define NETD_SERVER_PERMISSIONS_CONTROLLER_H
#include "Permission.h"
+#include "utils/RWLock.h"
#include <map>
@@ -32,6 +33,7 @@
bool isUserPermittedOnNetwork(unsigned uid, unsigned netId) const;
private:
+ mutable android::RWLock mRWLock;
std::map<unsigned, Permission> mUsers;
std::map<unsigned, Permission> mNetworks;
};
diff --git a/server/RouteController.cpp b/server/RouteController.cpp
index a58f02d..7f9ce39 100644
--- a/server/RouteController.cpp
+++ b/server/RouteController.cpp
@@ -278,11 +278,11 @@
modifyPerNetworkRules(netId, interface, oldPermission, false, false);
}
-bool RouteController::addDefaultNetwork(const char* interface, Permission permission) {
+bool RouteController::addToDefaultNetwork(const char* interface, Permission permission) {
return modifyDefaultNetworkRules(interface, permission, ADD);
}
-bool RouteController::removeDefaultNetwork(const char* interface, Permission permission) {
+bool RouteController::removeFromDefaultNetwork(const char* interface, Permission permission) {
return modifyDefaultNetworkRules(interface, permission, DEL);
}
diff --git a/server/RouteController.h b/server/RouteController.h
index 2769a31..6d66ed7 100644
--- a/server/RouteController.h
+++ b/server/RouteController.h
@@ -31,8 +31,8 @@
static bool modifyNetworkPermission(unsigned netId, const char* interface,
Permission oldPermission, Permission newPermission);
- static bool addDefaultNetwork(const char* interface, Permission permission);
- static bool removeDefaultNetwork(const char* interface, Permission permission);
+ static bool addToDefaultNetwork(const char* interface, Permission permission);
+ static bool removeFromDefaultNetwork(const char* interface, Permission permission);
static bool addRoute(const char* interface, const char* destination, const char* nexthop);
static bool removeRoute(const char* interface, const char* destination, const char* nexthop);