Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lsm cgroup api #1135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

altugbozkurt07
Copy link

@altugbozkurt07 altugbozkurt07 commented Jan 14, 2025

Hi @vadorovsky, @dave-tucker,

This is the refactored work based on the discussion we have had on discord.
Let me know if i missed anything.

Best

This change is Reviewable

Copy link

netlify bot commented Jan 14, 2025

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9381fcb
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/679b907a1d17f60008ae9f23
😎 Deploy Preview https://deploy-preview-1135--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify mergify bot added aya This is about aya (userspace) aya-bpf This is about aya-bpf (kernel) aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI labels Jan 14, 2025
@tamird
Copy link
Member

tamird commented Jan 14, 2025

Please avoid opening a new PR each time. There are comments I left in #1131 that remain unaddressed.

@altugbozkurt07
Copy link
Author

@tamird sorry, since we have changed the way we implemented api, i thought it deserved a new pr.

For the comments that remain unaddressed;
1- nix package is used in init crate so that is why i left it there. If you still want me to remove it from workspace and include it in specific crates where its used.
2- Done
3- Done
4- Removed the empty comment line
5- The changes proposed in this pr

Am i missing something other than what is stated in your comments?

Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've take a quick pass over and there are a few nits that need clearing up.
Please also check that the docs build and render correctly 🙏

Cargo.toml Outdated Show resolved Hide resolved
test/integration-test/Cargo.toml Show resolved Hide resolved
aya-ebpf-macros/src/lsm.rs Outdated Show resolved Hide resolved
aya-ebpf-macros/src/lsm.rs Outdated Show resolved Hide resolved
aya-ebpf-macros/src/lsm.rs Show resolved Hide resolved
aya/src/programs/lsm.rs Outdated Show resolved Hide resolved
aya/src/programs/lsm.rs Outdated Show resolved Hide resolved
/// The minimum kernel version required to use this feature is 6.0.
///
/// # Examples
/// ## LSM with cgroup attachment type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this subheading

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i remove this subheading, should i also remove it from lsm.rs ?

aya/src/programs/lsm_cgroup.rs Show resolved Hide resolved
let prog_fd = self.fd()?;
let prog_fd = prog_fd.as_fd();
let cgroup_fd = cgroup.as_fd();
let attach_type = self.data.expected_attach_type.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let attach_type = self.data.expected_attach_type.unwrap();
let attach_type = Some(BPF_LSM_CGROUP);

@tamird
Copy link
Member

tamird commented Jan 14, 2025

Please let us know when the tests are passing, or if you need help understanding the failures.

@altugbozkurt07
Copy link
Author

@dave-tucker thanks for your detailed feedback, i have updated the commit accordingly. Let me know if things are good to go for this one.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing.

aya/src/bpf.rs Outdated Show resolved Hide resolved
aya/src/programs/lsm.rs Outdated Show resolved Hide resolved
aya/src/programs/mod.rs Show resolved Hide resolved
@altugbozkurt07 altugbozkurt07 force-pushed the lsm_cgroup_api branch 2 times, most recently from 97a52dd to f67626f Compare January 21, 2025 15:03
Copy link

mergify bot commented Jan 21, 2025

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

@mergify mergify bot added the api/needs-review Makes an API change that needs review label Jan 21, 2025
@mergify mergify bot requested a review from alessandrod January 21, 2025 15:04
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 15 files at r1, 1 of 8 files at r2, 14 of 15 files at r3, all commit messages.
Dismissed @dave-tucker from 17 discussions.
Reviewable status: 20 of 23 files reviewed, 10 unresolved discussions (waiting on @alessandrod and @altugbozkurt07)


-- commits line 2 at r3:
why is this a double colon?


xtask/src/run.rs line 416 at r3 (raw file):

                // Heed the advice and boot with noapic. We don't know why this happens.
                kernel_args.push(" noapic");
                kernel_args.push(" lsm=lockdown,capability,bpf");

Please add a comment.


test/integration-test/src/tests/lsm.rs line 36 at r3 (raw file):

    let cgroup_path = Path::new("/sys/fs/cgroup/lsm_cgroup_test");
    if !cgroup_path.exists() {

i think you can drop this check


test/integration-test/src/tests/lsm.rs line 40 at r3 (raw file):

    }

    let _ = prog.attach(File::open(cgroup_path).unwrap()).unwrap();

what's the deal with this let _?


test/integration-test/src/tests/lsm.rs line 42 at r3 (raw file):

    let _ = prog.attach(File::open(cgroup_path).unwrap()).unwrap();

    match unsafe { fork().expect("Failed to fork process") } {

why do we need this fork? if the goal is to show that per-pid filtering occurs, you aren't doing that right now.


test/integration-test/src/tests/lsm.rs line 50 at r3 (raw file):

            let mut f = File::create(cgroup_path.join("cgroup.procs"))
                .expect("could not open cgroup procs");
            f.write_fmt(format_args!("{}", pid.as_raw() as u64))

how about write!(&mut f, "{pid}")?


test/integration-test/src/tests/lsm.rs line 59 at r3 (raw file):

        ForkResult::Child => {
            assert_matches::assert_matches!(TcpListener::bind("127.0.0.1:12345"), Ok(listener) => assert_eq!(
                listener.local_addr().unwrap(), SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 12345)))

do we need this assertion? probably all we care about is that it didn't error


test/integration-test/src/tests/lsm.rs line 80 at r3 (raw file):

    prog.attach().unwrap();

    assert_matches::assert_matches!(TcpListener::bind("127.0.0.1:12345"), Err(e) => assert_eq!(

we should also do this before attaching the program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/needs-review Makes an API change that needs review aya This is about aya (userspace) aya-bpf This is about aya-bpf (kernel) aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants