This is an automated email from the ASF dual-hosted git repository. bmahler pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 907aab5645830aac6f164b903da5945a7191ec29 Author: Benjamin Mahler <[email protected]> AuthorDate: Fri Jul 26 16:41:27 2024 -0400 Revert "[cgroups2] Add allow / deny list normalization validation." This reverts commit 45d290aeff6912c8e6a4b1a7358c4e9772c447b4. --- src/linux/cgroups2.cpp | 12 -- src/linux/cgroups2.hpp | 10 +- .../device_manager/device_manager.cpp | 138 ++------------------- .../device_manager/device_manager.hpp | 12 -- src/tests/containerizer/cgroups2_tests.cpp | 42 ------- src/tests/device_manager_tests.cpp | 95 -------------- 6 files changed, 8 insertions(+), 301 deletions(-) diff --git a/src/linux/cgroups2.cpp b/src/linux/cgroups2.cpp index 2c8290727..ac4678261 100644 --- a/src/linux/cgroups2.cpp +++ b/src/linux/cgroups2.cpp @@ -41,8 +41,6 @@ #include "linux/ebpf.hpp" #include "linux/fs.hpp" -#include "slave/containerizer/device_manager/device_manager.hpp" - using std::ostream; using std::set; using std::string; @@ -1467,16 +1465,6 @@ Try<Nothing> configure( const vector<Entry>& allow, const vector<Entry>& deny) { - using mesos::internal::slave::DeviceManager; - DeviceManager::CgroupDeviceAccess state = - CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {})); - state.allow_list = allow; - state.deny_list = deny; - if (!state.normalized()) { - return Error( - "Failed to validate arguments: allow or deny lists are not normalized."); - } - Try<ebpf::Program> program = DeviceProgram::build(allow, deny); if (program.isError()) { diff --git a/src/linux/cgroups2.hpp b/src/linux/cgroups2.hpp index a814bc03c..accaebdad 100644 --- a/src/linux/cgroups2.hpp +++ b/src/linux/cgroups2.hpp @@ -434,15 +434,7 @@ using cgroups::devices::Entry; // are hierarchical. I.e. if a parent cgroup does not allow an access then // 'this' cgroup will be denied access. // For access to be granted, the requested access must match an entry in the -// allow list, and not match with any entry on the deny list. If the device -// type, major, minor numbers match with an entry on the deny list, there must -// be no overlap between the requested access and the deny entry's accesses -// -// We additionally enforce the following constraints on both allow and deny: -// 1. No Entry can have no accesses specified -// 2. No two entries on the same list can have the same type, major & minor -// numbers. -// 3. No two entries on the same list can be encompassed by the other entry. +// allow list, and not match with any entry on the deny list. Try<Nothing> configure( const std::string& cgroup, const std::vector<Entry>& allow, diff --git a/src/slave/containerizer/device_manager/device_manager.cpp b/src/slave/containerizer/device_manager/device_manager.cpp index 6f013d4bc..4c9b86393 100644 --- a/src/slave/containerizer/device_manager/device_manager.cpp +++ b/src/slave/containerizer/device_manager/device_manager.cpp @@ -91,21 +91,9 @@ public: } } } - auto it = device_access_per_cgroup.find(cgroup); - if (it != device_access_per_cgroup.end()) { - return Failure( - "Failed to configure allow and deny devices: cgroup entry already " - "exists."); - } else { - auto result = device_access_per_cgroup.emplace( - cgroup, - CHECK_NOTERROR( - DeviceManager::CgroupDeviceAccess::create(allow_list, deny_list))); - if (!result.second) { - return Failure("Failed to insert new element for key '" + cgroup + "'"); - } - } + device_access_per_cgroup[cgroup].allow_list = allow_list; + device_access_per_cgroup[cgroup].deny_list = deny_list; Try<Nothing> commit = commit_device_access_changes(cgroup); if (commit.isError()) { @@ -136,21 +124,10 @@ public: } } - auto it = device_access_per_cgroup.find(cgroup); - if (it != device_access_per_cgroup.end()) { - it->second = DeviceManager::apply_diff( - it->second, non_wildcard_additions, non_wildcard_removals); - } else { - auto result = device_access_per_cgroup.emplace( - cgroup, - DeviceManager::apply_diff( - CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {})), - non_wildcard_additions, - non_wildcard_removals)); - if (!result.second) { - return Failure("Failed to insert new element for key '" + cgroup + "'"); - } - } + device_access_per_cgroup[cgroup] = DeviceManager::apply_diff( + device_access_per_cgroup[cgroup], + non_wildcard_additions, + non_wildcard_removals); Try<Nothing> commit = commit_device_access_changes(cgroup); if (commit.isError()) { @@ -172,7 +149,7 @@ public: { return device_access_per_cgroup.contains(cgroup) ? device_access_per_cgroup.at(cgroup) - : CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {})); + : DeviceManager::CgroupDeviceAccess(); } private: @@ -197,107 +174,6 @@ private: }; -DeviceManager::CgroupDeviceAccess::CgroupDeviceAccess( - const std::vector<cgroups::devices::Entry>& _allow_list, - const std::vector<cgroups::devices::Entry>& _deny_list) - : allow_list(_allow_list), deny_list(_deny_list) {} - - -bool DeviceManager::CgroupDeviceAccess::normalized() const -{ - auto has_empties = [](const vector<Entry>& entries) { - foreach (const Entry& entry, entries) { - if (entry.access.none()) { - return true; - } - } - return false; - }; - - if (has_empties(allow_list) || has_empties(deny_list)) { - return false; - } - - auto has_duplicate_selectors = [](const vector<Entry>& entries) { - hashset<string> seen; - foreach (const Entry& entry, entries) { - Entry no_access = entry; - no_access.access.write = false; - no_access.access.read = false; - no_access.access.mknod = false; - string item = stringify(no_access); - if (seen.contains(item)) { - return true; - } - seen.insert(item); - } - return false; - }; - - if ( - has_duplicate_selectors(allow_list) || has_duplicate_selectors(deny_list)) { - return false; - } - - auto has_encompassed_entries = [](const vector<Entry>& entries) { - foreach (const Entry& entry, entries) { - foreach (const Entry& other, entries) { - if ( - (!cgroups::devices::operator==(entry, other)) && - other.encompasses(entry)) { - return true; - } - } - } - return false; - }; - - if ( - has_encompassed_entries(allow_list) || has_encompassed_entries(deny_list)) { - return false; - } - return true; -} - - -Try<DeviceManager::CgroupDeviceAccess> -DeviceManager::CgroupDeviceAccess::create( - const vector<Entry>& allow_list, const vector<Entry>& deny_list) -{ - CgroupDeviceAccess res(allow_list, deny_list); - if (!res.normalized()) { - return Error( - "Failed to create CgroupDeviceAccess: The allow or deny list is not " - "normalized."); - } - return res; -} - - -bool DeviceManager::CgroupDeviceAccess::is_access_granted( - const Entry& query) const -{ - CHECK(normalized()); - bool encompassed_by_allow = false; - foreach (const Entry& allow, allow_list) { - if (allow.encompasses(query)) { - encompassed_by_allow = true; - } - } - if (!encompassed_by_allow) { - return false; - } - foreach (const Entry& deny, deny_list) { - if ( - deny.selector.encompasses(query.selector) && - deny.access.overlaps(query.access)) { - return false; - } - } - return true; -} - - Try<DeviceManager*> DeviceManager::create(const Flags& flags) { return new DeviceManager( diff --git a/src/slave/containerizer/device_manager/device_manager.hpp b/src/slave/containerizer/device_manager/device_manager.hpp index 8b78618b4..7c8523d8b 100644 --- a/src/slave/containerizer/device_manager/device_manager.hpp +++ b/src/slave/containerizer/device_manager/device_manager.hpp @@ -68,18 +68,6 @@ public: { std::vector<cgroups::devices::Entry> allow_list; std::vector<cgroups::devices::Entry> deny_list; - bool normalized() const; - // A device access is granted if it is encompassed by an allow entry - // and does not have access overlaps with any deny entry. - bool is_access_granted(const cgroups::devices::Entry& entry) const; - static Try<CgroupDeviceAccess> create( - const std::vector<cgroups::devices::Entry>& allow_list, - const std::vector<cgroups::devices::Entry>& deny_list); - - private: - CgroupDeviceAccess( - const std::vector<cgroups::devices::Entry>& allow_list, - const std::vector<cgroups::devices::Entry>& deny_list); }; static Try<DeviceManager*> create(const Flags& flags); diff --git a/src/tests/containerizer/cgroups2_tests.cpp b/src/tests/containerizer/cgroups2_tests.cpp index e01b290b0..39f17460c 100644 --- a/src/tests/containerizer/cgroups2_tests.cpp +++ b/src/tests/containerizer/cgroups2_tests.cpp @@ -749,48 +749,6 @@ INSTANTIATE_TEST_CASE_P( {os::DEV_NULL, O_RDWR}, }})); -TEST_F(Cgroups2Test, ROOT_CGROUPS2_ConfigureValidation) -{ - const string& cgroup = TEST_CGROUP; - // Error if there is empty accesses in any entry - devices::Entry empty_entry = CHECK_NOTERROR(devices::Entry::parse("c 1:3 w")); - empty_entry.access.read = false; - empty_entry.access.write = false; - empty_entry.access.mknod = false; - vector<devices::Entry> allow = {empty_entry}; - vector<devices::Entry> deny = { - CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}; - Try<Nothing> configure_status = devices::configure(cgroup, allow, deny); - EXPECT_ERROR(configure_status); - EXPECT_EQ( - string("Failed to validate arguments: allow or deny lists are not ") + - string("normalized."), - configure_status.error()); - // Error if there is any entry that shares the same type, major, and minor - // numbers with another entry in the same list - allow = { - CHECK_NOTERROR(devices::Entry::parse("b 3:1 rw")), - CHECK_NOTERROR(devices::Entry::parse("b 3:1 m"))}; - deny = {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}; - configure_status = devices::configure(cgroup, allow, deny); - EXPECT_ERROR(configure_status); - EXPECT_EQ( - string("Failed to validate arguments: allow or deny lists are not ") + - string("normalized."), - configure_status.error()); - // Error if there are entries which are encompassed by another on the same - // list - allow = { - CHECK_NOTERROR(devices::Entry::parse("c *:* rw")), - CHECK_NOTERROR(devices::Entry::parse("c 3:1 w"))}; - deny = {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}; - configure_status = devices::configure(cgroup, allow, deny); - EXPECT_ERROR(configure_status); - EXPECT_EQ( - string("Failed to validate arguments: allow or deny lists are not ") + - string("normalized."), - configure_status.error()); -} TEST_F(Cgroups2Test, ROOT_CGROUPS2_AtomicReplace) { diff --git a/src/tests/device_manager_tests.cpp b/src/tests/device_manager_tests.cpp index 999273c46..54d464e97 100644 --- a/src/tests/device_manager_tests.cpp +++ b/src/tests/device_manager_tests.cpp @@ -109,101 +109,6 @@ TEST(NonWildcardEntry, NonWildcardFromWildcard) } -TEST(CgroupDeviceAccessTest, IsAccessGrantedTest) -{ - // Character devices with major minor numbrs 1:3 can do write only: - DeviceManager::CgroupDeviceAccess cgroup_device_access = - CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( - {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}, {})); - EXPECT_TRUE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w"))); - EXPECT_FALSE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 rw"))); - EXPECT_FALSE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("b 1:3 w"))); - // Character devices with minor number 3 can do write only: - cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( - {CHECK_NOTERROR(devices::Entry::parse("c *:3 w"))}, {})); - EXPECT_TRUE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 4:3 w"))); - EXPECT_TRUE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c *:3 w"))); - // Character devices with major number 5 can do write only: - cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( - {CHECK_NOTERROR(devices::Entry::parse("c 5:* w"))}, {})); - EXPECT_TRUE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:* w"))); - EXPECT_TRUE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:2 w"))); - // All devices will match the catch-all and can perform all operations: - cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( - {CHECK_NOTERROR(devices::Entry::parse("a *:* rwm"))}, {})); - EXPECT_TRUE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 6:2 w"))); - // Deny all accesses to character device with major and numbers 1:3 - cgroup_device_access.deny_list = {*devices::Entry::parse("c 1:3 rwm")}; - cgroup_device_access = CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create( - {}, {CHECK_NOTERROR(devices::Entry::parse("c 1:3 rwm"))})); - EXPECT_FALSE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w"))); - EXPECT_FALSE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 rw"))); - - cgroup_device_access.allow_list = {}; - cgroup_device_access.deny_list = {}; - // Entry should be denied if encompassed by an entry in allow_list and - // overlaps with entry in deny_list - cgroup_device_access.allow_list.push_back(*devices::Entry::parse("c 1:3 rw")); - cgroup_device_access.deny_list.push_back(*devices::Entry::parse("c 1:3 w")); - EXPECT_FALSE( - cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 rw"))); -} - -TEST(CgroupDeviceAccessTest, NormalizedTest) -{ - // Not normalized if there is an entry with no accesses specified - devices::Entry empty_entry; - empty_entry.selector.type = devices::Entry::Selector::Type::CHARACTER; - empty_entry.selector.major = 1; - empty_entry.selector.minor = 3; - empty_entry.access.read = false; - empty_entry.access.write = false; - empty_entry.access.mknod = false; - Try<DeviceManager::CgroupDeviceAccess> cgroup_device_access = - DeviceManager::CgroupDeviceAccess::create( - {empty_entry}, {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}); - EXPECT_ERROR(cgroup_device_access); - EXPECT_EQ( - "Failed to create CgroupDeviceAccess: The allow or deny list is not " - "normalized.", - cgroup_device_access.error()); - - // Not normalized if there is any entry that shares the same type, major, and - // minor numbers with another entry in the same list - cgroup_device_access = DeviceManager::CgroupDeviceAccess::create( - {CHECK_NOTERROR(devices::Entry::parse("b 3:1 rw")), - CHECK_NOTERROR(devices::Entry::parse("b 3:1 m"))}, - {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}); - EXPECT_ERROR(cgroup_device_access); - EXPECT_EQ( - "Failed to create CgroupDeviceAccess: The allow or deny list is not " - "normalized.", - cgroup_device_access.error()); - - // Not normalized if there are entries which are encompassed by another on the - // same list - cgroup_device_access = DeviceManager::CgroupDeviceAccess::create( - {CHECK_NOTERROR(devices::Entry::parse("c *:* rw")), - CHECK_NOTERROR(devices::Entry::parse("c 3:1 w"))}, - {CHECK_NOTERROR(devices::Entry::parse("c 1:3 w"))}); - EXPECT_ERROR(cgroup_device_access); - EXPECT_EQ( - "Failed to create CgroupDeviceAccess: The allow or deny list is not " - "normalized.", - cgroup_device_access.error()); -} - - TEST_F(DeviceManagerTest, ROOT_DeviceManagerConfigure_Normal) { typedef std::pair<string, int> OpenArgs;
