-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/75080/#review226675
-----------------------------------------------------------
Fix it, then Ship it!
local edits:
```
diff --git a/src/linux/ebpf.cpp b/src/linux/ebpf.cpp
index 35ae701d1..e74768e80 100644
--- a/src/linux/ebpf.cpp
+++ b/src/linux/ebpf.cpp
@@ -26,9 +26,11 @@
#include "stout/check.hpp"
#include "stout/error.hpp"
+#include "stout/none.hpp"
#include "stout/nothing.hpp"
#include "stout/os/close.hpp"
#include "stout/os/open.hpp"
+#include "stout/stringify.hpp"
#include "stout/try.hpp"
using std::string;
@@ -126,40 +128,43 @@ Try<int> bpf_get_fd_by_id(uint32_t prog_id)
// Attaches the eBPF program identified by the provided fd to a cgroup.
-// If there is an existing ebpf program at the cgroup, we will replace it with
-// the ebpf program.
-Try<Nothing> attach(const string& cgroup, int fd)
+// If there is an existing ebpf program at the cgroup, we will replace
+// it atomically with the provided ebpf program.
+Try<Nothing> attach(const string& cgroup, int new_program_fd)
{
string cgroup_path = ::cgroups2::path(cgroup);
+
Try<int> cgroup_fd =
os::open(cgroup_path, O_DIRECTORY | O_RDONLY | O_CLOEXEC);
if (cgroup_fd.isError()) {
- return Error("Failed to open '" + cgroup_path + "': " + cgroup_fd.error());
+ return Error("Failed to open cgroup: " + cgroup_fd.error());
}
Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(cgroup_path);
if (attached.isError()) {
- return Error(
- "Failed to retrieve attached bpf device programs for cgroup at" +
- cgroup_path + " " + attached.error());
+ return Error("Failed to retrieve attached bpf device programs: "
+ + attached.error());
+ } else if (attached->size() > 1) {
+ return Error("Detected multiple (" + stringify(attached->size()) + ")"
+ " BPF_CGROUP_DEVICE attached to cgroup");
}
- Option<int> replace_program_fd;
+ Result<int> old_program_fd = None();
if (attached->size() == 1) {
- Try<int> _replace_program_fd = bpf_get_fd_by_id(attached->at(0));
- replace_program_fd = *_replace_program_fd;
- } else if (attached->size() > 1) {
- return Error(
- "Detected multiple BPF_CGROUP_DEVICE attached to cgroup at: " +
- cgroup_path);
+ uint32_t old_program_id = attached->at(0);
+ old_program_fd = bpf_get_fd_by_id(old_program_id);
+ if (old_program_fd.isError()) {
+ return Error("Failed to open existing program fd for id "
+ + stringify(old_program_id) + ": " +
old_program_fd.error());
+ }
}
bpf_attr attr;
memset(&attr, 0, sizeof(attr));
attr.attach_type = BPF_CGROUP_DEVICE;
attr.target_fd = *cgroup_fd;
- attr.attach_bpf_fd = fd;
+ attr.attach_bpf_fd = new_program_fd;
// BPF_F_ALLOW_MULTI allows multiple eBPF programs to be attached to a single
// cgroup and determines how the programs up and down the hierarchy will run.
@@ -187,16 +192,16 @@ Try<Nothing> attach(const string& cgroup, int fd)
// For full details, see:
//
https://elixir.bootlin.com/linux/v6.7.9/source/include/uapi/linux/bpf.h#L1090
attr.attach_flags = BPF_F_ALLOW_MULTI;
- if (replace_program_fd.isSome()) {
+ if (old_program_fd.isSome()) {
attr.attach_flags |= BPF_F_REPLACE;
- attr.replace_bpf_fd = *replace_program_fd;
+ attr.replace_bpf_fd = *old_program_fd;
}
Try<int, ErrnoError> result = bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
os::close(*cgroup_fd);
- if (replace_program_fd.isSome()) {
- os::close(*replace_program_fd);
+ if (old_program_fd.isSome()) {
+ os::close(*old_program_fd);
}
if (result.isError()) {
@@ -228,7 +233,9 @@ Try<Nothing> attach(const string& cgroup, const Program&
program)
Try<vector<uint32_t>> attached(const string& cgroup)
{
string cgroup_path = ::cgroups2::path(cgroup);
- Try<int> cgroup_fd = os::open(cgroup_path, O_DIRECTORY | O_RDONLY |
O_CLOEXEC);
+
+ Try<int> cgroup_fd =
+ os::open(cgroup_path, O_DIRECTORY | O_RDONLY | O_CLOEXEC);
if (cgroup_fd.isError()) {
return Error("Failed to open '" + cgroup_path + "': " + cgroup_fd.error());
}
@@ -268,7 +275,9 @@ Try<vector<uint32_t>> attached(const string& cgroup)
Try<Nothing> detach(const string& cgroup, uint32_t program_id)
{
string cgroup_path = ::cgroups2::path(cgroup);
- Try<int> cgroup_fd = os::open(cgroup_path, O_DIRECTORY | O_RDONLY |
O_CLOEXEC);
+
+ Try<int> cgroup_fd =
+ os::open(cgroup_path, O_DIRECTORY | O_RDONLY | O_CLOEXEC);
if (cgroup_fd.isError()) {
return Error("Failed to open '" + cgroup_path + "': " + cgroup_fd.error());
}
diff --git a/src/tests/containerizer/cgroups2_tests.cpp
b/src/tests/containerizer/cgroups2_tests.cpp
index 5fd3dc0aa..88bdf0f70 100644
--- a/src/tests/containerizer/cgroups2_tests.cpp
+++ b/src/tests/containerizer/cgroups2_tests.cpp
@@ -632,25 +632,29 @@ INSTANTIATE_TEST_CASE_P(
vector<devices::Entry>{},
vector<devices::Entry>{},
vector<OpenArgs>{},
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDWR}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDWR}}
+ },
// Deny access to /dev/null.
DeviceControllerTestParams{
vector<devices::Entry>{},
vector<devices::Entry>{*devices::Entry::parse("c 1:3 rwm")},
vector<OpenArgs>{},
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}
+ },
// Allow access to /dev/null.
DeviceControllerTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 1:3 rwm")},
vector<devices::Entry>{},
vector<OpenArgs>{{os::DEV_NULL, O_RDWR}},
- vector<OpenArgs>{}},
+ vector<OpenArgs>{}
+ },
// Allow read-only access to /dev/null.
DeviceControllerTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 1:3 r")},
vector<devices::Entry>{},
vector<OpenArgs>{{os::DEV_NULL, O_RDONLY}},
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}}
+ },
// Allow write-only access to /dev/null.
DeviceControllerTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 1:3 w")},
@@ -658,7 +662,8 @@ INSTANTIATE_TEST_CASE_P(
// Write-only allowed.
vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}},
// Read is blocked.
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ },
// Do not allow device if it's on both allow and deny list.
DeviceControllerTestParams{
vector<devices::Entry>{
@@ -666,7 +671,8 @@ INSTANTIATE_TEST_CASE_P(
vector<devices::Entry>{*devices::Entry::parse("c 1:3 w")},
vector<OpenArgs>{},
// Write-only is blocked.
- vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}
+ },
// Mismatched entry in deny list is ignored and write access is granted.
DeviceControllerTestParams{
vector<devices::Entry>{*devices::Entry::parse("c 1:3 w")},
@@ -674,7 +680,8 @@ INSTANTIATE_TEST_CASE_P(
// Write-only allowed.
vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}},
// Read is blocked.
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ },
// Access to /dev/null is denied because the allowed access is to
// a different device type with the same major:minor numbers.
DeviceControllerTestParams{
@@ -682,7 +689,8 @@ INSTANTIATE_TEST_CASE_P(
vector<devices::Entry>{},
vector<OpenArgs>{},
// /dev/null is blocked.
- vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ },
// Allow access to all devices.
DeviceControllerTestParams{
vector<devices::Entry>{*devices::Entry::parse("a *:* rwm")},
@@ -691,7 +699,8 @@ INSTANTIATE_TEST_CASE_P(
{os::DEV_NULL, O_WRONLY},
{os::DEV_NULL, O_RDWR},
{os::DEV_NULL, O_RDONLY}},
- vector<OpenArgs>{}},
+ vector<OpenArgs>{}
+ },
// Allow access with catch-all and specified device.
DeviceControllerTestParams{
vector<devices::Entry>{
@@ -701,7 +710,8 @@ INSTANTIATE_TEST_CASE_P(
{os::DEV_NULL, O_WRONLY},
{os::DEV_NULL, O_RDWR},
{os::DEV_NULL, O_RDONLY}},
- vector<OpenArgs>{}},
+ vector<OpenArgs>{}
+ },
// Allow access to all devices except one.
DeviceControllerTestParams{
vector<devices::Entry>{*devices::Entry::parse("a *:* rwm")},
@@ -709,7 +719,8 @@ INSTANTIATE_TEST_CASE_P(
// Read is allowed by catch-all.
vector<OpenArgs>{{os::DEV_NULL, O_RDONLY}},
// Write-only is blocked.
- vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}},
+ vector<OpenArgs>{{os::DEV_NULL, O_WRONLY}}
+ },
// Deny access to all devices.
DeviceControllerTestParams{
vector<devices::Entry>{},
@@ -718,7 +729,8 @@ INSTANTIATE_TEST_CASE_P(
vector<OpenArgs>{
{os::DEV_NULL, O_WRONLY},
{os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}}},
+ {os::DEV_NULL, O_RDONLY}}
+ },
// Deny access using catch-all and specified device.
DeviceControllerTestParams{
vector<devices::Entry>{},
@@ -728,7 +740,8 @@ INSTANTIATE_TEST_CASE_P(
vector<OpenArgs>{
{os::DEV_NULL, O_WRONLY},
{os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}}},
+ {os::DEV_NULL, O_RDONLY}}
+ },
// Catch-all in both allow and deny list.
DeviceControllerTestParams{
vector<devices::Entry>{
@@ -738,17 +751,27 @@ INSTANTIATE_TEST_CASE_P(
vector<OpenArgs>{
{os::DEV_NULL, O_WRONLY},
{os::DEV_NULL, O_RDWR},
- {os::DEV_NULL, O_RDONLY}}}));
+ {os::DEV_NULL, O_RDONLY}},
+ }
+));
-TEST_F(Cgroups2Test, ROOT_CGROUPS2_AtomicReplace) {
+TEST_F(Cgroups2Test, ROOT_CGROUPS2_AtomicReplace)
+{
const string& cgroup = TEST_CGROUP;
const vector<devices::Entry>& allow = {*devices::Entry::parse("c 1:3 w")};
const vector<devices::Entry>& deny = {};
- const vector<OpenArgs>& allowed_accesses = {{os::DEV_NULL, O_WRONLY}};
- const vector<OpenArgs>& blocked_accesses = {{os::DEV_NULL, O_RDWR},
{os::DEV_NULL, O_RDONLY}};
+ const vector<OpenArgs>& allowed_accesses = {
+ {os::DEV_NULL, O_WRONLY}
+ };
+ const vector<OpenArgs>& blocked_accesses = {
+ {os::DEV_NULL, O_RDWR},
+ {os::DEV_NULL, O_RDONLY}
+ };
const vector<devices::Entry>& to_be_replaced_allow = {};
- const vector<devices::Entry>& to_be_replaced_deny =
{*devices::Entry::parse("c 1:3 w")};
+ const vector<devices::Entry>& to_be_replaced_deny = {
+ *devices::Entry::parse("c 1:3 w")
+ };
ASSERT_SOME(cgroups2::create(cgroup));
string path = cgroups2::path(cgroup);
@@ -796,13 +819,13 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_AtomicReplace) {
foreach(const OpenArgs& args, allowed_accesses) {
if (os::open(args.first, args.second).isError()) {
SAFE_EXIT(EXIT_FAILURE, "Expected successful read after detaching"
- " device controller program");
+ " device controller program");
}
}
foreach(const OpenArgs& args, blocked_accesses) {
if (os::open(args.first, args.second).isError()) {
SAFE_EXIT(EXIT_FAILURE, "Expected successful read after detaching"
- " device controller program");
+ " device controller program");
}
}
```
src/linux/ebpf.cpp
Lines 150 (patched)
<https://reviews.apache.org/r/75080/#comment314927>
naked Try de-reference: this can crash the program, we need to check for
error here
src/tests/containerizer/cgroups2_tests.cpp
Lines 734-736 (original), 721-722 (patched)
<https://reviews.apache.org/r/75080/#comment314924>
try to avoid formatting unrelated lines and pull out formatting changes
like this into a pure-formatting patch in front (or behind) your patch
src/tests/containerizer/cgroups2_tests.cpp
Lines 744 (patched)
<https://reviews.apache.org/r/75080/#comment314925>
nit: brace on next line
src/tests/containerizer/cgroups2_tests.cpp
Lines 749 (patched)
<https://reviews.apache.org/r/75080/#comment314926>
nit: max line length is 80
- Benjamin Mahler
On July 11, 2024, 7:08 p.m., Jason Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/75080/
> -----------------------------------------------------------
>
> (Updated July 11, 2024, 7:08 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, if we try to attach device ebpf files to the same cgroup
> multiple times, they will all be attached, and they will all be run
> when a device requests access. This conflicts with our design to have
> one ebpf file per cgroup that represents all the files they want to
> allow or deny, where that file is updated when the cgroup adds or
> removes a device. So we add a patch to atomically replace any existing
> ebpf file already attached to our target cgroup using our new ebpf file.
>
>
> Diffs
> -----
>
> src/linux/cgroups2.cpp faf3a5853ebef6b80399be0e65406aa960a8e0e5
> src/linux/ebpf.cpp 50f2c6743a935d184b0c43cf4792ebb7f75cb09c
> src/tests/containerizer/cgroups2_tests.cpp
> 2ee39b342b3dd84f37ed78fea44b5a4b4f76f93a
>
>
> Diff: https://reviews.apache.org/r/75080/diff/6/
>
>
> Testing
> -------
>
> Added a test to verify that when a we attach a ebpf file to a cgroup that
> already has a file attached, the old file is replaced and the new file now
> controls device access. Cgroups2 tests passed.
>
>
> Thanks,
>
> Jason Zhou
>
>
