lsm: cgroup attachment type support

[altugbozkurt07]

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 

[netlify]

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	c2a92f5
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/67a0e8e22f70410008315ea1
😎 Deploy Preview	https://deploy-preview-1135--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[tamird]

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

[altugbozkurt07]

@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?

[dave-tucker]

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 🙏

[dave-tucker]

Remove this subheading

[altugbozkurt07]

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

[dave-tucker]

Suggested change

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

[tamird]

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

[altugbozkurt07]

@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.

[tamird]

Tests are failing.

[mergify]

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

[tamird]

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.

[altugbozkurt07]

Reviewable status: 20 of 23 files reviewed, 10 unresolved discussions (waiting on @alessandrod and @tamird)

---

-- commits line 2 at r3:

Previously, tamird (Tamir Duberstein) wrote…

why is this a double colon?

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

i think you can drop this check

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

what's the deal with this let _?

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

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

i wanted to make sure that only the processes in the specified cgroup are affected by the lsm hook, and other processes should be able to perform actions without any issues. Maybe a bug might be accidentally introduced in the lsm_cgroup implementation in ebpf vm runtime that might cause it to get executed against all workloads, which is very very less likely to happen. But i thought this might not hurt to check

---

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

Previously, tamird (Tamir Duberstein) wrote…

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

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

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

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

we should also do this before attaching the program.

done

[tamird]

Reviewed 3 of 4 files at r4.
Reviewable status: 20 of 23 files reviewed, 5 unresolved discussions (waiting on @alessandrod and @altugbozkurt07)

---

-- commits line 2 at r3:

Previously, altugbozkurt07 wrote…

done

please remove the space before the colon

---

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

Previously, altugbozkurt07 wrote…

i wanted to make sure that only the processes in the specified cgroup are affected by the lsm hook, and other processes should be able to perform actions without any issues. Maybe a bug might be accidentally introduced in the lsm_cgroup implementation in ebpf vm runtime that might cause it to get executed against all workloads, which is very very less likely to happen. But i thought this might not hurt to check

It is not our job to test the ebpf VM.

---

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

Previously, tamird (Tamir Duberstein) wrote…

Please add a comment.

This comment is not sufficient. Please explain why, not what.

[altugbozkurt07]

Reviewable status: 15 of 23 files reviewed, 5 unresolved discussions (waiting on @alessandrod and @tamird)

---

-- commits line 2 at r3:

Previously, tamird (Tamir Duberstein) wrote…

please remove the space before the colon

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

It is not our job to test the ebpf VM.

okay updated the test accordingly

---

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

Previously, tamird (Tamir Duberstein) wrote…

This comment is not sufficient. Please explain why, not what.

done

[dave-tucker]

@altugbozkurt07 thanks for addressing the last set of review comments. Looks like this needs a quick cargo +nightly fmt and to be rebased. Once it's green I'll take another look.

[altugbozkurt07]

@dave-tucker thanks for your help and feedback during the process. I just pushed the +fmted version. Let me know if there is anything else i missed

[tamird]

There are more non-fmt lint failures.

Reviewed 4 of 7 files at r6, 1 of 2 files at r7, 2 of 2 files at r9, all commit messages.
Reviewable status: 20 of 23 files reviewed, 8 unresolved discussions (waiting on @alessandrod, @altugbozkurt07, and @dave-tucker)

---

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

Previously, altugbozkurt07 wrote…

done

Please add a period.

---

test/integration-test/Cargo.toml line 31 at r7 (raw file):

tokio = { workspace = true, features = ["macros", "rt-multi-thread", "time"] }
xdpilone = { workspace = true }
nix = { workspace = true, features = ["process"] }

You no longer need this crate I think

---

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

#[test]
#[ignore = "Lsm program type requires a special kernel config to be enabled and github runners dont allow us to configure kernel parameters for linux vms[waiting on this pr: 1063]"]

goodness. can we shorten this?

---

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

    assert_matches::assert_matches!(TcpListener::bind("127.0.0.1:12345"), Ok(_));

    let cgroup_path = Path::new("/sys/fs/cgroup/lsm_cgroup_test");

what creates this path?

---

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

    prog.attach(File::open(cgroup_path).unwrap()).unwrap();

    let pid = getpid();

can we drop the nix dep? https://doc.rust-lang.org/std/process/fn.id.html

---

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

    let pid = getpid();
    let mut f =
        File::create(cgroup_path.join("cgroup.procs")).expect("could not open cgroup procs");

we should clean this file up when we're done, yes? we already have a dependency on the tempfile crate, can we use it for that?

[altugbozkurt07]

Other lint errors are related to public api documentation as far as i can understand and @dave-tucker told me someone could push a commit to fix it.

Reviewable status: 14 of 23 files reviewed, 8 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @tamird)

---

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

Previously, tamird (Tamir Duberstein) wrote…

goodness. can we shorten this?

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

what creates this path?

Cgroupfs is mounted during init. This is needed for lsm_cgroup test.

Code snippet:

Mount {
        source: "cgroup2",
        target: "/sys/fs/cgroup",
        fstype: "cgroup2",
        flags: nix::mount::MsFlags::empty(),
        data: None,
        target_mode: None,
},

---

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

Previously, tamird (Tamir Duberstein) wrote…

can we drop the nix dep? https://doc.rust-lang.org/std/process/fn.id.html

done

---

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

Previously, tamird (Tamir Duberstein) wrote…

we should clean this file up when we're done, yes? we already have a dependency on the tempfile crate, can we use it for that?

We cant use tempfs, because cgroupfs is a special filesystem utilized by kernel for organizing processes. But i can remove the file at the end of the test.

---

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

Previously, tamird (Tamir Duberstein) wrote…

Please add a period.

done

---

test/integration-test/Cargo.toml line 31 at r7 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

You no longer need this crate I think

done

[tamird]

Reviewed 6 of 9 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: 20 of 23 files reviewed, 6 unresolved discussions (waiting on @alessandrod, @altugbozkurt07, and @dave-tucker)

---

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

Previously, altugbozkurt07 wrote…

Cgroupfs is mounted during init. This is needed for lsm_cgroup test.

Yes. What creates lsm_cgroup_test?

---

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

Previously, altugbozkurt07 wrote…

We cant use tempfs, because cgroupfs is a special filesystem utilized by kernel for organizing processes. But i can remove the file at the end of the test.

This cleanup won't happen if your test fails. That's why I suggested the tempfile crate.

Perhaps https://docs.rs/tempfile/latest/tempfile/struct.TempPath.html#method.from_path would be of use?

---

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

Previously, altugbozkurt07 wrote…

done

why is one instance of "lsm" capitalized and the other is not?

---

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

#[test]
#[ignore = "LSM programs need a special kernel config, which is not supported by GitHub runners[waiting on PR: 1063]."]

you could plumb a config (feature?) through xtask and set it in our CI. then that comment can go in the CI configs rather than here.

---

xtask/public-api/aya.txt line 1365 at r11 (raw file):

pub aya::maps::MapError::SyscallError(aya::sys::SyscallError)
pub aya::maps::MapError::Unsupported
pub aya::maps::MapError::Unsupported::map_type: aya_obj::generated::linux_bindings_aarch64::bpf_map_type

i see you're running aarch64. Our infra uses x86_64, so you'll need to run CARGO_CFG_BPF_TARGET_ARCH=x86_64 cargo +nightly xtask public-api --bless --target x86_64-unknown-linux-gnu

[tamird]

Looks like you need to rebase after #1162.