Improve the locking design inside TrafficController

Rename the mOwnerMatchMutex lock to mMutex and use it to protect all
data structures in TrafficController that could be accessed
concurrently. Add necessary annotations to the data structures needing
protection to catch unsafe access at compile time.

Test: netd_unit_test, netd_integration_test
Bug: 130334320
Change-Id: I847ce518df8626f647e1d80468e4daf4604872f2

1 files changed, 14 insertions, 11 deletions

diff --git a/server/TrafficController.cpp b/server/TrafficController.cpp
index 1f3b557f8..c39440fe8 100644
--- a/server/TrafficController.cpp
+++ b/server/TrafficController.cpp
@@ -180,7 +180,7 @@ Status changeOwnerAndMode(const char* path, gid_t group, const char* debugName,
 TrafficController::TrafficController() : mBpfLevel(getBpfSupportLevel()) {}
 
 Status TrafficController::initMaps() {
-    std::lock_guard ownerMapGuard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
     RETURN_IF_NOT_OK(mCookieTagMap.init(COOKIE_TAG_MAP_PATH));
     RETURN_IF_NOT_OK(changeOwnerAndMode(COOKIE_TAG_MAP_PATH, AID_NET_BW_ACCT, "CookieTagMap",
                                         false));
@@ -330,7 +330,7 @@ Status TrafficController::start() {
 }
 
 int TrafficController::tagSocket(int sockFd, uint32_t tag, uid_t uid, uid_t callingUid) {
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
     if (uid != callingUid && !hasUpdateDeviceStatsPermission(callingUid)) {
         return -EPERM;
     }
@@ -348,7 +348,7 @@ int TrafficController::tagSocket(int sockFd, uint32_t tag, uid_t uid, uid_t call
     // Now we go through the stats map and count how many entries are associated
     // with target uid. If the uid entry hit the limit for each uid, we block
     // the request to prevent the map from overflow. It is safe here to iterate
-    // over the map since when mOwnerMatchMutex is hold, system server cannot toggle
+    // over the map since when mMutex is hold, system server cannot toggle
     // the live stats map and clean it. So nobody can delete entries from the map.
     const auto countUidStatsEntries = [uid, &totalEntryCount](const StatsKey& key,
                                                               BpfMap<StatsKey, StatsValue>&) {
@@ -407,6 +407,7 @@ int TrafficController::untagSocket(int sockFd) {
 int TrafficController::setCounterSet(int counterSetNum, uid_t uid, uid_t callingUid) {
     if (counterSetNum < 0 || counterSetNum >= OVERFLOW_COUNTERSET) return -EINVAL;
 
+    std::lock_guard guard(mMutex);
     if (!hasUpdateDeviceStatsPermission(callingUid)) return -EPERM;
 
     if (mBpfLevel == BpfLevel::NONE) {
@@ -439,6 +440,7 @@ int TrafficController::setCounterSet(int counterSetNum, uid_t uid, uid_t calling
 // is called inside removeUidsLocked() while holding mStatsLock. So it is safe
 // to iterate and modify the stats maps.
 int TrafficController::deleteTagData(uint32_t tag, uid_t uid, uid_t callingUid) {
+    std::lock_guard guard(mMutex);
     if (!hasUpdateDeviceStatsPermission(callingUid)) return -EPERM;
 
     if (mBpfLevel == BpfLevel::NONE) {
@@ -522,7 +524,7 @@ int TrafficController::addInterface(const char* name, uint32_t ifaceIndex) {
 
 Status TrafficController::updateOwnerMapEntry(UidOwnerMatchType match, uid_t uid, FirewallRule rule,
                                               FirewallType type) {
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
     if ((rule == ALLOW && type == WHITELIST) || (rule == DENY && type == BLACKLIST)) {
         RETURN_IF_NOT_OK(addRule(mUidOwnerMap, uid, match));
     } else if ((rule == ALLOW && type == BLACKLIST) || (rule == DENY && type == WHITELIST)) {
@@ -585,7 +587,7 @@ Status TrafficController::addRule(BpfMap<uint32_t, UidOwnerValue>& map, uint32_t
 Status TrafficController::updateUidOwnerMap(const std::vector<std::string>& appStrUids,
                                             BandwidthController::IptJumpOp jumpHandling,
                                             BandwidthController::IptOp op) {
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
     UidOwnerMatchType match = jumpOpToMatch(jumpHandling);
     if (match == NO_MATCH) {
         return statusFromErrno(
@@ -642,7 +644,7 @@ int TrafficController::changeUidOwnerRule(ChildChain chain, uid_t uid, FirewallR
 
 Status TrafficController::replaceRulesInMap(const UidOwnerMatchType match,
                                             const std::vector<int32_t>& uids) {
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
     std::set<int32_t> uidSet(uids.begin(), uids.end());
     std::vector<uint32_t> uidsToDelete;
     auto getUidsToDelete = [&uidsToDelete, &uidSet](const uint32_t& key,
@@ -673,7 +675,7 @@ Status TrafficController::addUidInterfaceRules(const int iif,
     if (!iif) {
         return statusFromErrno(EINVAL, "Interface rule must specify interface");
     }
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
 
     for (auto uid : uidsToAdd) {
         netdutils::Status result = addRule(mUidOwnerMap, uid, IIF_MATCH, iif);
@@ -689,7 +691,7 @@ Status TrafficController::removeUidInterfaceRules(const std::vector<int32_t>& ui
         ALOGW("UID ingress interface filtering not possible without BPF owner match");
         return statusFromErrno(EOPNOTSUPP, "eBPF not supported");
     }
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
 
     for (auto uid : uidsToDelete) {
         netdutils::Status result = removeRule(mUidOwnerMap, uid, IIF_MATCH);
@@ -730,7 +732,7 @@ int TrafficController::replaceUidOwnerMap(const std::string& name, bool isWhitel
 }
 
 int TrafficController::toggleUidOwnerMap(ChildChain chain, bool enable) {
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
     uint32_t key = UID_RULES_CONFIGURATION_KEY;
     auto oldConfiguration = mConfigurationMap.readValue(key);
     if (!isOk(oldConfiguration)) {
@@ -768,7 +770,7 @@ BpfLevel TrafficController::getBpfLevel() {
 }
 
 Status TrafficController::swapActiveStatsMap() {
-    std::lock_guard guard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
 
     if (mBpfLevel == BpfLevel::NONE) {
         return statusFromErrno(EOPNOTSUPP, "This device doesn't have eBPF support");
@@ -810,6 +812,7 @@ Status TrafficController::swapActiveStatsMap() {
 }
 
 void TrafficController::setPermissionForUids(int permission, const std::vector<uid_t>& uids) {
+    std::lock_guard guard(mMutex);
     if (permission == INetd::PERMISSION_UNINSTALLED) {
         for (uid_t uid : uids) {
             // Clean up all permission information for the related uid if all the
@@ -879,7 +882,7 @@ void dumpBpfMap(const std::string& mapName, DumpWriter& dw, const std::string& h
 const String16 TrafficController::DUMP_KEYWORD = String16("trafficcontroller");
 
 void TrafficController::dump(DumpWriter& dw, bool verbose) {
-    std::lock_guard ownerMapGuard(mOwnerMatchMutex);
+    std::lock_guard guard(mMutex);
     ScopedIndent indentTop(dw);
     dw.println("TrafficController");