aya-{common,ebpf}: Add spin lock support#1467
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. |
|
Couldn't re-open #1356, because of a force push / rebase. All comments from there are addressed. |
There was a problem hiding this comment.
Pull request overview
Adds first-class bpf_spin_lock support to Aya by introducing a shared aya-common crate that defines a SpinLock type usable in map values, plus an aya-ebpf RAII guard and extension trait for locking/unlocking from eBPF programs, validated via a new XDP integration test.
Changes:
- Introduce new
aya-commoncrate exportingSpinLockfor use in shared map value structs. - Add
aya_ebpf::spin_lockmodule withEbpfSpinLock::lock()returning an RAIISpinLockGuardthat unlocks on drop. - Add integration eBPF program + userspace test verifying spin-locked counter increments in an XDP program.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| xtask/public-api/aya-ebpf.txt | Updates expected aya-ebpf public API to include new spin_lock module/types. |
| xtask/public-api/aya-common.txt | Adds expected aya-common public API snapshot. |
| test/integration-test/src/tests/spin_lock.rs | New userspace integration test sending UDP traffic and asserting counter increments. |
| test/integration-test/src/tests.rs | Registers the new spin_lock integration test module. |
| test/integration-test/src/lib.rs | Adds new embedded eBPF artifact reference for spin_lock. |
| test/integration-ebpf/src/spin_lock.rs | New XDP eBPF program using SpinLock + RAII guard to protect a shared counter. |
| test/integration-ebpf/Cargo.toml | Adds the new spin_lock eBPF binary target. |
| test/integration-common/src/lib.rs | Adds shared Counter struct containing SpinLock for both eBPF and userspace. |
| test/integration-common/Cargo.toml | Adds dependency on aya-common for shared types. |
| ebpf/aya-ebpf/src/spin_lock.rs | Implements EbpfSpinLock trait and SpinLockGuard unlocking on Drop. |
| ebpf/aya-ebpf/src/lib.rs | Exposes the new spin_lock module from aya-ebpf. |
| ebpf/aya-ebpf/Cargo.toml | Adds dependency on the new aya-common crate. |
| aya-common/src/spin_lock.rs | Introduces the shared SpinLock type intended to match kernel bpf_spin_lock. |
| aya-common/src/lib.rs | Exposes SpinLock from the new aya-common crate. |
| aya-common/Cargo.toml | Defines new workspace crate aya-common and its features/dependencies. |
| Cargo.toml | Adds aya-common to the workspace members list. |
Comments suppressed due to low confidence (1)
test/integration-common/Cargo.toml:22
integration-common’suserfeature enablesaya, but doesn’t enableaya-common/user. If the intent of theuserfeature is “all shared types areaya::Podin userspace”, consider includingaya-common/userin the feature soaya_common::SpinLock(and future shared types) get their userspacePodimpl whenintegration-commonis built withfeatures = ["user"].
[dependencies]
aya = { path = "../../aya", optional = true }
aya-common = { path = "../../aya-common" }
[features]
user = ["aya"]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c970726 to
5a50435
Compare
tamird
left a comment
There was a problem hiding this comment.
@tamird partially reviewed 6 files and made 9 comments.
Reviewable status: 5 of 16 files reviewed, 9 unresolved discussions (waiting on @vadorovsky).
ebpf/aya-ebpf/src/spin_lock.rs line 10 at r2 (raw file):
/// An RAII implementation of a scope of a spin lock. When this structure is /// dropped (falls out of scope), the lock will be unlocked. #[must_use = "A RAII guard that holds a spin lock."]
stdlib does not use capitalization nor punctuation here:
#[must_use = "if unused the Mutex will immediately unlock"]
ebpf/aya-ebpf/src/spin_lock.rs line 18 at r2 (raw file):
fn drop(&mut self) { unsafe { helpers::bpf_spin_unlock(core::ptr::from_ref(self.spin_lock).cast_mut().cast());
from_ref + cast_mut? why not just from_mut?
ebpf/aya-ebpf/src/spin_lock.rs line 24 at r2 (raw file):
/// Extension trait allowing to acquire a [`SpinLock`] in an eBPF program. pub trait EbpfSpinLock {
hmm, why an extension trait? why can't userspace get a wrapper type that exposes methods? we don't want anyone else implementing this, right?
ebpf/aya-ebpf/src/spin_lock.rs line 33 at r2 (raw file):
fn lock(&self) -> SpinLockGuard<'_> { unsafe { helpers::bpf_spin_lock(core::ptr::from_ref(self).cast_mut().cast());
shouldn't this be self.spin_lock? can you please add some helpers that take the correct pointer type so we don't footgun ourselves with void*?
aya-common/src/spin_lock.rs line 4 at r2 (raw file):
// The name `bpf_spin_lock` is expected by the verifier. // Wrapping this struct in an another struct does not work.
typo: "an another" should just be "another"
aya-common/src/spin_lock.rs line 7 at r2 (raw file):
#[doc(hidden)] #[repr(C)] #[derive(Debug, Copy, Clone, Default)]
why does it hold a uint? what does it mean to copy it? I am confused by the lack of information here.
aya-common/src/spin_lock.rs line 8 at r2 (raw file):
#[repr(C)] #[derive(Debug, Copy, Clone, Default)] pub struct bpf_spin_lock(c_uint);
does this name need to be pub, given the below?
aya-common/src/spin_lock.rs line 14 at r2 (raw file):
#[cfg(feature = "user")] unsafe impl aya::Pod for SpinLock {}
spinlock is pod?
aya-common/src/lib.rs line 3 at r2 (raw file):
#![no_std] pub mod spin_lock;
do we need this mod to be pub?
vadorovsky
left a comment
There was a problem hiding this comment.
@vadorovsky made 3 comments.
Reviewable status: 5 of 16 files reviewed, 9 unresolved discussions (waiting on @tamird).
aya-common/src/spin_lock.rs line 7 at r2 (raw file):
why does it hold a uint?
Because it's used by the helper.
https://elixir.bootlin.com/linux/v6.18.6/source/kernel/bpf/helpers.c#L289
Do I really have to comment on that? I would expect people to look at the kernel sources if they're curious about why certain kernel have certain layout. We don't explain how every single binding works.
what does it mean to copy it?
- Pod requires Copy.
- It lets people derive Copy (if they want) on the types they want to store in maps. The lack of it would prevent people from deriving Copy on types that have a spin lock.
aya-common/src/spin_lock.rs line 14 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
spinlock is pod?
Yes - it's meant to be used in BPF maps. If we don't implement it ourselves, users will have to do it - which would be a horrible user experience.
ebpf/aya-ebpf/src/spin_lock.rs line 24 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
hmm, why an extension trait? why can't userspace get a wrapper type that exposes methods? we don't want anyone else implementing this, right?
eBPF spin locks can't be user in the userspace.
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 1 file and made 2 comments.
Reviewable status: 6 of 16 files reviewed, 9 unresolved discussions (waiting on @vadorovsky).
aya-common/src/spin_lock.rs line 14 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
Yes - it's meant to be used in BPF maps. If we don't implement it ourselves, users will have to do it - which would be a horrible user experience.
I guess I'm surprised we want to allow this; copying the spinlock seems like asking for undefined behavior
ebpf/aya-ebpf/src/spin_lock.rs line 24 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
eBPF spin locks can't be user in the userspace.
sorry, i meant it the other way around, but i guess the reason is explained on the type definition in spin_lock.rs.
can you please seal this trait and comment on it that it exists because we want methods only in eBPF and can't use a wrapper type?
tamird
left a comment
There was a problem hiding this comment.
@tamird made 1 comment.
Reviewable status: 6 of 16 files reviewed, 9 unresolved discussions (waiting on @vadorovsky).
aya-common/src/spin_lock.rs line 7 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
why does it hold a uint?
Because it's used by the helper.
https://elixir.bootlin.com/linux/v6.18.6/source/kernel/bpf/helpers.c#L289Do I really have to comment on that? I would expect people to look at the kernel sources if they're curious about why certain kernel have certain layout. We don't explain how every single binding works.
what does it mean to copy it?
- Pod requires Copy.
- It lets people derive Copy (if they want) on the types they want to store in maps. The lack of it would prevent people from deriving Copy on types that have a spin lock.
OK, I see. Is it also required that the inner field is unnamed?
vadorovsky
left a comment
There was a problem hiding this comment.
@vadorovsky made 4 comments.
Reviewable status: 6 of 16 files reviewed, 9 unresolved discussions (waiting on @tamird).
aya-common/src/spin_lock.rs line 7 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
OK, I see. Is it also required that the inner field is unnamed?
No, the unnamed field is not a requirement - just my choice, since we aren't really interested in accessing it, and naming it would make clippy/rustc complain that the field is unused. We just need to ensure that the layout and alignment of this struct is same as the kernel expects.
aya-common/src/spin_lock.rs line 8 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
does this name need to be pub, given the below?
Public type aliases require the aliased item to be also public.
aya-common/src/spin_lock.rs line 14 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I guess I'm surprised we want to allow this; copying the spinlock seems like asking for undefined behavior
Yeah, giving it a thought, I do agree that it's a shitty and confusing API. I can't think of a legitimate reason why Pod would require Copy (or even Clone) and the explanation why we ended up with putting these derives carelessly is likely "because bindgen applies all these derives" or "because I've seen these derives in bytemuck".
I'll try to get rid of that requirement.
ebpf/aya-ebpf/src/spin_lock.rs line 33 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
shouldn't this be self.spin_lock? can you please add some helpers that take the correct pointer type so we don't footgun ourselves with void*?
The problem is that the autogenerated helper has its own `bpf_
tamird
left a comment
There was a problem hiding this comment.
@tamird made 3 comments and resolved 1 discussion.
Reviewable status: 6 of 16 files reviewed, 8 unresolved discussions (waiting on @vadorovsky).
aya-common/src/spin_lock.rs line 7 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
No, the unnamed field is not a requirement - just my choice, since we aren't really interested in accessing it, and naming it would make clippy/rustc complain that the field is unused. We just need to ensure that the layout and alignment of this struct is same as the kernel expects.
You can give it a name starting with an underscore and the lint will be silent. That gives an opportunity to name it something explanatory.
aya-common/src/spin_lock.rs line 14 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
Yeah, giving it a thought, I do agree that it's a shitty and confusing API. I can't think of a legitimate reason why Pod would require
Copy(or evenClone) and the explanation why we ended up with putting these derives carelessly is likely "because bindgen applies all these derives" or "because I've seen these derives in bytemuck".I'll try to get rid of that requirement.
Well I guess the reason is that e.g. BPF_MAP_LOOKUP_ELEM copies the bytes into user-supplied memory. So you need a way to indicate that it's safe to do that. So I guess you're just supposed to be OK with copying spin locks into userspace.
ebpf/aya-ebpf/src/spin_lock.rs line 33 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
The problem is that the autogenerated helper has its own `bpf_
ah i just assumed it takes void* -- but the solution i propose still stands: we want our own helper that takes our pointer type so we can have some type safety. otherwise .cast() will just accept whatever
vadorovsky
left a comment
There was a problem hiding this comment.
@vadorovsky made 7 comments and resolved 1 discussion.
Reviewable status: 6 of 16 files reviewed, 7 unresolved discussions (waiting on @tamird).
aya-common/src/lib.rs line 3 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
do we need this mod to be pub?
Done. Nope, it doesn't.
aya-common/src/spin_lock.rs line 7 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
You can give it a name starting with an underscore and the lint will be silent. That gives an opportunity to name it something explanatory.
Actually I removed this handwritten binding now, see https://reviewable.io/reviews/aya-rs/aya/1467#-OkAoqTiDltPmF84f76H
aya-common/src/spin_lock.rs line 14 at r2 (raw file):
To be clear - after giving it more thought, I don't think cloning/copying the spin lock is an UB. It's just an integer with value 1 (if it's not locked, otherwise 0). I agree that the ability to clone or copy a lock might be confusing for people, but I don't think it's malicious in practice.
Well I guess the reason is that e.g.
BPF_MAP_LOOKUP_ELEMcopies the bytes into user-supplied memory. So you need a way to indicate that it's safe to do that. So I guess you're just supposed to be OK with copying spin locks into userspace.
Yeah. Therefore I don't think we can avoid Copy. Are you fine with leaving this code as it is, or do you have any ideas?
ebpf/aya-ebpf/src/spin_lock.rs line 10 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
stdlib does not use capitalization nor punctuation here:
#[must_use = "if unused the Mutex will immediately unlock"]
Done.
ebpf/aya-ebpf/src/spin_lock.rs line 18 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
from_ref + cast_mut? why not just from_mut?
Done.
ebpf/aya-ebpf/src/spin_lock.rs line 24 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
sorry, i meant it the other way around, but i guess the reason is explained on the type definition in spin_lock.rs.
can you please seal this trait and comment on it that it exists because we want methods only in eBPF and can't use a wrapper type?
Done.
ebpf/aya-ebpf/src/spin_lock.rs line 33 at r2 (raw file):
we want our own helper that takes our pointer type so we can have some type safety
And you would hand-write it? I don't think it's a good idea, because the autogenerated binding defines the IDs of BPF helpers.
But I have a better idea - we can just use the autogenerated binding and expose it with `pub type.! That will require adding aya-ebpf-bindings as a dep to aya-common, but I think it's fine. Pushed that, lmk if you agree.
vadorovsky
left a comment
There was a problem hiding this comment.
@vadorovsky made 1 comment.
Reviewable status: 3 of 18 files reviewed, 8 unresolved discussions (waiting on @tamird).
aya-common/src/spin_lock.rs line 2 at r4 (raw file):
/// A spin lock that can be used to protect shared data in eBPF maps pub type SpinLock = aya_ebpf_bindings::bindings::bpf_spin_lock;
Now I wonder if introducing a new crate just for this one export is an overkill. But on the other hand, require to use the aya-ebpf-bindings crate just for definitions of types stored in maps (that people usually would store in their *-common crates), sounds worse to me. Any opinions?
6609cbe to
04840cd
Compare
tamird
left a comment
There was a problem hiding this comment.
Just one comment on the sealed trait.
@tamird reviewed 17 files and all commit messages, made 5 comments, and resolved 8 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vadorovsky).
aya-common/src/spin_lock.rs line 14 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
To be clear - after giving it more thought, I don't think cloning/copying the spin lock is an UB. It's just an integer with value
1(if it's not locked, otherwise0). I agree that the ability to clone or copy a lock might be confusing for people, but I don't think it's malicious in practice.Well I guess the reason is that e.g.
BPF_MAP_LOOKUP_ELEMcopies the bytes into user-supplied memory. So you need a way to indicate that it's safe to do that. So I guess you're just supposed to be OK with copying spin locks into userspace.Yeah. Therefore I don't think we can avoid
Copy. Are you fine with leaving this code as it is, or do you have any ideas?
I think this is OK,
aya-common/src/spin_lock.rs line 2 at r4 (raw file):
Previously, vadorovsky (Michal R) wrote…
Now I wonder if introducing a new crate just for this one export is an overkill. But on the other hand, require to use the
aya-ebpf-bindingscrate just for definitions of types stored in maps (that people usually would store in their*-commoncrates), sounds worse to me. Any opinions?
I think this is OK.
ebpf/aya-ebpf/src/spin_lock.rs line 33 at r2 (raw file):
Previously, vadorovsky (Michal R) wrote…
we want our own helper that takes our pointer type so we can have some type safety
And you would hand-write it? I don't think it's a good idea, because the autogenerated binding defines the IDs of BPF helpers.
But I have a better idea - we can just use the autogenerated binding and expose it with `pub type.! That will require adding aya-ebpf-bindings as a dep to aya-common, but I think it's fine. Pushed that, lmk if you agree.
looks good now.
ebpf/aya-ebpf/src/spin_lock.rs line 21 at r5 (raw file):
mod sealed { trait Sealed {}
can you name this EbpfSpinLock? this should be the one we have an impl for (and then a blanket impl for the outer trait)
Add a new `aya-common` crate with a `SpinLock` struct, that wraps `bpf_spin_lock` and allows to use it in BPF maps. Add an extension trait `EbpfSpinLock` that provides a `lock` method, which is a wrapper over `bpf_spin_lock` helper. It returns a `SpinLockGuard` that calls `bpf_spin_unlock` helper once dropped. Test that functionality with a simple XDP counter program.
04840cd to
299377c
Compare
Add a new
aya-commoncrate with aSpinLockstruct, that wrapsbpf_spin_lockand allows to use it in BPF maps.Add an extension trait
EbpfSpinLockthat provides alockmethod, which is a wrapper overbpf_spin_lockhelper. It returns aSpinLockGuardthat callsbpf_spin_unlockhelper once dropped.Test that functionality with a simple XDP counter program.
This change is