-
Notifications
You must be signed in to change notification settings - Fork 290
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
feat(aya): Add iterator program type #1088
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. |
a06e293
to
26f5a4f
Compare
26f5a4f
to
1ca59a6
Compare
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
374416a
to
88d20a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @alessandrod and @vadorovsky)
-- commits
line 2 at r1:
do we need to worry about compatibility with older kernels?
aya/src/sys/bpf.rs
line 416 at r1 (raw file):
// We need to set it to 0. // // https://elixir.bootlin.com/linux/v6.11.8/source/tools/lib/bpf/libbpf.c#L12790
why aren't we linking to github? also, linking to the kernel side would be more convincing =/
aya/src/programs/iter.rs
line 8 at r1 (raw file):
use crate::{ generated::{bpf_attach_type::BPF_TRACE_ITER, bpf_prog_type},
inconsistent import here
aya/src/programs/iter.rs
line 22 at r1 (raw file):
/// An eBPF iterator which allows to dump into user space. /// /// It can be seen as an alternative to `/proc` filesystem, which offers more
the antecedent of "which offers more flexibility" is a bit unclear. like, i don't know if you're saying the iterator is more flexible or procfs is.
aya/src/programs/iter.rs
line 72 at r1 (raw file):
/// Attaches the program. /// /// The returned value can be used to detach, see [Iter::detach].
this should be [Iter::detach
] or [Self::detach
].
aya/src/programs/iter.rs
line 89 at r1 (raw file):
/// Detaches the program. /// /// See [Iter::attach].
ditto
aya/src/programs/iter.rs
line 96 at r1 (raw file):
/// Takes ownership of the link referenced by the provided link_id. /// /// The link will be detached on `Drop` and the caller is now responsible
i can't parse this sentence because half of it is in the future tense and half is in the present tense.
aya/src/programs/iter.rs
line 166 at r1 (raw file):
// SAFETY: We are sure that the file descriptor is valid. This // `File` takes responsibility of closing it. let file = unsafe { std::fs::File::from_raw_fd(fd.as_raw_fd()) };
can we just use From<OwnedFd>
? https://doc.rust-lang.org/std/fs/struct.File.html#impl-From%3COwnedFd%3E-for-File
MockableFd.into_inner() returns OwnedFd.
aya/src/programs/iter.rs
line 176 at r1 (raw file):
/// retrieve the outputs of the iterator program. #[cfg(feature = "async_std")] pub fn into_async_io_file(self) -> Result<async_fs::File, LinkError> {
it is completely pointless to provide this IMO, the caller can trivially do it having obtained the std File
aya/src/programs/iter.rs
line 205 at r1 (raw file):
/// retrieve the outputs of the iterator program. #[cfg(feature = "async_tokio")] pub fn into_tokio_file(self) -> Result<tokio::fs::File, LinkError> {
ditto
https://docs.rs/tokio/latest/tokio/fs/struct.File.html#method.from_std
test/integration-test/bpf/iter.bpf.c
line 12 at r1 (raw file):
struct seq_file *seq = ctx->meta->seq; struct task_struct *task = ctx->task; if (task == NULL) {
under what circumstances is this possible?
test/integration-test/src/tests/iter.rs
line 24 at r1 (raw file):
assert_eq!(line_title, "tgid pid name"); assert!(line_init == "1 1 init" || line_init == "1 1 systemd");
this assertion emits nothing useful on failure
test/integration-test/src/tests/iter.rs
line 44 at r1 (raw file):
assert_eq!(line_title, "tgid pid name"); assert!(line_init == "1 1 init" || line_init == "1 1 systemd");
ditto
aya/Cargo.toml
line 34 at r1 (raw file):
[features] default = [] async_tokio = ["tokio/fs", "tokio/io-util", "tokio/net"]
these new dependencies belong in the intergration test crate, not here. they are not used by aya.
aya/src/programs/mod.rs
line 332 at r1 (raw file):
Self::Lsm(_) => ProgramType::Lsm, Self::BtfTracePoint(_) | Self::FEntry(_) | Self::FExit(_) | Self::Iter(_) => { ProgramType::Tracing
how come?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @alessandrod and @tamird)
Previously, tamird (Tamir Duberstein) wrote…
do we need to worry about compatibility with older kernels?
I don't think so. On older kernels, the program will just fail to load - but I think that's OK. I'm not a big fan of explicit kernel version checks. There are no incompatibilties across kernels supporting iterators.
aya/src/programs/mod.rs
line 332 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
how come?
Iterators are a subset of tracing programs:
https://elixir.bootlin.com/linux/v6.12-rc7/source/tools/lib/bpf/libbpf.c#L9405
I will add a comment.
aya/src/sys/bpf.rs
line 416 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why aren't we linking to github? also, linking to the kernel side would be more convincing =/
I personally find bootlin easier to use to jump around kernel code. Their search actually works, unlike search on github. 😛 Jumping to symbol definitions always works. I would strongly insist on sticking to bootlin for kernel code.
I know we have mixed github and bootlin links in different places in Aya. I would be happy to change all of them to bootlin in a separate PR.
Regarding the kernel side proof, let me try to find it. The overall expl
test/integration-test/bpf/iter.bpf.c
line 12 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
under what circumstances is this possible?
Good point, that should never happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @alessandrod and @vadorovsky)
aya/src/sys/bpf.rs
line 416 at r1 (raw file):
I personally find bootlin easier to use to jump around kernel code. Their search actually works, unlike search on github. 😛 Jumping to symbol definitions always works. I would strongly insist on sticking to bootlin for kernel code.
Yeah, github is a bit iffy on this tooling, but it's getting better, and it's the canonical repo.
I know we have mixed github and bootlin links in different places in Aya. I would be happy to change all of them to bootlin in a separate PR.
I think we should go the other way.
tamird@Tamirs-MacBook-Pro aya % git grep elixir | wc -l
6
tamird@Tamirs-MacBook-Pro aya % git grep -F 'torvalds/linux' | wc -l
46
88d20a0
to
a7c6d50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @alessandrod and @tamird)
aya/Cargo.toml
line 34 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
these new dependencies belong in the intergration test crate, not here. they are not used by aya.
Done.
aya/src/programs/iter.rs
line 8 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
inconsistent import here
I left this one and removed the other one (obj::generated
) - other aya/src/program
modules import from crate::generated
.
aya/src/programs/iter.rs
line 22 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
the antecedent of "which offers more flexibility" is a bit unclear. like, i don't know if you're saying the iterator is more flexible or procfs is.
Done, but feel free to propose something else, if it still doesn't look good to you.
aya/src/programs/iter.rs
line 96 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
i can't parse this sentence because half of it is in the future tense and half is in the present tense.
Good call, done. It's a copy paste from other modules though, so it needs to be fixed there as well.
aya/src/programs/iter.rs
line 166 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we just use
From<OwnedFd>
? https://doc.rust-lang.org/std/fs/struct.File.html#impl-From%3COwnedFd%3E-for-FileMockableFd.into_inner() returns OwnedFd.
Nice one! With that trick, I don't even have to do the whole dance with ManuallyDrop
. Thanks!
aya/src/programs/iter.rs
line 176 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
it is completely pointless to provide this IMO, the caller can trivially do it having obtained the std File
You're right, done.
aya/src/programs/iter.rs
line 205 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
https://docs.rs/tokio/latest/tokio/fs/struct.File.html#method.from_std
Done.
aya/src/sys/bpf.rs
line 416 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I personally find bootlin easier to use to jump around kernel code. Their search actually works, unlike search on github. 😛 Jumping to symbol definitions always works. I would strongly insist on sticking to bootlin for kernel code.
Yeah, github is a bit iffy on this tooling, but it's getting better, and it's the canonical repo.
I know we have mixed github and bootlin links in different places in Aya. I would be happy to change all of them to bootlin in a separate PR.
I think we should go the other way.
tamird@Tamirs-MacBook-Pro aya % git grep elixir | wc -l 6 tamird@Tamirs-MacBook-Pro aya % git grep -F 'torvalds/linux' | wc -l 46
Done.
test/integration-test/bpf/iter.bpf.c
line 12 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
Good point, that should never happen.
Lol, removing it makes verifier unhappy
0: R1=ctx(off=0,imm=0) R10=fp0
; struct seq_file *seq = ctx->meta->seq;
0: (79) r2 = *(u64 *)(r1 +0)
func 'bpf_iter_task' arg0 has btf_id 14387 type STRUCT 'bpf_iter_meta'
1: R1=ctx(off=0,imm=0) R2_w=trusted_ptr_bpf_iter_meta(off=0,imm=0)
; struct task_struct *task = ctx->task;
1: (79) r7 = *(u64 *)(r1 +8) ; R1=ctx(off=0,imm=0) R7_w=ptr_or_null_task_struct(id=1,off=0,imm=0)
; struct seq_file *seq = ctx->meta->seq;
2: (79) r6 = *(u64 *)(r2 +0) ; R2_w=trusted_ptr_bpf_iter_meta(off=0,imm=0) R6_w=trusted_ptr_seq_file(off=0,imm=0)
; if (ctx->meta->seq_num == 0) {
3: (79) r1 = *(u64 *)(r2 +16) ; R1_w=scalar() R2_w=trusted_ptr_bpf_iter_meta(off=0,imm=0)
; if (ctx->meta->seq_num == 0) {
4: (55) if r1 != 0x0 goto pc+8 ; R1_w=0
5: (bf) r4 = r10 ; R4_w=fp0 R10=fp0
; BPF_SEQ_PRINTF(seq, "tgid pid name\n");
6: (07) r4 += -24 ; R4_w=fp-24
7: (bf) r1 = r6 ; R1_w=trusted_ptr_seq_file(off=0,imm=0) R6_w=trusted_ptr_seq_file(off=0,imm=0)
8: (18) r2 = 0xffff888f3a46a910 ; R2_w=map_value(off=0,ks=4,vs=38,imm=0)
10: (b7) r3 = 24 ; R3_w=24
11: (b7) r5 = 0 ; R5_w=0
12: (85) call bpf_seq_printf#126 ; R0=scalar()
; BPF_SEQ_PRINTF(seq, "%-8d %-8d %s\n", task->tgid, task->pid, task->comm);
13: (61) r1 = *(u32 *)(r7 +2460)
R7 invalid mem access 'ptr_or_null_'
verification time 305 usec
stack depth 24
test/integration-test/src/tests/iter.rs
line 24 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this assertion emits nothing useful on failure
I added some manual message, but overall it's a bummer that there is no assert_in!
which would do that for me.
test/integration-test/src/tests/iter.rs
line 44 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
ditto
9165e7f
to
f197317
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @alessandrod and @vadorovsky)
aya/Cargo.toml
line 34 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
Done.
async-fs is still here.
aya/src/programs/iter.rs
line 8 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
I left this one and removed the other one (
obj::generated
) - otheraya/src/program
modules import fromcrate::generated
.
the other inconsistency is that you import BPF_TRACE_ITER
but not bpf_prog_type::BPF_PROG_TYPE_TRACING
(same with bpf_link_type::BPF_LINK_TYPE_ITER
aya/src/programs/iter.rs
line 22 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
Done, but feel free to propose something else, if it still doesn't look good to you.
I think removing the comma after "filesystem" would make things clearer.
aya/src/programs/iter.rs
line 72 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this should be [
Iter::detach
] or [Self::detach
].
Why not Self?
aya/src/programs/iter.rs
line 96 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
Good call, done. It's a copy paste from other modules though, so it needs to be fixed there as well.
Did you fix the others?
test/integration-test/Cargo.toml
line 30 at r3 (raw file):
tokio = { workspace = true, features = [ "fs", "io-util",
as in another comment i think we can revert this, no need to test tokio itself
test/integration-test/src/tests/iter.rs
line 34 at r3 (raw file):
#[test(tokio::test)] async fn iter_async_task() {
do we get any value out of testing this? we're just testing tokio here.
aya/src/programs/iter.rs
line 14 at r3 (raw file):
}; /// An eBPF iterator which allows to dump into user space.
s/eBPF/BPF/ per the docs
aya/src/programs/iter.rs
line 154 at r3 (raw file):
}) })?; let file: std::fs::File = fd.into_inner().into();
nit: I think you can drop local? Ok(fd.into_inner().into())
should be ale to infer the target type
aya/src/sys/bpf.rs
line 420 at r3 (raw file):
// https://github.com/torvalds/linux/blob/v6.12/kernel/bpf/bpf_iter.c#L517-L518 LinkTarget::Iter => { attr.link_create.__bindgen_anon_2.target_fd = 0;
wait do we actually need to do this? we start with a mem::zeroed above. I mean it's fine to do it, but I think it is't necessary.
f197317
to
2be594f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @alessandrod and @tamird)
aya/Cargo.toml
line 34 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
async-fs is still here.
Done.
aya/src/programs/iter.rs
line 8 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
the other inconsistency is that you import
BPF_TRACE_ITER
but notbpf_prog_type::BPF_PROG_TYPE_TRACING
(same withbpf_link_type::BPF_LINK_TYPE_ITER
Done.
aya/src/programs/iter.rs
line 22 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I think removing the comma after "filesystem" would make things clearer.
Done.
aya/src/programs/iter.rs
line 72 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Why not Self?
Done
aya/src/programs/iter.rs
line 89 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
Done.
aya/src/programs/iter.rs
line 96 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Did you fix the others?
No, because I'm not touching the other files in this PR. To be clear - I copy pasted that docstring from the XDP module. I think it deserves a separate PR.
aya/src/programs/iter.rs
line 14 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
s/eBPF/BPF/ per the docs
Done.
aya/src/programs/iter.rs
line 154 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
nit: I think you can drop local?
Ok(fd.into_inner().into())
should be ale to infer the target type
Nice one, done
aya/src/sys/bpf.rs
line 420 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
wait do we actually need to do this? we start with a mem::zeroed above. I mean it's fine to do it, but I think it is't necessary.
That's true, let's just drop it.
test/integration-test/Cargo.toml
line 30 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
as in another comment i think we can revert this, no need to test tokio itself
Honestly I added the tokio test mostly as an example for people how to use BPF iterators with Tokio.
I removed the Tokio test, but I added an example with Tokio in the docstring in iter.rs.
test/integration-test/src/tests/iter.rs
line 34 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
do we get any value out of testing this? we're just testing tokio here.
see above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 15 files reviewed, 12 unresolved discussions (waiting on @alessandrod and @tamird)
aya/Cargo.toml
line 32 at r4 (raw file):
# We are using the `fs` and `io-util` features of Tokio in the code example # in the documentation. tokio = { workspace = true, features = ["fs", "io-util", "rt"] }
To be clear - I removed the Tokio test, but I still want to keep some Tokio code example, so I decided to include it in docs.
2be594f
to
96bd62b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 15 files reviewed, 12 unresolved discussions (waiting on @alessandrod and @tamird)
aya/Cargo.toml
line 32 at r4 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
To be clear - I removed the Tokio test, but I still want to keep some Tokio code example, so I decided to include it in docs.
Or nevermind, clippy was complaining about this dep
https://github.com/aya-rs/aya/actions/runs/11994377992/job/33436346948?pr=1088
I removed it and I will just include the Tokio example in the book.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 6 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @alessandrod and @vadorovsky)
aya/src/programs/iter.rs
line 96 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
No, because I'm not touching the other files in this PR. To be clear - I copy pasted that docstring from the XDP module. I think it deserves a separate PR.
Sure. I think it can just be another commit in this PR.
test/integration-test/Cargo.toml
line 30 at r3 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
Honestly I added the tokio test mostly as an example for people how to use BPF iterators with Tokio.
I removed the Tokio test, but I added an example with Tokio in the docstring in iter.rs.
remove the extra features?
BPF iterators[0] are a way to dump kernel data into user-space and an alternative to `/proc` filesystem. This change adds support for BPF iterators on the user-space side. It provides a possibility to retrieve the outputs of BPF iterator programs both from sync and async Rust code. [0] https://docs.kernel.org/bpf/bpf_iterators.html
Don't mix the tenses.
96bd62b
to
b3abdd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r6, 24 of 24 files at r7, all commit messages.
Reviewable status: 38 of 39 files reviewed, all discussions resolved (waiting on @alessandrod)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 6 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)
BPF iterators[0] are a way to dump kernel data into user-space and an alternative to
/proc
filesystem.This change adds support for BPF iterators on the user-space side. It provides a possibility to retrieve the outputs of BPF iterator programs both from sync and async Rust code.
[0] https://docs.kernel.org/bpf/bpf_iterators.html
This change is