You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jason Zhou <ja...@gmail.com> on 2024/06/03 19:45:13 UTC
Review Request 75026: [cgroups2] update device configuration EBPF program generator
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
This means that we need to change our previous logic that exits on the first match to now check both the allow and deny list of a cgroup
before determining whether access may be granted.
This patch implements the logic change, and removes functions that are no longer necessary for the DeviceProgram class.
We now pass the entire allow and deny list to a configure function inside the DeviceProgram object, which will create a ebpf program
with the updated logic and attempt to attach it to the cgroup.
Diffs
-----
src/linux/cgroups2.cpp 9e2ca2207a4e407fb6b07b6fbf709bbc3b397673
Diff: https://reviews.apache.org/r/75026/diff/1/
Testing
-------
All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers
Thanks,
Jason Zhou
Re: Review Request 75026: [cgroups2] update device configuration EBPF program generator
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/#review226625
-----------------------------------------------------------
> In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
> This means that we need to change our previous logic that exits on the first match to now check both the allow and deny list of a cgroup
> before determining whether access may be granted.
Could you make the desired vs current behavior more clear? And explain the motivation for the desired behavior?
src/linux/cgroups2.hpp
Lines 436-439 (patched)
<https://reviews.apache.org/r/75026/#comment314889>
The second sentence seems redundant here, the first one is enough although it could be more clear (e.g. "be on" -> "match an entry in"
src/linux/cgroups2.cpp
Lines 1091-1096 (original), 1091-1096 (patched)
<https://reviews.apache.org/r/75026/#comment314893>
maybe a note here that we use r6 for allow checking purposes?
src/linux/cgroups2.cpp
Lines 1120-1122 (original), 1118-1135 (patched)
<https://reviews.apache.org/r/75026/#comment314894>
Maybe explain this a bit more visually? E.g.
// ALLOW BLOCK
// CHECK R6 TO JUMP TO DENY BLOCKS
// ALLOW BLOCK
// CHECK R6 TO JUMP TO DENY BLOCKS
// ...
// DENY BLOCK
// EXIT EARLY
// DENY BLOCK
// EXIT EARLY
// ...
// CHECK ALLOWED AND NOT DENIED
src/linux/cgroups2.cpp
Lines 1164 (patched)
<https://reviews.apache.org/r/75026/#comment314897>
hm.. why do we need register 6 if we're trying to jump past the allow blocks..? seems we can use the allow and deny trailer blocks to assume that no allows or no denies matched
src/linux/cgroups2.cpp
Lines 1165 (patched)
<https://reviews.apache.org/r/75026/#comment314896>
we're jumping here if an allow matches early then jumping again?
src/linux/cgroups2.cpp
Line 1125 (original), 1176-1177 (patched)
<https://reviews.apache.org/r/75026/#comment314895>
this looks like another case of non-local reasoning..? `_add_device_checks` needs to know that this adds two instructions afterwards to correctly jump past the exit here? It would be nice if we could avoid that by passing in the jump point?
this seems to be accomplished by the `1 +` prefix inside the function?
maybe these 2 instruction trailers can go inside the function if we jump to the end of the deny checks? that would also be more symmetric?
src/linux/cgroups2.cpp
Lines 1203 (patched)
<https://reviews.apache.org/r/75026/#comment314890>
nit: no need for the leading underscore
src/linux/cgroups2.cpp
Lines 1143-1151 (original), 1210-1218 (patched)
<https://reviews.apache.org/r/75026/#comment314891>
Maybe split 5 into 2 sections and re-word the explanation here to clarify how it works now?
src/linux/cgroups2.cpp
Lines 1230 (patched)
<https://reviews.apache.org/r/75026/#comment314892>
hm.. the member variable tracking this feels quite tricky and requires the reader to know that the caller MUST call this with all the allows first for this variable to make sense here
Maybe instead we can just take in an argument of the deny block jump, and this returns the size of the code generated (or maybe the caller can infer that from the program size change?)
- Benjamin Mahler
On June 3, 2024, 7:45 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75026/
> -----------------------------------------------------------
>
> (Updated June 3, 2024, 7:45 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
> This means that we need to change our previous logic that exits on the first match to now check both the allow and deny list of a cgroup
> before determining whether access may be granted.
>
> This patch implements the logic change, and removes functions that are no longer necessary for the DeviceProgram class.
> We now pass the entire allow and deny list to a configure function inside the DeviceProgram object, which will create a ebpf program
> with the updated logic and attempt to attach it to the cgroup.
>
>
> Diffs
> -----
>
> src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
> src/linux/cgroups2.cpp 9e2ca2207a4e407fb6b07b6fbf709bbc3b397673
> src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
>
>
> Diff: https://reviews.apache.org/r/75026/diff/4/
>
>
> Testing
> -------
>
> All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
>
>
> Thanks,
>
> Jason Zhou
>
>
Re: Review Request 75026: [cgroups2] Fix allow deny semantics for device access.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 10, 2024, 8:18 p.m., Benjamin Mahler wrote:
> > Ship It!
```
diff --git a/src/linux/cgroups2.cpp b/src/linux/cgroups2.cpp
index 9ae8df387..59c212ebc 100644
--- a/src/linux/cgroups2.cpp
+++ b/src/linux/cgroups2.cpp
@@ -1164,7 +1164,8 @@ public:
// |Exit instruction to deny access |
// +-------------------------------------------------------------+
static Try<ebpf::Program> build(
- const vector<Entry>& allow, const vector<Entry>& deny)
+ const vector<Entry>& allow,
+ const vector<Entry>& deny)
{
// The BPF_PROG_TYPE_CGROUP_DEVICE program takes in
// `struct bpf_cgroup_dev_ctx*` as input. We extract the fields into
@@ -1209,25 +1210,22 @@ public:
return program;
}
- auto allow_block_trailer =
- [](short jmp_size_to_deny_section) -> vector<bpf_insn> {
- return {BPF_JMP_A(jmp_size_to_deny_section)};
+ auto allow_block_trailer = [](short jmp_size_to_deny_section) {
+ return vector<bpf_insn>({BPF_JMP_A(jmp_size_to_deny_section)});
};
- auto allow_section_trailer = []() -> vector<bpf_insn> {
- return {BPF_EXIT_INSN()};
+ auto allow_section_trailer = []() {
+ return vector<bpf_insn>({BPF_EXIT_INSN()});
};
- auto deny_block_trailer = []() -> vector<bpf_insn> {
- return {BPF_EXIT_INSN()};
+ auto deny_block_trailer = []() {
+ return vector<bpf_insn>({BPF_EXIT_INSN()});
};
- auto deny_section_trailer = []() -> vector<bpf_insn> {
- return {
+ auto deny_section_trailer = []() {
+ return vector<bpf_insn>({
BPF_MOV64_IMM(BPF_REG_0, ALLOW_ACCESS),
BPF_EXIT_INSN(),
- };
+ });
};
- // We will use the catch-all for allow to determine if we should skip
- // generating the allow section.
bool allow_catch_all = [&allow]() {
foreach (const Entry& entry, allow) {
if (entry.is_catch_all()) {
@@ -1238,24 +1236,25 @@ public:
}();
// We will only add the code for the allow section if there is no catch-all
- // allow entry present. If there is a catch-all, we wll skip everything
+ // allow entry present. If there is a catch-all, we will skip everything
// in the allow section, including exit instruction at the end,
// since we just need to check if the device is explicitly denied.
if (!allow_catch_all) {
// We calculate the total jump distance to skip over trailer instructions
// at the end of the allow section, we initialize jump size to length of
- // said instructions, then add the lengths of individual allow blocks to it.
+ // said instructions, then add the lengths of individual allow blocks.
short start_of_deny_jmp_size = allow_section_trailer().size();
vector<vector<bpf_insn>> allow_device_check_blocks = {};
short allow_block_trailer_size = allow_block_trailer(0).size();
foreach (const Entry& entry, allow) {
- vector<bpf_insn> device_check_block =
+ vector<bpf_insn> allow_block =
add_device_checks(entry, allow_block_trailer_size);
- allow_device_check_blocks.push_back(device_check_block);
- start_of_deny_jmp_size +=
- device_check_block.size() + allow_block_trailer_size;
+ allow_device_check_blocks.push_back(allow_block);
+
+ start_of_deny_jmp_size += allow_block.size() + allow_block_trailer_size;
}
+
foreach (vector<bpf_insn>& allow_block, allow_device_check_blocks) {
start_of_deny_jmp_size -=
(allow_block.size() + allow_block_trailer_size);
@@ -1278,10 +1277,9 @@ public:
// To reach this block, we must have matched with an entry in allow
// to jump over the exit instruction at the end of allow blocks,
- // or there is a catch-all in allow.
- // We will also have to have not matched with any of the deny
- // entries to avoid their exit instructions. Meaning that
- // the device is on the allow list, and not on the deny list.
+ // or there is a catch-all in allow. We will also have to have not
+ // matched with any of the deny entries to avoid their exit instructions.
+ // Meaning that the device is on the allow list, and not on the deny list.
// Hence, we grant them access.
program.append(deny_section_trailer());
@@ -1290,7 +1288,8 @@ public:
private:
static vector<bpf_insn> add_device_checks(
- const Entry& entry, short trailer_length)
+ const Entry& entry,
+ short trailer_length)
{
// We create a block of bytecode with the format:
// 1. Major Version Check
@@ -1307,34 +1306,32 @@ private:
// 2. One of (1,2,3,4) does not match the requested access and we skip
// the rest of the current block
- vector<bpf_insn> device_check_block = {};
if (entry.is_catch_all()) {
- return device_check_block;
+ return {};
}
- auto check_major_instructions =
- [](short jmp_size, int major) -> vector<bpf_insn> {
- return {
+ auto check_major_instructions = [](short jmp_size, int major) {
+ return vector<bpf_insn>({
BPF_JMP_IMM(
BPF_JNE, BPF_REG_4, major, jmp_size),
- };
+ });
};
- auto check_minor_instructions =
- [](short jmp_size, int minor) -> vector<bpf_insn> {
- return {
+ auto check_minor_instructions = [](short jmp_size, int minor) {
+ return vector<bpf_insn>({
BPF_JMP_IMM(
BPF_JNE, BPF_REG_5, minor, jmp_size),
- };
+ });
};
auto check_access_instructions =
- [](short jmp_size, const Entry::Access& access) -> vector<bpf_insn> {
+ [](short jmp_size, const Entry::Access& access)
+ {
int bpf_access = 0;
bpf_access |= access.read ? BPF_DEVCG_ACC_READ : 0;
bpf_access |= access.write ? BPF_DEVCG_ACC_WRITE : 0;
bpf_access |= access.mknod ? BPF_DEVCG_ACC_MKNOD : 0;
- return {
+ return vector<bpf_insn>({
BPF_MOV32_REG(BPF_REG_1, BPF_REG_3),
BPF_ALU32_IMM(BPF_AND, BPF_REG_1, bpf_access),
BPF_JMP_REG(
@@ -1342,7 +1339,7 @@ private:
BPF_REG_1,
BPF_REG_3,
static_cast<short>(jmp_size - 2)),
- };
+ });
};
auto check_type_instructions =
@@ -1383,6 +1380,7 @@ private:
// instruction to land at the beginning of the next entry-block
nxt_blk_jmp_size -= 1;
+ vector<bpf_insn> device_check_block = {};
// 1. Check major version (r4) against entry.
if (check_major) {
```
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/#review226649
-----------------------------------------------------------
On July 9, 2024, 11:30 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75026/
> -----------------------------------------------------------
>
> (Updated July 9, 2024, 11:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, the EBPF program we generate has the behavior where the deny
> list has no effect, as we will allow device access iff the device
> matched with an allow entry.
>
> Instead we want to grant access to a device iff it is in a cgroup's
> allow list *and not in its deny list.*
>
> This means that we need to change our existing logic, which exits on the
> first match. It is not our desired behavior because the current EBPF
> program construction logic puts the allow-device checks before the
> deny-device checks, meaning that if a device is on both allow and deny
> lists for a cgroup, it will be granted access.
>
> This change revamps the EBPF program construction to now check both the
> allow and deny list of a cgroup before determining whether access may be
> granted. Specifically, if a device is matched with an entry inside the
> allow list, we will also be checking if it matches with any entry on
> the deny list, and deny the device's access if that is the case.
>
> We also avoid generating specific parts of the EBPF program code to
> avoid creating unreachable code, explanations with a diagram are
> attached above the cgroups2::devices::DeviceProgram::build function.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 9be53e31e46129f019deeda5b7690f3b4e4004ce
> src/linux/cgroups.cpp c1272fbcac9926a378b0d8f69263be214fc21c5e
> src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
> src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
> src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
>
>
> Diff: https://reviews.apache.org/r/75026/diff/12/
>
>
> Testing
> -------
>
> All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
>
>
> Thanks,
>
> Jason Zhou
>
>
Re: Review Request 75026: [cgroups2] Fix allow deny semantics for device access.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/#review226649
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On July 9, 2024, 11:30 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75026/
> -----------------------------------------------------------
>
> (Updated July 9, 2024, 11:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, the EBPF program we generate has the behavior where the deny
> list has no effect, as we will allow device access iff the device
> matched with an allow entry.
>
> Instead we want to grant access to a device iff it is in a cgroup's
> allow list *and not in its deny list.*
>
> This means that we need to change our existing logic, which exits on the
> first match. It is not our desired behavior because the current EBPF
> program construction logic puts the allow-device checks before the
> deny-device checks, meaning that if a device is on both allow and deny
> lists for a cgroup, it will be granted access.
>
> This change revamps the EBPF program construction to now check both the
> allow and deny list of a cgroup before determining whether access may be
> granted. Specifically, if a device is matched with an entry inside the
> allow list, we will also be checking if it matches with any entry on
> the deny list, and deny the device's access if that is the case.
>
> We also avoid generating specific parts of the EBPF program code to
> avoid creating unreachable code, explanations with a diagram are
> attached above the cgroups2::devices::DeviceProgram::build function.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 9be53e31e46129f019deeda5b7690f3b4e4004ce
> src/linux/cgroups.cpp c1272fbcac9926a378b0d8f69263be214fc21c5e
> src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
> src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
> src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
>
>
> Diff: https://reviews.apache.org/r/75026/diff/12/
>
>
> Testing
> -------
>
> All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
>
>
> Thanks,
>
> Jason Zhou
>
>
Re: Review Request 75026: [cgroups2] Fix allow deny semantics for device access.
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/
-----------------------------------------------------------
(Updated July 9, 2024, 11:30 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Rewrite the configure function into static function
Summary (updated)
-----------------
[cgroups2] Fix allow deny semantics for device access.
Repository: mesos
Description (updated)
-------
Currently, the EBPF program we generate has the behavior where the deny
list has no effect, as we will allow device access iff the device
matched with an allow entry.
Instead we want to grant access to a device iff it is in a cgroup's
allow list *and not in its deny list.*
This means that we need to change our existing logic, which exits on the
first match. It is not our desired behavior because the current EBPF
program construction logic puts the allow-device checks before the
deny-device checks, meaning that if a device is on both allow and deny
lists for a cgroup, it will be granted access.
This change revamps the EBPF program construction to now check both the
allow and deny list of a cgroup before determining whether access may be
granted. Specifically, if a device is matched with an entry inside the
allow list, we will also be checking if it matches with any entry on
the deny list, and deny the device's access if that is the case.
We also avoid generating specific parts of the EBPF program code to
avoid creating unreachable code, explanations with a diagram are
attached above the cgroups2::devices::DeviceProgram::build function.
Diffs (updated)
-----
src/linux/cgroups.hpp 9be53e31e46129f019deeda5b7690f3b4e4004ce
src/linux/cgroups.cpp c1272fbcac9926a378b0d8f69263be214fc21c5e
src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
Diff: https://reviews.apache.org/r/75026/diff/11/
Changes: https://reviews.apache.org/r/75026/diff/10-11/
Testing
-------
All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
Thanks,
Jason Zhou
Re: Review Request 75026: [cgroups2] update device configuration EBPF program generator
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/
-----------------------------------------------------------
(Updated July 8, 2024, 11:18 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Reorganize logic for better readability
Repository: mesos
Description (updated)
-------
In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
This means that we need to change our existing logic, which exits on the first match. It is not our desired behavior because the current EBPF program construction logic puts the allow-device checks before the deny-device checks, meaning that if a device is on both allow and deny lists for a cgroup, it will be granted access.
This change revamps the EBPF program construction to now check both the allow and deny list of a cgroup before determining whether access may be granted. Specifically, if a device is matched with an entry inside the allow list, we will also be checking if it matches with any entry on the deny list, and deny the device's access if that is the case.
We also avoid generating specific parts of the EBPF program code to avoid creating unreachable code, explanations with a diagram are attached above the cgroups2::devices::DeviceProgram::generate_ebpf_insn function
This patch implements the logic change, and removes functions that are no longer necessary for the DeviceProgram class.
We now pass the entire allow and deny list to a configure function inside the DeviceProgram object, which will create a ebpf program
with the updated logic and attempt to attach it to the cgroup.
Diffs (updated)
-----
src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
Diff: https://reviews.apache.org/r/75026/diff/9/
Changes: https://reviews.apache.org/r/75026/diff/8-9/
Testing
-------
All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
Thanks,
Jason Zhou
Re: Review Request 75026: [cgroups2] update device configuration EBPF program generator
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/
-----------------------------------------------------------
(Updated July 8, 2024, 6:57 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
remove trailing whitespace
Repository: mesos
Description (updated)
-------
In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
This means that we need to change our existing logic, which exits on the first match. It is not our desired behavior because the current EBPF program construction logic puts the allow-device checks before the deny-device checks, meaning that if a device is on both allow and deny lists for a cgroup, it will be granted access.
This change revamps the EBPF program construction to now check both the allow and deny list of a cgroup before determining whether access may be granted. Specifically, if a device is matched with an entry inside the allow list, we will also be checking if it matches with any entry on the deny list, and deny the device's access if that is the case.
We also avoid generating specific parts of the EBPF program code to avoid creating unreachable code, explanations with a diagram are attached above the cgroups2::devices::DeviceProgram::configure function
This patch implements the logic change, and removes functions that are no longer necessary for the DeviceProgram class.
We now pass the entire allow and deny list to a configure function inside the DeviceProgram object, which will create a ebpf program
with the updated logic and attempt to attach it to the cgroup.
Diffs (updated)
-----
src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
Diff: https://reviews.apache.org/r/75026/diff/8/
Changes: https://reviews.apache.org/r/75026/diff/7-8/
Testing
-------
All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
Thanks,
Jason Zhou
Re: Review Request 75026: [cgroups2] update device configuration EBPF program generator
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/
-----------------------------------------------------------
(Updated July 4, 2024, 1:29 a.m.)
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description (updated)
-------
In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
This means that we need to change our existing logic, which exits on the first match. It is not our desired behavior because the current EBPF program construction logic puts the allow-device checks before the deny-device checks, meaning that if a device is on both allow and deny lists for a cgroup, it will be granted access.
This change revamps the EBPF program construction to now check both the allow and deny list of a cgroup before determining whether access may be granted. Specifically, if a device is matched with an entry inside the allow list, we will also be checking if it matches with any entry on the deny list, and deny the device's access if that is the case.
We also avoid generating specific parts of the EBPF program code to avoid creating unreachable code, explanations with a diagram are attached above the cgroups2::devices::DeviceProgram::configure function. Note that we still go through the entries and will get their device check code, but will not append them to the ebpf program if specific conditions are fulfilled
This patch implements the logic change, and removes functions that are no longer necessary for the DeviceProgram class.
We now pass the entire allow and deny list to a configure function inside the DeviceProgram object, which will create a ebpf program
with the updated logic and attempt to attach it to the cgroup.
Diffs
-----
src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
Diff: https://reviews.apache.org/r/75026/diff/7/
Testing
-------
All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
Thanks,
Jason Zhou
Re: Review Request 75026: [cgroups2] update device configuration EBPF program generator
Posted by Jason Zhou <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/
-----------------------------------------------------------
(Updated July 3, 2024, 8:15 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Revamp logic to remove reliance on spare register R6, added more tests, improved code generation to avoid unreachable code, added more documentation + diagram for example EBPF file structure
Repository: mesos
Description (updated)
-------
In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
This means that we need to change our existing logic, which exits on the first match. It is not our desired behavior because the current EBPF program construction logic puts the allow-device checks before the deny-device checks, meaning that if a device is on both allow and deny lists for a cgroup, it will be granted access.
This change revamps the EBPF program construction to now check both the allow and deny list of a cgroup before determining whether access may be granted. Specifically, if a device is matched with an entry inside the allow list, we will also be checking if it matches with any entry on the deny list, and deny the device's access if that is the case.
We also avoid generating specific parts of the EBPF program code to avoid creating unreachable code, explanations with a diagram are attached above the cgroups2::devices::DeviceProgram::configure function
This patch implements the logic change, and removes functions that are no longer necessary for the DeviceProgram class.
We now pass the entire allow and deny list to a configure function inside the DeviceProgram object, which will create a ebpf program
with the updated logic and attempt to attach it to the cgroup.
Diffs (updated)
-----
src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
src/linux/cgroups2.cpp d1fc2638cdf9a07199f90952e04998072021011c
src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
Diff: https://reviews.apache.org/r/75026/diff/5/
Changes: https://reviews.apache.org/r/75026/diff/4-5/
Testing
-------
All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
Thanks,
Jason Zhou
Re: Review Request 75026: [cgroups2] update device configuration EBPF program generator
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75026/#review226626
-----------------------------------------------------------
src/tests/containerizer/cgroups2_tests.cpp
Lines 669-671 (patched)
<https://reviews.apache.org/r/75026/#comment314898>
might want to test some more cases here:
* allow all char devices but deny one?
- Benjamin Mahler
On June 3, 2024, 7:45 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75026/
> -----------------------------------------------------------
>
> (Updated June 3, 2024, 7:45 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In cgroups2, we want our EBPF file to only grant access to a device if it is in a cgroup's allow list and not in its deny list.
> This means that we need to change our previous logic that exits on the first match to now check both the allow and deny list of a cgroup
> before determining whether access may be granted.
>
> This patch implements the logic change, and removes functions that are no longer necessary for the DeviceProgram class.
> We now pass the entire allow and deny list to a configure function inside the DeviceProgram object, which will create a ebpf program
> with the updated logic and attempt to attach it to the cgroup.
>
>
> Diffs
> -----
>
> src/linux/cgroups2.hpp 64254d04f65128713cf3489b25bcba42590b6020
> src/linux/cgroups2.cpp 9e2ca2207a4e407fb6b07b6fbf709bbc3b397673
> src/tests/containerizer/cgroups2_tests.cpp cb1e229f7f40aa71f57417c33fccb2cfb313a1f5
>
>
> Diff: https://reviews.apache.org/r/75026/diff/4/
>
>
> Testing
> -------
>
> All Cgroups2 tests pass i.e. the generated ebpf files pass the verifiers, tests added for new behavior for when device is on both allow and deny list, and test that mismatched entries are ignored.
>
>
> Thanks,
>
> Jason Zhou
>
>