aya: fix pid=0 handling in uprobe attach#1543
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on swananan).
aya/src/programs/uprobe.rs line 111 at r1 (raw file):
/// Attaches the uprobe to the function `fn_name` defined in the `target`. /// If the attach point specifies an offset, it is added to the address of /// the target function. If `pid` is not `None`, the program executes only
Should we use a tri-variant enum instead of Option? The integer case would carry a NonZero
perf_event_open treats pid=0 as the calling process/thread, so Some(0) should carry that meaning in UProbe::attach. However, aya unconditionally reads /proc/<pid>/maps when pid is Some, but /proc/0/maps does not exist. Use the current pid only for the ProcMap lookup, and keep pid=0 for the actual attach semantics. Add documentation and an integration test.
1941b95 to
d8136e1
Compare
swananan
left a comment
There was a problem hiding this comment.
UProbeScope mirrors PerfEventScope as prior art, and I keep the two commits for clarity. PerfEventScope::OneProcess needs NonZeroU32 too, I can raise a follow-up PR for this.
@swananan made 2 comments.
Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion (waiting on tamird).
aya/src/programs/uprobe.rs line 111 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Should we use a tri-variant enum instead of Option? The integer case would carry a
NonZero
Done
tamird
left a comment
There was a problem hiding this comment.
@tamird partially reviewed 6 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: 5 of 21 files reviewed, 4 unresolved discussions (waiting on swananan).
aya/src/programs/uprobe.rs line 2 at r2 (raw file):
//! User space probes. use core::num::NonZeroU32;
you can get this from std for slightly tidier imports
aya/src/programs/uprobe.rs line 93 at r2 (raw file):
/// Specifies which processes a uprobe should fire for. #[derive(Debug, Clone, Copy, PartialEq, Eq)]
we probably don't need anything here except maybe Debug
aya/src/programs/uprobe.rs line 121 at r2 (raw file):
Self::OneProcess(pid) => Some(pid.get()), } }
both of these are called in the same function -- i think we can just bake the knowledge into that function until we have a real second user
Code quote:
const fn perf_event_pid(self) -> Option<u32> {
match self {
Self::AllProcesses => None,
// Keep the kernel's pid=0 sentinel for the calling process/thread.
Self::CallingProcess => Some(0),
Self::OneProcess(pid) => Some(pid.get()),
}
}
fn proc_map_pid(self) -> Option<u32> {
match self {
Self::AllProcesses => None,
// /proc/0/maps does not exist, so resolve the calling process's
// ProcMap with its real pid; the actual attach still uses pid=0.
Self::CallingProcess => Some(std::process::id()),
Self::OneProcess(pid) => Some(pid.get()),
}
}test/integration-test/src/tests/array.rs line 74 at r2 (raw file):
symbol, "/proc/self/exe", aya::programs::uprobe::UProbeScope::AllProcesses,
maybe add an import? there are already several at the top of the file
here and all others please
d8136e1 to
d3f38bf
Compare
Replace the pid Option in UProbe::attach with UProbeScope so the all-processes, calling-process, and explicit-pid cases are all expressed directly in the type. This removes pid=0 as a magic value from the public API, keeps the calling-process case explicit, and uses the current pid for ProcMap resolution introduced in the previous commit. Update docs and integration tests to use the new scope-based API.
d3f38bf to
b205939
Compare
swananan
left a comment
There was a problem hiding this comment.
@swananan made 4 comments.
Reviewable status: 0 of 21 files reviewed, 4 unresolved discussions (waiting on tamird).
aya/src/programs/uprobe.rs line 2 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
you can get this from std for slightly tidier imports
Done.
aya/src/programs/uprobe.rs line 93 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
we probably don't need anything here except maybe Debug
Done. Same mistake, will keep it in the mind.
Restored Clone + Copy, since load.rs now uses #[test_case] to run the same uprobe unload test across multiple scopes.
aya/src/programs/uprobe.rs line 121 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
both of these are called in the same function -- i think we can just bake the knowledge into that function until we have a real second user
Done.
test/integration-test/src/tests/array.rs line 74 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
maybe add an import? there are already several at the top of the file
here and all others please
Done.
tamird
left a comment
There was a problem hiding this comment.
That would be great, ty.
@codex review
@tamird reviewed 21 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on swananan).
aya/src/programs/uprobe.rs line 176 at r4 (raw file):
let Self { data, kind } = self; let path = path.as_os_str(); attach::<Self, _>(data, *kind, path, offset, perf_event_pid, cookie)
I wonder if the pid-to-int conversion should be down inside this method. Depends who else calls it
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect handling of pid=0 when attaching single uprobes and replaces the pid: Option<u32> API with a typed UProbeScope to make attach semantics explicit and consistent with the multi-attach path.
Changes:
- Introduce
UProbeScope(AllProcesses,CallingProcess,OneProcess(NonZeroU32)) and updateUProbe::attachto use it. - Fix single-attach
CallingProcessbehavior by using the real PID only for/proc/<pid>/mapsresolution while still passingpid=0toperf_event_open. - Update docs and integration tests to use the new scope-based API (including coverage for all three scopes).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
aya/src/programs/uprobe.rs |
Adds UProbeScope and updates UProbe::attach to correctly handle calling-process scope without /proc/0/maps access. |
aya/src/bpf.rs |
Updates the Ebpf::program_mut example to use UProbeScope::AllProcesses. |
xtask/public-api/aya.txt |
Updates blessed public API surface for the new UProbeScope and modified UProbe::attach signature. |
test/integration-test/src/tests/load.rs |
Updates uprobe attach callsites and adds an integration test that exercises all UProbeScope variants. |
test/integration-test/src/tests/uprobe_cookie.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/strncmp.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/stack_trace.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/ring_buf.rs |
Updates uprobe attach calls to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/relocations.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/prog_array.rs |
Updates uprobe attach calls to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/printk.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/per_cpu_array.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/maps_disjoint.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/lpm_trie.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/log.rs |
Updates uprobe attach calls to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/linear_data_structures.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/info.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/btf_relocations.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/bpf_probe_read.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/bloom_filter.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
test/integration-test/src/tests/array.rs |
Updates uprobe attach call to pass UProbeScope::AllProcesses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
swananan
left a comment
There was a problem hiding this comment.
@swananan made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on tamird).
aya/src/programs/uprobe.rs line 176 at r4 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I wonder if the pid-to-int conversion should be down inside this method. Depends who else calls it
Looked into this, only uprobe will use this method with pid, even multi uprobe doesn't need this. I would keep this conversion here.
tamird
left a comment
There was a problem hiding this comment.
@tamird resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved.
|
@swananan could you update aya-template? see these build failures. thank you! |
Sure |
Summary
While working on multi-uprobe support (#1417), @alessandrod pointed out that
Some(0)has inconsistent semantics between the single-attach (perf_event_open: pid=0 → current process) and multi-attach (bpf_link_create: pid=0 → all processes) paths.Investigating this, I found that
Some(0)never actually worked in aya's existing single-attach path either:UProbe::attachunconditionally reads/proc/{pid}/mapswhen pid isSome, and/proc/0/mapsdoes not exist (pid 0 is the kernel swapper process). The attach fails before even reaching the kernel.This PR uses the real pid only for the ProcMap lookup, and keeps the original pid=0 for the actual attach call, matching libbpf's single-attach behavior which passes pid=0 directly to
perf_event_open_probe(ref).And replace the pid Option in UProbe::attach with UProbeScope so the all-processes, calling-process, and explicit-pid cases are all expressed directly in the type.
This removes pid=0 as a magic value from the public API, keeps the calling-process case explicit, and uses the current pid for ProcMap resolution introduced in the previous commit. Update docs and integration tests to use the new scope-based API.
Added/updated tests?
We strongly encourage you to add a test for your changes.
have not been included
Checklist
cargo +nightly fmt.You can find failing lints with
cargo xtask clippy.cargo test.cargo xtask public-api --bless.(Optional) What GIF best describes this PR or how it makes you feel?
This change is