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-common crate exporting SpinLock for use in shared map value structs.
• Add aya_ebpf::spin_lock module with EbpfSpinLock::lock() returning an RAII SpinLockGuard that 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.

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

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

1. Pod requires Copy.
2. 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):

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):

eBPF spin locks can't be user in the userspace.

@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):

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):

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 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):

OK, I see. Is it also required that the inner field is unnamed?

@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):

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):

Public type aliases require the aliased item to be also public.

aya-common/src/spin_lock.rs line 14 at r2 (raw file):

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):

The problem is that the autogenerated helper has its own `bpf_

@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):

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):

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):

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 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):

Done. Nope, it doesn't.

aya-common/src/spin_lock.rs line 7 at r2 (raw file):

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

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):

Done.

ebpf/aya-ebpf/src/spin_lock.rs line 18 at r2 (raw file):

Done.

ebpf/aya-ebpf/src/spin_lock.rs line 24 at r2 (raw file):

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

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):

I think this is OK,

aya-common/src/spin_lock.rs line 2 at r4 (raw file):

I think this is OK.

ebpf/aya-ebpf/src/spin_lock.rs line 33 at r2 (raw file):

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)