From 70c77c5ea7d31aa7369bbfa0afbc2ea08859ef0d Mon Sep 17 00:00:00 2001 From: Michal R Date: Mon, 29 Sep 2025 08:20:58 +0200 Subject: [PATCH] aya-ebpf: Improve documentation of `maps::hash_map` module * Use the third person. * Make use of `#[doc]` and split out the most repetetive chunk into a separate file. * Make the `Safety` comment for `get*` operations more clear, provide context and inks. --- ebpf/aya-ebpf/src/maps/hash_map.rs | 88 ++++++++++------------------ ebpf/aya-ebpf/src/maps/map_safety.md | 32 ++++++++++ 2 files changed, 64 insertions(+), 56 deletions(-) create mode 100644 ebpf/aya-ebpf/src/maps/map_safety.md diff --git a/ebpf/aya-ebpf/src/maps/hash_map.rs b/ebpf/aya-ebpf/src/maps/hash_map.rs index 1b34ac03..b45ab6ff 100644 --- a/ebpf/aya-ebpf/src/maps/hash_map.rs +++ b/ebpf/aya-ebpf/src/maps/hash_map.rs @@ -48,35 +48,28 @@ impl HashMap { } } - /// Retrieve the value associate with `key` from the map. - /// - /// # Safety - /// - /// Unless the map flag `BPF_F_NO_PREALLOC` is used, the kernel does not guarantee the atomicity - /// of `insert` or `remove`, and any element removed from the map might get aliased by another - /// element in the map, causing garbage to be read, or corruption in case of writes. + #[doc = "Retrieves the value associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub unsafe fn get(&self, key: impl Borrow) -> Option<&V> { unsafe { get(self.def.get(), key.borrow()) } } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, but this returns a raw pointer and it's up to the caller - /// to decide whether it's safe to dereference the pointer or not. + #[doc = "Retrieves the pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr(&self, key: impl Borrow) -> Option<*const V> { get_ptr(self.def.get(), key.borrow()) } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, and additionally cares should be taken to avoid - /// concurrent writes, but it's up to the caller to decide whether it's safe to dereference the - /// pointer or not. + #[doc = "Retrieves the mutable pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr_mut(&self, key: impl Borrow) -> Option<*mut V> { get_ptr_mut(self.def.get(), key.borrow()) } + /// Inserts a key-value pair into the map. #[inline] pub fn insert( &self, @@ -87,6 +80,7 @@ impl HashMap { insert(self.def.get().cast(), key.borrow(), value.borrow(), flags) } + /// Removes a key from the map. #[inline] pub fn remove(&self, key: impl Borrow) -> Result<(), c_long> { remove(self.def.get().cast(), key.borrow()) @@ -129,35 +123,28 @@ impl LruHashMap { } } - /// Retrieve the value associate with `key` from the map. - /// - /// # Safety - /// - /// Unless the map flag `BPF_F_NO_PREALLOC` is used, the kernel does not guarantee the atomicity - /// of `insert` or `remove`, and any element removed from the map might get aliased by another - /// element in the map, causing garbage to be read, or corruption in case of writes. + #[doc = "Retrieves the value associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub unsafe fn get(&self, key: impl Borrow) -> Option<&V> { unsafe { get(self.def.get(), key.borrow()) } } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, but this returns a raw pointer and it's up to the caller - /// to decide whether it's safe to dereference the pointer or not. + #[doc = "Retrieves the pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr(&self, key: impl Borrow) -> Option<*const V> { get_ptr(self.def.get(), key.borrow()) } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, and additionally cares should be taken to avoid - /// concurrent writes, but it's up to the caller to decide whether it's safe to dereference the - /// pointer or not. + #[doc = "Retrieves the mutable pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr_mut(&self, key: impl Borrow) -> Option<*mut V> { get_ptr_mut(self.def.get(), key.borrow()) } + /// Inserts a key-value pair into the map. #[inline] pub fn insert( &self, @@ -168,6 +155,7 @@ impl LruHashMap { insert(self.def.get().cast(), key.borrow(), value.borrow(), flags) } + /// Removes a key from the map. #[inline] pub fn remove(&self, key: impl Borrow) -> Result<(), c_long> { remove(self.def.get().cast(), key.borrow()) @@ -210,35 +198,28 @@ impl PerCpuHashMap { } } - /// Retrieve the value associate with `key` from the map. - /// - /// # Safety - /// - /// Unless the map flag `BPF_F_NO_PREALLOC` is used, the kernel does not guarantee the atomicity - /// of `insert` or `remove`, and any element removed from the map might get aliased by another - /// element in the map, causing garbage to be read, or corruption in case of writes. + #[doc = "Retrieves the value associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub unsafe fn get(&self, key: impl Borrow) -> Option<&V> { unsafe { get(self.def.get(), key.borrow()) } } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, but this returns a raw pointer and it's up to the caller - /// to decide whether it's safe to dereference the pointer or not. + #[doc = "Retrieves the pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr(&self, key: impl Borrow) -> Option<*const V> { get_ptr(self.def.get(), key.borrow()) } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, and additionally cares should be taken to avoid - /// concurrent writes, but it's up to the caller to decide whether it's safe to dereference the - /// pointer or not. + #[doc = "Retrieves the mutable pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr_mut(&self, key: impl Borrow) -> Option<*mut V> { get_ptr_mut(self.def.get(), key.borrow()) } + /// Inserts a key-value pair into the map. #[inline] pub fn insert( &self, @@ -249,6 +230,7 @@ impl PerCpuHashMap { insert(self.def.get().cast(), key.borrow(), value.borrow(), flags) } + /// Removes a key from the map. #[inline] pub fn remove(&self, key: impl Borrow) -> Result<(), c_long> { remove(self.def.get().cast(), key.borrow()) @@ -291,35 +273,28 @@ impl LruPerCpuHashMap { } } - /// Retrieve the value associate with `key` from the map. - /// - /// # Safety - /// - /// Unless the map flag `BPF_F_NO_PREALLOC` is used, the kernel does not guarantee the atomicity - /// of `insert` or `remove`, and any element removed from the map might get aliased by another - /// element in the map, causing garbage to be read, or corruption in case of writes. + #[doc = "Retrieves the value associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub unsafe fn get(&self, key: impl Borrow) -> Option<&V> { unsafe { get(self.def.get(), key.borrow()) } } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, but this returns a raw pointer and it's up to the caller - /// to decide whether it's safe to dereference the pointer or not. + #[doc = "Retrieves the pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr(&self, key: impl Borrow) -> Option<*const V> { get_ptr(self.def.get(), key.borrow()) } - /// Retrieve the value associate with `key` from the map. - /// The same caveat as `get` applies, and additionally cares should be taken to avoid - /// concurrent writes, but it's up to the caller to decide whether it's safe to dereference the - /// pointer or not. + #[doc = "Retrieves the mutable pointer associated with `key` from the map."] + #[doc = include_str!("map_safety.md")] #[inline] pub fn get_ptr_mut(&self, key: impl Borrow) -> Option<*mut V> { get_ptr_mut(self.def.get(), key.borrow()) } + /// Inserts a key-value pair into the map. #[inline] pub fn insert( &self, @@ -330,6 +305,7 @@ impl LruPerCpuHashMap { insert(self.def.get().cast(), key.borrow(), value.borrow(), flags) } + /// Removes a key from the map. #[inline] pub fn remove(&self, key: impl Borrow) -> Result<(), c_long> { remove(self.def.get().cast(), key.borrow()) diff --git a/ebpf/aya-ebpf/src/maps/map_safety.md b/ebpf/aya-ebpf/src/maps/map_safety.md new file mode 100644 index 00000000..68214e53 --- /dev/null +++ b/ebpf/aya-ebpf/src/maps/map_safety.md @@ -0,0 +1,32 @@ +# Safety + +The pointer returned by a BPF map lookup is only stable until a concurrent +update or delete. In the kernel’s default *preallocated* mode (no +`BPF_F_NO_PREALLOC`), deleted elements are immediately recycled onto a +per-CPU freelist and may be reused by another update before an RCU grace +period elapses. Readers can therefore observe aliasing (values changing +underneath them) or, in rare cases, false-positive lookups when an old and +new key overlap. This behavior was reported on [LKML in 2018][lkml-2018]. + +Using `BPF_F_NO_PREALLOC` historically forced RCU-delayed freeing, but since +the switch to `bpf_mem_alloc`, both prealloc and no-prealloc modes may +recycle elements quickly; the main distinction now is +[memory vs. allocation overhead][htab-atomic-overwrite]. + +The [official kernel docs][kernel-doc-map-hash] describe `BPF_F_NO_PREALLOC` +as a *memory-usage knob*, not a safety guarantee. + +Patches in 2020 mitigated some issues (e.g. +[zero-filling reused per-CPU slots][zero-filling]) but did not eliminate reuse +races. + +A 2023 patch by Alexei proposed a fallback scheme to +[delay reuse via RCU grace periods in certain conditions][reuse-delay] (rather +than always reusing immediately). However, this approach is not universally +applied, and immediate reuse is still considered a “known quirk” in many cases. + +[lkml-2018]: https://lore.kernel.org/lkml/CAG48ez1-WZH55+Wa2vgwZY_hpZJfnDxMzxGLtuN1hG1z6hKf5Q@mail.gmail.com/T/ +[htab-atomic-overwrite]: https://lore.kernel.org/bpf/20250204082848.13471-2-hotforest@gmail.com/T/ +[kernel-doc-map-hash]: https://www.kernel.org/doc/html/v6.10/bpf/map_hash.html +[zero-filling]: https://lore.kernel.org/all/20201104112332.15191-1-david.verbeiren@tessares.net/ +[reuse-delay]: https://lore.kernel.org/bpf/20230706033447.54696-13-alexei.starovoitov@gmail.com/