From 18970369e27228c45117b125724a22d8999ec1fc Mon Sep 17 00:00:00 2001 From: Thia Wyrod Date: Fri, 3 Dec 2021 12:07:49 -0800 Subject: [PATCH] aya: Remove unnecessary unsafe markers on map iteration. Map iteration can yield stale keys and values by virtue of sharing a data structure with BPF programs which can modify it. However, all accesses remain perfectly safe and will not cause memory corruption or data races. --- aya/src/maps/array/array.rs | 4 ++-- aya/src/maps/array/per_cpu_array.rs | 4 ++-- aya/src/maps/array/program_array.rs | 2 +- aya/src/maps/hash_map/hash_map.rs | 29 ++++++++++------------- aya/src/maps/hash_map/per_cpu_hash_map.rs | 8 +++---- aya/src/maps/mod.rs | 15 +++++------- aya/src/maps/sock/sock_hash.rs | 8 +++---- aya/src/maps/sock/sock_map.rs | 2 +- aya/src/maps/stack_trace.rs | 2 +- 9 files changed, 34 insertions(+), 40 deletions(-) diff --git a/aya/src/maps/array/array.rs b/aya/src/maps/array/array.rs index 6b41cc5d..d6f1988a 100644 --- a/aya/src/maps/array/array.rs +++ b/aya/src/maps/array/array.rs @@ -94,7 +94,7 @@ impl, V: Pod> Array { /// An iterator over the elements of the array. The iterator item type is `Result`. - pub unsafe fn iter(&self) -> impl Iterator> + '_ { + pub fn iter(&self) -> impl Iterator> + '_ { (0..self.len()).map(move |i| self.get(&i, 0)) } @@ -134,7 +134,7 @@ impl, V: Pod> IterableMap for Array { &self.inner } - unsafe fn get(&self, index: &u32) -> Result { + fn get(&self, index: &u32) -> Result { self.get(index, 0) } } diff --git a/aya/src/maps/array/per_cpu_array.rs b/aya/src/maps/array/per_cpu_array.rs index 099778e8..ed91dfaf 100644 --- a/aya/src/maps/array/per_cpu_array.rs +++ b/aya/src/maps/array/per_cpu_array.rs @@ -113,7 +113,7 @@ impl, V: Pod> PerCpuArray { /// An iterator over the elements of the array. The iterator item type is /// `Result, MapError>`. - pub unsafe fn iter(&self) -> impl Iterator, MapError>> + '_ { + pub fn iter(&self) -> impl Iterator, MapError>> + '_ { (0..self.len()).map(move |i| self.get(&i, 0)) } @@ -153,7 +153,7 @@ impl, V: Pod> IterableMap> for PerCp &self.inner } - unsafe fn get(&self, index: &u32) -> Result, MapError> { + fn get(&self, index: &u32) -> Result, MapError> { self.get(index, 0) } } diff --git a/aya/src/maps/array/program_array.rs b/aya/src/maps/array/program_array.rs index 3a3f7d02..0ceff2b9 100644 --- a/aya/src/maps/array/program_array.rs +++ b/aya/src/maps/array/program_array.rs @@ -79,7 +79,7 @@ impl> ProgramArray { /// An iterator over the indices of the array that point to a program. The iterator item type /// is `Result`. - pub unsafe fn indices(&self) -> MapKeys<'_, u32> { + pub fn indices(&self) -> MapKeys<'_, u32> { MapKeys::new(&self.inner) } diff --git a/aya/src/maps/hash_map/hash_map.rs b/aya/src/maps/hash_map/hash_map.rs index 9567fd6c..3b44c10c 100644 --- a/aya/src/maps/hash_map/hash_map.rs +++ b/aya/src/maps/hash_map/hash_map.rs @@ -61,7 +61,7 @@ impl, K: Pod, V: Pod> HashMap { } /// Returns a copy of the value associated with the key. - pub unsafe fn get(&self, key: &K, flags: u64) -> Result { + pub fn get(&self, key: &K, flags: u64) -> Result { let fd = self.inner.deref().fd_or_err()?; let value = bpf_map_lookup_elem(fd, key, flags).map_err(|(code, io_error)| { MapError::SyscallError { @@ -75,13 +75,13 @@ impl, K: Pod, V: Pod> HashMap { /// An iterator visiting all key-value pairs in arbitrary order. The /// iterator item type is `Result<(K, V), MapError>`. - pub unsafe fn iter(&self) -> MapIter<'_, K, V, Self> { + pub fn iter(&self) -> MapIter<'_, K, V, Self> { MapIter::new(self) } /// An iterator visiting all keys in arbitrary order. The iterator element /// type is `Result`. - pub unsafe fn keys(&self) -> MapKeys<'_, K> { + pub fn keys(&self) -> MapKeys<'_, K> { MapKeys::new(&self.inner) } } @@ -103,7 +103,7 @@ impl, K: Pod, V: Pod> IterableMap for HashMap Result { + fn get(&self, key: &K) -> Result { HashMap::get(self, key, 0) } } @@ -373,7 +373,7 @@ mod tests { let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); assert!(matches!( - unsafe { hm.get(&1, 0) }, + hm.get(&1, 0), Err(MapError::SyscallError { call, code: -1, io_error }) if call == "bpf_map_lookup_elem" && io_error.raw_os_error() == Some(EFAULT) )); } @@ -394,10 +394,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - assert!(matches!( - unsafe { hm.get(&1, 0) }, - Err(MapError::KeyNotFound) - )); + assert!(matches!(hm.get(&1, 0), Err(MapError::KeyNotFound))); } fn bpf_key(attr: &bpf_attr) -> Option { @@ -432,7 +429,7 @@ mod tests { pinned: false, }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - let keys = unsafe { hm.keys() }.collect::, _>>(); + let keys = hm.keys().collect::, _>>(); assert!(matches!(keys, Ok(ks) if ks.is_empty())) } @@ -477,7 +474,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - let keys = unsafe { hm.keys() }.collect::, _>>().unwrap(); + let keys = hm.keys().collect::, _>>().unwrap(); assert_eq!(&keys, &[10, 20, 30]) } @@ -505,7 +502,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - let mut keys = unsafe { hm.keys() }; + let mut keys = hm.keys(); assert!(matches!(keys.next(), Some(Ok(10)))); assert!(matches!(keys.next(), Some(Ok(20)))); assert!(matches!( @@ -534,7 +531,7 @@ mod tests { pinned: false, }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - let items = unsafe { hm.iter() }.collect::, _>>().unwrap(); + let items = hm.iter().collect::, _>>().unwrap(); assert_eq!(&items, &[(10, 100), (20, 200), (30, 300)]) } @@ -568,7 +565,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - let items = unsafe { hm.iter() }.collect::, _>>().unwrap(); + let items = hm.iter().collect::, _>>().unwrap(); assert_eq!(&items, &[(10, 100), (30, 300)]) } @@ -602,7 +599,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - let mut iter = unsafe { hm.iter() }; + let mut iter = hm.iter(); assert!(matches!(iter.next(), Some(Ok((10, 100))))); assert!(matches!(iter.next(), Some(Ok((20, 200))))); assert!(matches!( @@ -642,7 +639,7 @@ mod tests { }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); - let mut iter = unsafe { hm.iter() }; + let mut iter = hm.iter(); assert!(matches!(iter.next(), Some(Ok((10, 100))))); assert!(matches!( iter.next(), diff --git a/aya/src/maps/hash_map/per_cpu_hash_map.rs b/aya/src/maps/hash_map/per_cpu_hash_map.rs index 77eb76a2..a76d1c23 100644 --- a/aya/src/maps/hash_map/per_cpu_hash_map.rs +++ b/aya/src/maps/hash_map/per_cpu_hash_map.rs @@ -73,7 +73,7 @@ impl, K: Pod, V: Pod> PerCpuHashMap { } /// Returns a slice of values - one for each CPU - associated with the key. - pub unsafe fn get(&self, key: &K, flags: u64) -> Result, MapError> { + pub fn get(&self, key: &K, flags: u64) -> Result, MapError> { let fd = self.inner.deref().fd_or_err()?; let values = bpf_map_lookup_elem_per_cpu(fd, key, flags).map_err(|(code, io_error)| { MapError::SyscallError { @@ -87,13 +87,13 @@ impl, K: Pod, V: Pod> PerCpuHashMap { /// An iterator visiting all key-value pairs in arbitrary order. The /// iterator item type is `Result<(K, PerCpuValues), MapError>`. - pub unsafe fn iter(&self) -> MapIter<'_, K, PerCpuValues, Self> { + pub fn iter(&self) -> MapIter<'_, K, PerCpuValues, Self> { MapIter::new(self) } /// An iterator visiting all keys in arbitrary order. The iterator element /// type is `Result`. - pub unsafe fn keys(&self) -> MapKeys<'_, K> { + pub fn keys(&self) -> MapKeys<'_, K> { MapKeys::new(&self.inner) } } @@ -154,7 +154,7 @@ impl, K: Pod, V: Pod> IterableMap> &self.inner } - unsafe fn get(&self, key: &K) -> Result, MapError> { + fn get(&self, key: &K) -> Result, MapError> { PerCpuHashMap::get(self, key, 0) } } diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index 46e3889f..3f124182 100644 --- a/aya/src/maps/mod.rs +++ b/aya/src/maps/mod.rs @@ -237,7 +237,7 @@ impl Drop for Map { pub trait IterableMap { fn map(&self) -> ⤅ - unsafe fn get(&self, key: &K) -> Result; + fn get(&self, key: &K) -> Result; } /// Iterator returned by `map.keys()`. @@ -317,14 +317,11 @@ impl> Iterator for MapIter<'_, K, V, I> { fn next(&mut self) -> Option { loop { match self.keys.next() { - Some(Ok(key)) => { - let value = unsafe { self.map.get(&key) }; - match value { - Ok(value) => return Some(Ok((key, value))), - Err(MapError::KeyNotFound) => continue, - Err(e) => return Some(Err(e)), - } - } + Some(Ok(key)) => match self.map.get(&key) { + Ok(value) => return Some(Ok((key, value))), + Err(MapError::KeyNotFound) => continue, + Err(e) => return Some(Err(e)), + }, Some(Err(e)) => return Some(Err(e)), None => return None, } diff --git a/aya/src/maps/sock/sock_hash.rs b/aya/src/maps/sock/sock_hash.rs index d71e9f84..59f3f54a 100644 --- a/aya/src/maps/sock/sock_hash.rs +++ b/aya/src/maps/sock/sock_hash.rs @@ -87,7 +87,7 @@ impl, K: Pod> SockHash { } /// Returns the fd of the socket stored at the given key. - pub unsafe fn get(&self, key: &K, flags: u64) -> Result { + pub fn get(&self, key: &K, flags: u64) -> Result { let fd = self.inner.deref().fd_or_err()?; let value = bpf_map_lookup_elem(fd, key, flags).map_err(|(code, io_error)| { MapError::SyscallError { @@ -101,13 +101,13 @@ impl, K: Pod> SockHash { /// An iterator visiting all key-value pairs in arbitrary order. The /// iterator item type is `Result<(K, V), MapError>`. - pub unsafe fn iter(&self) -> MapIter<'_, K, RawFd, Self> { + pub fn iter(&self) -> MapIter<'_, K, RawFd, Self> { MapIter::new(self) } /// An iterator visiting all keys in arbitrary order. The iterator element /// type is `Result`. - pub unsafe fn keys(&self) -> MapKeys<'_, K> { + pub fn keys(&self) -> MapKeys<'_, K> { MapKeys::new(&self.inner) } } @@ -129,7 +129,7 @@ impl, K: Pod> IterableMap for SockHash { &self.inner } - unsafe fn get(&self, key: &K) -> Result { + fn get(&self, key: &K) -> Result { SockHash::get(self, key, 0) } } diff --git a/aya/src/maps/sock/sock_map.rs b/aya/src/maps/sock/sock_map.rs index 13a46b1e..0a0b9c13 100644 --- a/aya/src/maps/sock/sock_map.rs +++ b/aya/src/maps/sock/sock_map.rs @@ -71,7 +71,7 @@ impl> SockMap { /// An iterator over the indices of the array that point to a program. The iterator item type /// is `Result`. - pub unsafe fn indices(&self) -> MapKeys<'_, u32> { + pub fn indices(&self) -> MapKeys<'_, u32> { MapKeys::new(&self.inner) } diff --git a/aya/src/maps/stack_trace.rs b/aya/src/maps/stack_trace.rs index 6a24b8fc..cfaa7e71 100644 --- a/aya/src/maps/stack_trace.rs +++ b/aya/src/maps/stack_trace.rs @@ -156,7 +156,7 @@ impl> IterableMap for StackTraceMap { &self.inner } - unsafe fn get(&self, index: &u32) -> Result { + fn get(&self, index: &u32) -> Result { self.get(index, 0) } }