From 9017a07049cd7ec618aead020663e8461bd2b24b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Thu, 16 Jun 2022 14:49:27 -0700 Subject: [PATCH] switch netd_configuration_map from hash map to array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This eliminates the need for netd_updatable BpfHandler.cpp to initialize the hash map with a zero. On startup the map will be freshly initialized and thus zero. On restart it might not be empty, but it doesn't matter to netd. Furthermore the mainline component of the system server will re-initialize it again anyway: see service/native/TrafficController.cpp initMaps() This does remove the ability to call deleteValue on a key, since that would always return -EINVAL, but since we don't currently do that, that's really a feature. (It does suggest though that we should have a BpfMapNonNullable class which is writeable, but without a deleteValue() function) Additionally BpfMap arrays are more efficient for the kernel bpf jit compiler, as - on newer kernels - it can optimize the read/write into a simple memory access (as opposed to a bpf helper call). Before: $ adb shell ls -l /sys/fs/bpf/netd_shared/map_netd_configuration_map -rw-rw---- 1 root net_bw_acct 0 2022-06-11 08:20 /sys/fs/bpf/netd_shared/ map_netd_configuration_map After: $ adbz shell ls -l /sys/fs/bpf/netd_shared/map_netd_configuration_map -r--rw---- 1 root net_bw_acct 0 2022-06-16 15:03 /sys/fs/bpf/netd_shared/map_netd_configuration_map Bug: 218408035 Bug: 235590615 Test: TreeHugger Signed-off-by: Maciej Żenczykowski Change-Id: I21730e4fa22fbf0c94ab0ca5c5db03aa000b7680 (cherry picked from commit b10e055f4b63ef5ae12585069481558b93ecd87f) Merged-In: I21730e4fa22fbf0c94ab0ca5c5db03aa000b7680 --- bpf_progs/bpf_shared.h | 4 ++-- bpf_progs/netd.c | 7 ++++++- netd/BpfHandler.cpp | 2 -- netd/BpfHandler.h | 2 +- netd/BpfHandlerTest.cpp | 8 ++++---- service/native/TrafficControllerTest.cpp | 6 +++--- 6 files changed, 16 insertions(+), 13 deletions(-) diff --git a/bpf_progs/bpf_shared.h b/bpf_progs/bpf_shared.h index dd9fb07d53..fd449a3de4 100644 --- a/bpf_progs/bpf_shared.h +++ b/bpf_progs/bpf_shared.h @@ -190,9 +190,9 @@ typedef struct { STRUCT_SIZE(UidOwnerValue, 2 * 4); // 8 // Entry in the configuration map that stores which UID rules are enabled. -#define UID_RULES_CONFIGURATION_KEY 1 +#define UID_RULES_CONFIGURATION_KEY 0 // Entry in the configuration map that stores which stats map is currently in use. -#define CURRENT_STATS_MAP_CONFIGURATION_KEY 2 +#define CURRENT_STATS_MAP_CONFIGURATION_KEY 1 typedef struct { uint32_t iif; // The input interface index diff --git a/bpf_progs/netd.c b/bpf_progs/netd.c index acb2f9c3d3..64320c68e0 100644 --- a/bpf_progs/netd.c +++ b/bpf_progs/netd.c @@ -63,13 +63,18 @@ #define DEFINE_BPF_MAP_RW_NETD(the_map, TYPE, TypeOfKey, TypeOfValue, num_entries) \ DEFINE_BPF_MAP_UGM(the_map, TYPE, TypeOfKey, TypeOfValue, num_entries, AID_ROOT, AID_NET_BW_ACCT, 0660) +// Bpf map arrays on creation are preinitialized to 0 and do not support deletion of a key, +// see: kernel/bpf/arraymap.c array_map_delete_elem() returns -EINVAL (from both syscall and ebpf) +// Additionally on newer kernels the bpf jit can optimize out the lookups. +// only valid indexes are [0..CONFIGURATION_MAP_SIZE-1] +DEFINE_BPF_MAP_RO_NETD(configuration_map, ARRAY, uint32_t, uint32_t, CONFIGURATION_MAP_SIZE) + DEFINE_BPF_MAP_RW_NETD(cookie_tag_map, HASH, uint64_t, UidTagValue, COOKIE_UID_MAP_SIZE) DEFINE_BPF_MAP_NO_NETD(uid_counterset_map, HASH, uint32_t, uint8_t, UID_COUNTERSET_MAP_SIZE) DEFINE_BPF_MAP_NO_NETD(app_uid_stats_map, HASH, uint32_t, StatsValue, APP_STATS_MAP_SIZE) DEFINE_BPF_MAP_RW_NETD(stats_map_A, HASH, StatsKey, StatsValue, STATS_MAP_SIZE) DEFINE_BPF_MAP_RO_NETD(stats_map_B, HASH, StatsKey, StatsValue, STATS_MAP_SIZE) DEFINE_BPF_MAP_NO_NETD(iface_stats_map, HASH, uint32_t, StatsValue, IFACE_STATS_MAP_SIZE) -DEFINE_BPF_MAP_RW_NETD(configuration_map, HASH, uint32_t, uint32_t, CONFIGURATION_MAP_SIZE) DEFINE_BPF_MAP_NO_NETD(uid_owner_map, HASH, uint32_t, UidOwnerValue, UID_OWNER_MAP_SIZE) DEFINE_BPF_MAP_RW_NETD(uid_permission_map, HASH, uint32_t, uint8_t, UID_OWNER_MAP_SIZE) diff --git a/netd/BpfHandler.cpp b/netd/BpfHandler.cpp index 003433919b..6ae26c38f1 100644 --- a/netd/BpfHandler.cpp +++ b/netd/BpfHandler.cpp @@ -110,8 +110,6 @@ Status BpfHandler::initMaps() { 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(mConfigurationMap.writeValue(CURRENT_STATS_MAP_CONFIGURATION_KEY, SELECT_MAP_A, - BPF_ANY)); RETURN_IF_NOT_OK(mUidPermissionMap.init(UID_PERMISSION_MAP_PATH)); return netdutils::status::ok; diff --git a/netd/BpfHandler.h b/netd/BpfHandler.h index 7e3b94d6e8..5ee04d1a53 100644 --- a/netd/BpfHandler.h +++ b/netd/BpfHandler.h @@ -63,7 +63,7 @@ class BpfHandler { BpfMap mCookieTagMap; BpfMap mStatsMapA; BpfMapRO mStatsMapB; - BpfMap mConfigurationMap; + BpfMapRO mConfigurationMap; BpfMap mUidPermissionMap; std::mutex mMutex; diff --git a/netd/BpfHandlerTest.cpp b/netd/BpfHandlerTest.cpp index 1bd222dfce..a031dbb9e6 100644 --- a/netd/BpfHandlerTest.cpp +++ b/netd/BpfHandlerTest.cpp @@ -49,7 +49,7 @@ class BpfHandlerTest : public ::testing::Test { BpfHandler mBh; BpfMap mFakeCookieTagMap; BpfMap mFakeStatsMapA; - BpfMap mFakeConfigurationMap; + BpfMapRO mFakeConfigurationMap; BpfMap mFakeUidPermissionMap; void SetUp() { @@ -62,7 +62,7 @@ class BpfHandlerTest : public ::testing::Test { mFakeStatsMapA.resetMap(BPF_MAP_TYPE_HASH, TEST_MAP_SIZE); ASSERT_VALID(mFakeStatsMapA); - mFakeConfigurationMap.resetMap(BPF_MAP_TYPE_HASH, 1); + mFakeConfigurationMap.resetMap(BPF_MAP_TYPE_ARRAY, CONFIGURATION_MAP_SIZE); ASSERT_VALID(mFakeConfigurationMap); mFakeUidPermissionMap.resetMap(BPF_MAP_TYPE_HASH, TEST_MAP_SIZE, 0); @@ -75,8 +75,8 @@ class BpfHandlerTest : public ::testing::Test { mBh.mConfigurationMap = mFakeConfigurationMap; ASSERT_VALID(mBh.mConfigurationMap); // Always write to stats map A by default. - ASSERT_RESULT_OK(mBh.mConfigurationMap.writeValue(CURRENT_STATS_MAP_CONFIGURATION_KEY, - SELECT_MAP_A, BPF_ANY)); + static_assert(SELECT_MAP_A == 0, "bpf map arrays are zero-initialized"); + mBh.mUidPermissionMap = mFakeUidPermissionMap; ASSERT_VALID(mBh.mUidPermissionMap); } diff --git a/service/native/TrafficControllerTest.cpp b/service/native/TrafficControllerTest.cpp index c920398078..9e53f11d0d 100644 --- a/service/native/TrafficControllerTest.cpp +++ b/service/native/TrafficControllerTest.cpp @@ -83,7 +83,7 @@ class TrafficControllerTest : public ::testing::Test { mFakeStatsMapA.resetMap(BPF_MAP_TYPE_HASH, TEST_MAP_SIZE); ASSERT_VALID(mFakeStatsMapA); - mFakeConfigurationMap.resetMap(BPF_MAP_TYPE_HASH, 1); + mFakeConfigurationMap.resetMap(BPF_MAP_TYPE_ARRAY, CONFIGURATION_MAP_SIZE); ASSERT_VALID(mFakeConfigurationMap); mFakeUidOwnerMap.resetMap(BPF_MAP_TYPE_HASH, TEST_MAP_SIZE); @@ -101,8 +101,8 @@ class TrafficControllerTest : public ::testing::Test { ASSERT_VALID(mTc.mConfigurationMap); // Always write to stats map A by default. - ASSERT_RESULT_OK(mTc.mConfigurationMap.writeValue(CURRENT_STATS_MAP_CONFIGURATION_KEY, - SELECT_MAP_A, BPF_ANY)); + static_assert(SELECT_MAP_A == 0); + mTc.mUidOwnerMap = mFakeUidOwnerMap; ASSERT_VALID(mTc.mUidOwnerMap); mTc.mUidPermissionMap = mFakeUidPermissionMap;