You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2024/07/26 20:34:57 UTC
(mesos) branch master updated (acb5a8a50 -> fd17efe34)
This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
from acb5a8a50 [ebpf] Correct ebpf deny block behavior.
new 45d290aef [cgroups2] Add allow / deny list normalization validation.
new fd17efe34 [cgroups2] Clarify device documentation.
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
src/linux/cgroups2.cpp | 12 ++
src/linux/cgroups2.hpp | 12 +-
.../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, 303 insertions(+), 8 deletions(-)
(mesos) 01/02: [cgroups2] Add allow / deny list normalization validation.
Posted by bm...@apache.org.
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 45d290aeff6912c8e6a4b1a7358c4e9772c447b4
Author: Jason Zhou <ja...@gmail.com>
AuthorDate: Fri Jul 26 01:45:30 2024 -0400
[cgroups2] Add allow / deny list normalization validation.
Currently we assume that a device state is normalized before using it
for generating ebpf files. However, we have not been enforcing these
constraints on the device access state.
We enforce some basic validation on cgroups2::configure on the state
to ensure that we are able to generate a correct ebpf program. If the
lists are not normalized, we generate incorrect programs!
An allow or deny list is 'normalized' iff everything below are true:
1. No entries have empty accesses specified.
2. No two entries on the same list can have the same selector
(type, major & minor numbers).
3. No two entries on the same list can be encompassed by the other
entry. See Entry::encompassed.
This patch adds helpers to check if a device state is normalized,
and will only allow users to create new CgroupDeviceAccess instances
using a helper that checks that the allow and deny lists are normalized.
A new helper function is added to check if an entry would be granted
access, and requires the state to be normalized.
Review: https://reviews.apache.org/r/75099/
---
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, 301 insertions(+), 8 deletions(-)
diff --git a/src/linux/cgroups2.cpp b/src/linux/cgroups2.cpp
index ac4678261..2c8290727 100644
--- a/src/linux/cgroups2.cpp
+++ b/src/linux/cgroups2.cpp
@@ -41,6 +41,8 @@
#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;
@@ -1465,6 +1467,16 @@ 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 accaebdad..a814bc03c 100644
--- a/src/linux/cgroups2.hpp
+++ b/src/linux/cgroups2.hpp
@@ -434,7 +434,15 @@ 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.
+// 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.
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 4c9b86393..6f013d4bc 100644
--- a/src/slave/containerizer/device_manager/device_manager.cpp
+++ b/src/slave/containerizer/device_manager/device_manager.cpp
@@ -91,9 +91,21 @@ 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()) {
@@ -124,10 +136,21 @@ public:
}
}
- device_access_per_cgroup[cgroup] = DeviceManager::apply_diff(
- device_access_per_cgroup[cgroup],
- non_wildcard_additions,
- non_wildcard_removals);
+ 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 + "'");
+ }
+ }
Try<Nothing> commit = commit_device_access_changes(cgroup);
if (commit.isError()) {
@@ -149,7 +172,7 @@ public:
{
return device_access_per_cgroup.contains(cgroup)
? device_access_per_cgroup.at(cgroup)
- : DeviceManager::CgroupDeviceAccess();
+ : CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {}));
}
private:
@@ -174,6 +197,107 @@ 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 7c8523d8b..8b78618b4 100644
--- a/src/slave/containerizer/device_manager/device_manager.hpp
+++ b/src/slave/containerizer/device_manager/device_manager.hpp
@@ -68,6 +68,18 @@ 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 39f17460c..e01b290b0 100644
--- a/src/tests/containerizer/cgroups2_tests.cpp
+++ b/src/tests/containerizer/cgroups2_tests.cpp
@@ -749,6 +749,48 @@ 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 54d464e97..999273c46 100644
--- a/src/tests/device_manager_tests.cpp
+++ b/src/tests/device_manager_tests.cpp
@@ -109,6 +109,101 @@ 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;
(mesos) 02/02: [cgroups2] Clarify device documentation.
Posted by bm...@apache.org.
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 fd17efe3402fc859efc63d3cd32658d1ec61a015
Author: Benjamin Mahler <bm...@apache.org>
AuthorDate: Fri Jul 26 16:34:34 2024 -0400
[cgroups2] Clarify device documentation.
---
src/linux/cgroups2.hpp | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/linux/cgroups2.hpp b/src/linux/cgroups2.hpp
index a814bc03c..a06534432 100644
--- a/src/linux/cgroups2.hpp
+++ b/src/linux/cgroups2.hpp
@@ -436,13 +436,15 @@ using cgroups::devices::Entry;
// 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
+// 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.
+//
+// 1. Entries must not have empty accesses specified.
+// 2. No two entries on the same list can have the same selector
+// (type, major & minor numbers).
+// 3. No two entries on the same list can be encompassed by the other entry
+// (See Entry::encompassed).
Try<Nothing> configure(
const std::string& cgroup,
const std::vector<Entry>& allow,