You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by "DevinLeamy (via GitHub)" <gi...@apache.org> on 2024/03/11 18:56:22 UTC
[PR] [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id. [mesos]
DevinLeamy opened a new pull request, #511:
URL: https://github.com/apache/mesos/pull/511
Removing programs that are attached to a cgroup is a three step process.
1. Fetch the program ids that are attached to the cgroup.
2. Fetch the file descriptors of the attached programs, using their program ids.
3. Use the file descriptors of the attached programs to detach them.
This patch adds the function `ebpf::cgroups2::detach` to carry out the second and third steps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id. [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler closed pull request #511: [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id.
URL: https://github.com/apache/mesos/pull/511
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #511:
URL: https://github.com/apache/mesos/pull/511#discussion_r1520400858
##########
src/linux/ebpf.cpp:
##########
@@ -99,6 +99,48 @@ Try<int> load(const Program& program)
return *fd;
}
+
+// Detach an eBPF program from a target (AKA path) by its attachment type
+// and program id. Returns Nothing() on success or if no program is found.
+Try<Nothing> detach(uint32_t attach_type, int program_id, const string& target)
+{
+ Try<int> target_fd = os::open(target, O_DIRECTORY | O_RDONLY | O_CLOEXEC);
Review Comment:
Closed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #511:
URL: https://github.com/apache/mesos/pull/511#discussion_r1520404783
##########
src/linux/ebpf.cpp:
##########
@@ -99,6 +99,48 @@ Try<int> load(const Program& program)
return *fd;
}
+
+// Detach an eBPF program from a target (AKA path) by its attachment type
+// and program id. Returns Nothing() on success or if no program is found.
+Try<Nothing> detach(uint32_t attach_type, int program_id, const string& target)
+{
+ Try<int> target_fd = os::open(target, O_DIRECTORY | O_RDONLY | O_CLOEXEC);
+ if (target_fd.isError()) {
+ return Error("Failed to open '" + target + "': " + target_fd.error());
+ }
+
+ bpf_attr attr;
+ memset(&attr, 0, sizeof(attr));
+ attr.prog_id = program_id;
+
+ Try<int, ErrnoError> program_fd =
+ bpf(BPF_PROG_GET_FD_BY_ID, &attr, sizeof(attr));
Review Comment:
Noted. Updated to close this `program_fd` file descriptor to have `detach()` effectively detach and unload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #511:
URL: https://github.com/apache/mesos/pull/511#discussion_r1520401111
##########
src/linux/ebpf.hpp:
##########
@@ -61,6 +61,15 @@ namespace cgroups2 {
// device access dynamically.
Try<Nothing> attach(int fd, const std::string& cgroup);
+// Detach a BPF_CGROUP_DEVICE eBPF program from a cgroup, by program id.
+// @param cgroup Absolute path of a cgroup.
+// @param program_id Id of the eBPF program to detach.
Review Comment:
Made it surface the error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id. [mesos]
Posted by "DevinLeamy (via GitHub)" <gi...@apache.org>.
DevinLeamy commented on code in PR #511:
URL: https://github.com/apache/mesos/pull/511#discussion_r1520281007
##########
src/linux/ebpf.hpp:
##########
@@ -61,6 +61,15 @@ namespace cgroups2 {
// device access dynamically.
Try<Nothing> attach(int fd, const std::string& cgroup);
+// Detach a BPF_CGROUP_DEVICE eBPF program from a cgroup, by program id.
+// @param cgroup Absolute path of a cgroup.
+// @param program_id Id of the eBPF program to detach.
+Try<Nothing> detach(const std::string& cgroup, uint32_t program_id);
+
+// Get the program ids of all programs that have been attached to a cgroup.
+// @param cgroup Absolute path of a cgroup.
+Try<std::vector<uint32_t>> attached(const std::string& cgroup);
Review Comment:
Was missing from the previous patch. Can create a new patch for this if that's preferred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ebpf][cgroups2] Detach eBPF BPF_CGROUP_DEVICE programs by id. [mesos]
Posted by "bmahler (via GitHub)" <gi...@apache.org>.
bmahler commented on code in PR #511:
URL: https://github.com/apache/mesos/pull/511#discussion_r1520343182
##########
src/linux/ebpf.cpp:
##########
@@ -99,6 +99,48 @@ Try<int> load(const Program& program)
return *fd;
}
+
+// Detach an eBPF program from a target (AKA path) by its attachment type
+// and program id. Returns Nothing() on success or if no program is found.
+Try<Nothing> detach(uint32_t attach_type, int program_id, const string& target)
+{
+ Try<int> target_fd = os::open(target, O_DIRECTORY | O_RDONLY | O_CLOEXEC);
Review Comment:
this is getting leaked without a corresponding os::close
##########
src/linux/ebpf.cpp:
##########
@@ -99,6 +99,48 @@ Try<int> load(const Program& program)
return *fd;
}
+
+// Detach an eBPF program from a target (AKA path) by its attachment type
+// and program id. Returns Nothing() on success or if no program is found.
+Try<Nothing> detach(uint32_t attach_type, int program_id, const string& target)
Review Comment:
what is target here? isn't it a cgroup? in which case, shouldn't we make this function cgroup namespaced? I guess to be generic this would be taking an int target which is either a detach location fd or a netdevice ifindex, but since we're only doing cgroup device stuff, maybe we just keep the code cgroup specific for now, like we did with attach()?
##########
src/linux/ebpf.hpp:
##########
@@ -61,6 +61,15 @@ namespace cgroups2 {
// device access dynamically.
Try<Nothing> attach(int fd, const std::string& cgroup);
+// Detach a BPF_CGROUP_DEVICE eBPF program from a cgroup, by program id.
+// @param cgroup Absolute path of a cgroup.
+// @param program_id Id of the eBPF program to detach.
Review Comment:
let's clarify that if we don't find the program attached, this returns Nothing rather than an error
##########
src/linux/ebpf.cpp:
##########
@@ -99,6 +99,48 @@ Try<int> load(const Program& program)
return *fd;
}
+
+// Detach an eBPF program from a target (AKA path) by its attachment type
+// and program id. Returns Nothing() on success or if no program is found.
+Try<Nothing> detach(uint32_t attach_type, int program_id, const string& target)
+{
+ Try<int> target_fd = os::open(target, O_DIRECTORY | O_RDONLY | O_CLOEXEC);
+ if (target_fd.isError()) {
+ return Error("Failed to open '" + target + "': " + target_fd.error());
+ }
+
+ bpf_attr attr;
+ memset(&attr, 0, sizeof(attr));
+ attr.prog_id = program_id;
+
+ Try<int, ErrnoError> program_fd =
+ bpf(BPF_PROG_GET_FD_BY_ID, &attr, sizeof(attr));
Review Comment:
from the docs, it looks like loading a program produces an fd, and unloading a program is done by closing the fd, so you probably want to keep that in mind here and figure out the implications of that (do we need an unload function that will detach and close fds..?, replacement will need to close the previous fd..? etc)
##########
src/linux/ebpf.hpp:
##########
@@ -61,6 +61,15 @@ namespace cgroups2 {
// device access dynamically.
Try<Nothing> attach(int fd, const std::string& cgroup);
+// Detach a BPF_CGROUP_DEVICE eBPF program from a cgroup, by program id.
+// @param cgroup Absolute path of a cgroup.
+// @param program_id Id of the eBPF program to detach.
Review Comment:
or better yet, we surface this error and no need to update this comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org