BpfHandler - eliminate mutex

since it doesn't actually work (this runs in netd, but is meant
to protect against system server modifications)

Test: TreeHugger
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: I87f268f1a68c559b19caa4d625cb9eb8395c91e2

netd/BpfHandler.cpp

@@ -110,12 +110,12 @@ Status BpfHandler::init(const char* cg2_path) {
 }
 
 Status BpfHandler::initMaps() {
-    std::lock_guard guard(mMutex);
-    RETURN_IF_NOT_OK(mCookieTagMap.init(COOKIE_TAG_MAP_PATH));
     RETURN_IF_NOT_OK(mStatsMapA.init(STATS_MAP_A_PATH));
     RETURN_IF_NOT_OK(mStatsMapB.init(STATS_MAP_B_PATH));
     RETURN_IF_NOT_OK(mConfigurationMap.init(CONFIGURATION_MAP_PATH));
     RETURN_IF_NOT_OK(mUidPermissionMap.init(UID_PERMISSION_MAP_PATH));
+    // initialized last so mCookieTagMap.isValid() implies everything else is valid too
+    RETURN_IF_NOT_OK(mCookieTagMap.init(COOKIE_TAG_MAP_PATH));
     ALOGI("%s successfully", __func__);
 
     return netdutils::status::ok;
@@ -133,7 +133,6 @@ bool BpfHandler::hasUpdateDeviceStatsPermission(uid_t uid) {
 }
 
 int BpfHandler::tagSocket(int sockFd, uint32_t tag, uid_t chargeUid, uid_t realUid) {
-    std::lock_guard guard(mMutex);
     if (!mCookieTagMap.isValid()) return -EPERM;
 
     if (chargeUid != realUid && !hasUpdateDeviceStatsPermission(realUid)) return -EPERM;
@@ -185,9 +184,9 @@ int BpfHandler::tagSocket(int sockFd, uint32_t tag, uid_t chargeUid, uid_t realU
     uint32_t perUidEntryCount = 0;
     // Now we go through the stats map and count how many entries are associated
     // with chargeUid. If the uid entry hit the limit for each chargeUid, we block
-    // the request to prevent the map from overflow. It is safe here to iterate
-    // 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.
+    // the request to prevent the map from overflow. Note though that it isn't really
+    // safe here to iterate over the map since it might be modified by the system server,
+    // which might toggle the live stats map and clean it.
     const auto countUidStatsEntries = [chargeUid, &totalEntryCount, &perUidEntryCount](
                                               const StatsKey& key,
                                               const BpfMap<StatsKey, StatsValue>&) {
@@ -227,9 +226,9 @@ int BpfHandler::tagSocket(int sockFd, uint32_t tag, uid_t chargeUid, uid_t realU
     }
     // Update the tag information of a socket to the cookieUidMap. Use BPF_ANY
     // flag so it will insert a new entry to the map if that value doesn't exist
-    // yet. And update the tag if there is already a tag stored. Since the eBPF
+    // yet and update the tag if there is already a tag stored. Since the eBPF
     // program in kernel only read this map, and is protected by rcu read lock. It
-    // should be fine to cocurrently update the map while eBPF program is running.
+    // should be fine to concurrently update the map while eBPF program is running.
     res = mCookieTagMap.writeValue(sock_cookie, newKey, BPF_ANY);
     if (!res.ok()) {
         ALOGE("Failed to tag the socket: %s, fd: %d", strerror(res.error().code()),
@@ -240,8 +239,6 @@ int BpfHandler::tagSocket(int sockFd, uint32_t tag, uid_t chargeUid, uid_t realU
 }
 
 int BpfHandler::untagSocket(int sockFd) {
-    std::lock_guard guard(mMutex);
-
     uint64_t sock_cookie = getSocketCookie(sockFd);
     if (sock_cookie == NONEXISTENT_COOKIE) return -errno;
 

netd/BpfHandler.h

@@ -16,8 +16,6 @@
 
 #pragma once
 
-#include <mutex>
-
 #include <netdutils/Status.h>
 #include "bpf/BpfMap.h"
 #include "bpf_shared.h"
@@ -66,8 +64,6 @@ class BpfHandler {
     BpfMapRO<uint32_t, uint32_t> mConfigurationMap;
     BpfMap<uint32_t, uint8_t> mUidPermissionMap;
 
-    std::mutex mMutex;
-
     // The limit on the number of stats entries a uid can have in the per uid stats map. BpfHandler
     // will block that specific uid from tagging new sockets after the limit is reached.
     const uint32_t mPerUidStatsEntriesLimit;

netd/BpfHandlerTest.cpp

@@ -53,7 +53,6 @@ class BpfHandlerTest : public ::testing::Test {
     BpfMap<uint32_t, uint8_t> mFakeUidPermissionMap;
 
     void SetUp() {
-        std::lock_guard guard(mBh.mMutex);
         ASSERT_EQ(0, setrlimitForTest());
 
         mFakeCookieTagMap.resetMap(BPF_MAP_TYPE_HASH, TEST_MAP_SIZE);