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
diff --git a/server/TrafficController.cpp b/server/TrafficController.cpp
index 1f3b557..c39440f 100644
--- a/server/TrafficController.cpp
+++ b/server/TrafficController.cpp
@@ -180,7 +180,7 @@
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 @@
}
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 @@
// 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::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 @@
// 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 @@
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::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 @@
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 @@
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 @@
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::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 @@
}
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 @@
}
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 @@
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");