lsm-cgroup: attachment type support #1131
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
so i basically re-implemented it so it would be using the latest aya infrastructure and we will have a single LSM program type that would support both attachment types, instead of creating a different program type for each attachment type. I thought this would make it more consistent with the relevant BPF API provided by the OS.
See my comment on the attach method. I think this is the wrong direction.
Reviewed 3 of 12 files at r1, all commit messages.
Reviewable status: 3 of 12 files reviewed, 5 unresolved discussions (waiting on @altugbozkurt07)
Cargo.toml line 81 at r1 (raw file):
log = { version = "0.4", default-features = false } netns-rs = { version = "0.1", default-features = false } nix = { version = "0.29.0", default-features = true }
please undo this. if you're using a specific feature in a particular crate, name it in that crate's manifest.
aya/src/programs/lsm.rs line 11 at r1 (raw file):
obj::btf::{Btf, BtfKind}, programs::{ define_link_wrapper, load_program, utils::attach_raw_tracepoint, FdLink, FdLinkId,
LsmAttachType can be here.
aya/src/programs/lsm.rs line 31 at r1 (raw file):
/// /// # Examples /// LSM with MAC attachment type
Make this a subhead? ## LSM with MAC attachment type
aya/src/programs/lsm.rs line 52 at r1 (raw file):
/// ``` /// /// LSM with cgroup attachment type
ditto
aya/src/programs/lsm.rs line 101 at r1 (raw file):
/// /// The returned value can be used to detach, see [Lsm::detach]. pub fn attach<T: AsFd>(&mut self, cgroup: Option<T>) -> Result<LsmLinkId, ProgramError> {
this API is unfortunate. we shouldn't require or even allow the caller to pass the cgroup argument if the attachment type isn't cgroup.
| let section_prefix = if *sleepable { "lsm.s" } else { "lsm" }; | ||
| let section_name: Cow<'_, _> = if let Some(hook) = hook { | ||
| format!("{}/{}", section_prefix, hook).into() | ||
| if self.cgroup{ |
There was a problem hiding this comment.
The if self.cgroup { ... } else { ... } branches are almost identical. I think we could avoid repetition by doing something like:
let section_prefix = if self.cgroup {
"lsm_cgroup"
} else {
"lsm"
};
let section_name = if let Some(name) = &self.hook {
format!("{section_prefix}/{name}");
};
let fn_name = &self.item.sig.ident;
let item = &self.item;
quote! {
#[no_mangle]
#[link_section = #section_name]
fn #fn_name(ctx: *mut ::core::ffi::c_void) -> i32 {
return #fn_name(::aya_ebpf::programs::LsmContext::new(ctx));
#item
}
}|
|
||
| // LSM probes need to return an integer corresponding to the correct | ||
| // policy decision. Therefore we do not simply default to a return value | ||
| // of 0 as in other program types. |
There was a problem hiding this comment.
Let's put this comment before quote!?
| if let Err(err) = prog.load("socket_bind", &btf) { | ||
| panic!("{err}"); | ||
| } |
There was a problem hiding this comment.
| if let Err(err) = prog.load("socket_bind", &btf) { | |
| panic!("{err}"); | |
| } | |
| prog.load("socket_bind", &btf).unwrap(); |
| panic!("{err}"); | ||
| } | ||
|
|
||
| let cgroup_path = Path::new(".").join("/sys/fs/cgroup/").join("lsm_cgroup_test"); |
There was a problem hiding this comment.
These multiple joins look unnecessary.
| let cgroup_path = Path::new(".").join("/sys/fs/cgroup/").join("lsm_cgroup_test"); | |
| let cgroup_path = Path::new("/sys/fs/cgroup/lsm_cgroup_test"); |
| ) | ||
| .unwrap(); | ||
|
|
||
| unsafe { |
There was a problem hiding this comment.
You don't need to put this while block in unsafe. I would try to use unsafe in single lines where it's absolutely necessary.
|
Hi both, I have revisited the pr based on the feedbacks given. @tamird one thing i could not understand is what you meant by |
You can add |
|
Okay thanks for the clarification. Thanks |
I have followed up on the work from this PR, which has been more than 2 years since the initial commit for lsm_cgroup support created,
so i basically re-implemented it so it would be using the latest aya infrastructure and we will have a single LSM program type that would support both attachment types, instead of creating a different program type for each attachment type. I thought this would make it more consistent with the relevant BPF API provided by the OS.
@vadorovsky @dave-tucker i would really appreciate your review on this, and let me know if there is anything needs to be done before merging this into upstream.
This change is