From 89bc255f1d14d72a61064b9b40b641b58f8970e0 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 16 Aug 2023 11:54:07 -0400 Subject: [PATCH 1/2] aya: MapData::fd is non-optional The primary driver of change here is that `MapData::create` is now a factory function that returns `Result` rather than mutating `&mut self`. The remaining changes are consequences of that change, the most notable of which is the removal of several errors which are no longer possible. --- aya-obj/src/relocation.rs | 36 ++-- aya/src/bpf.rs | 34 +--- aya/src/maps/array/array.rs | 6 +- aya/src/maps/array/per_cpu_array.rs | 6 +- aya/src/maps/array/program_array.rs | 6 +- aya/src/maps/bloom_filter.rs | 125 +++++-------- aya/src/maps/hash_map/hash_map.rs | 214 ++++++---------------- aya/src/maps/hash_map/mod.rs | 4 +- aya/src/maps/hash_map/per_cpu_hash_map.rs | 6 +- aya/src/maps/lpm_trie.rs | 166 ++++++----------- aya/src/maps/mod.rs | 156 +++++++--------- aya/src/maps/perf/perf_event_array.rs | 4 +- aya/src/maps/queue.rs | 6 +- aya/src/maps/sock/sock_hash.rs | 5 +- aya/src/maps/sock/sock_map.rs | 8 +- aya/src/maps/stack.rs | 6 +- aya/src/maps/stack_trace.rs | 3 +- aya/src/sys/bpf.rs | 14 +- test/integration-test/src/tests/rbpf.rs | 2 +- xtask/public-api/aya-obj.txt | 7 +- xtask/public-api/aya.txt | 9 +- 21 files changed, 284 insertions(+), 539 deletions(-) diff --git a/aya-obj/src/relocation.rs b/aya-obj/src/relocation.rs index fc02a79f..8269f6ca 100644 --- a/aya-obj/src/relocation.rs +++ b/aya-obj/src/relocation.rs @@ -73,15 +73,6 @@ pub enum RelocationError { address: u64, }, - /// Referenced map not created yet - #[error("the map `{name}` at section `{section_index}` has not been created")] - MapNotCreated { - /// The section index - section_index: usize, - /// The map name - name: String, - }, - /// Invalid relocation offset #[error("invalid offset `{offset}` applying relocation #{relocation_number}")] InvalidRelocationOffset { @@ -114,7 +105,7 @@ pub(crate) struct Symbol { impl Object { /// Relocates the map references - pub fn relocate_maps<'a, I: Iterator, &'a Map)>>( + pub fn relocate_maps<'a, I: Iterator>( &mut self, maps: I, text_sections: &HashSet, @@ -187,8 +178,8 @@ impl Object { fn relocate_maps<'a, I: Iterator>( fun: &mut Function, relocations: I, - maps_by_section: &HashMap, &Map)>, - maps_by_symbol: &HashMap, &Map)>, + maps_by_section: &HashMap, + maps_by_symbol: &HashMap, symbol_table: &HashMap, text_sections: &HashSet, ) -> Result<(), RelocationError> { @@ -230,7 +221,7 @@ fn relocate_maps<'a, I: Iterator>( continue; } - let (name, fd, map) = if let Some(m) = maps_by_symbol.get(&rel.symbol_index) { + let (_name, fd, map) = if let Some(m) = maps_by_symbol.get(&rel.symbol_index) { let map = &m.2; debug!( "relocating map by symbol index {:?}, kind {:?} at insn {ins_index} in section {}", @@ -266,18 +257,13 @@ fn relocate_maps<'a, I: Iterator>( }; debug_assert_eq!(map.section_index(), section_index); - let map_fd = fd.ok_or_else(|| RelocationError::MapNotCreated { - name: (*name).into(), - section_index, - })?; - if !map.data().is_empty() { instructions[ins_index].set_src_reg(BPF_PSEUDO_MAP_VALUE as u8); instructions[ins_index + 1].imm = instructions[ins_index].imm + sym.address as i32; } else { instructions[ins_index].set_src_reg(BPF_PSEUDO_MAP_FD as u8); } - instructions[ins_index].imm = map_fd; + instructions[ins_index].imm = *fd; } Ok(()) @@ -588,7 +574,7 @@ mod test { let maps_by_section = HashMap::new(); let map = fake_legacy_map(1); - let maps_by_symbol = HashMap::from([(1, ("test_map", Some(1), &map))]); + let maps_by_symbol = HashMap::from([(1, ("test_map", 1, &map))]); relocate_maps( &mut fun, @@ -642,8 +628,8 @@ mod test { let map_1 = fake_legacy_map(1); let map_2 = fake_legacy_map(2); let maps_by_symbol = HashMap::from([ - (1, ("test_map_1", Some(1), &map_1)), - (2, ("test_map_2", Some(2), &map_2)), + (1, ("test_map_1", 1, &map_1)), + (2, ("test_map_2", 2, &map_2)), ]); relocate_maps( @@ -683,7 +669,7 @@ mod test { let maps_by_section = HashMap::new(); let map = fake_btf_map(1); - let maps_by_symbol = HashMap::from([(1, ("test_map", Some(1), &map))]); + let maps_by_symbol = HashMap::from([(1, ("test_map", 1, &map))]); relocate_maps( &mut fun, @@ -737,8 +723,8 @@ mod test { let map_1 = fake_btf_map(1); let map_2 = fake_btf_map(2); let maps_by_symbol = HashMap::from([ - (1, ("test_map_1", Some(1), &map_1)), - (2, ("test_map_2", Some(2), &map_2)), + (1, ("test_map_1", 1, &map_1)), + (2, ("test_map_2", 2, &map_2)), ]); relocate_maps( diff --git a/aya/src/bpf.rs b/aya/src/bpf.rs index ee074dba..f5841f3f 100644 --- a/aya/src/bpf.rs +++ b/aya/src/bpf.rs @@ -4,7 +4,7 @@ use std::{ ffi::CString, fs, io, os::{ - fd::{AsFd as _, OwnedFd, RawFd}, + fd::{AsFd as _, OwnedFd}, raw::c_int, }, path::{Path, PathBuf}, @@ -475,35 +475,15 @@ impl<'a> BpfLoader<'a> { } } } - let mut map = MapData { - obj, - fd: None, - pinned: false, - }; - let fd = match map.obj.pinning() { + let btf_fd = btf_fd.as_deref().map(|fd| fd.as_fd()); + let mut map = match obj.pinning() { + PinningType::None => MapData::create(obj, &name, btf_fd)?, PinningType::ByName => { - let path = match &map_pin_path { - Some(p) => p, - None => return Err(BpfError::NoPinPath), - }; - // try to open map in case it's already pinned - match map.open_pinned(&name, path) { - Ok(fd) => { - map.pinned = true; - fd as RawFd - } - Err(_) => { - let fd = map.create(&name, btf_fd.as_deref().map(|f| f.as_fd()))?; - map.pin(&name, path).map_err(|error| MapError::PinError { - name: Some(name.to_string()), - error, - })?; - fd - } - } + let path = map_pin_path.as_ref().ok_or(BpfError::NoPinPath)?; + MapData::create_pinned(path, obj, &name, btf_fd)? } - PinningType::None => map.create(&name, btf_fd.as_deref().map(|f| f.as_fd()))?, }; + let fd = map.fd; if !map.obj.data().is_empty() && map.obj.section_kind() != BpfSectionKind::Bss { bpf_map_update_elem_ptr(fd, &0 as *const _, map.obj.data_mut().as_mut_ptr(), 0) .map_err(|(_, io_error)| SyscallError { diff --git a/aya/src/maps/array/array.rs b/aya/src/maps/array/array.rs index 7f242ccc..97bb46d6 100644 --- a/aya/src/maps/array/array.rs +++ b/aya/src/maps/array/array.rs @@ -39,8 +39,6 @@ impl, V: Pod> Array { let data = map.borrow(); check_kv_size::(data)?; - let _fd = data.fd_or_err()?; - Ok(Array { inner: map, _v: PhantomData, @@ -63,7 +61,7 @@ impl, V: Pod> Array { pub fn get(&self, index: &u32, flags: u64) -> Result { let data = self.inner.borrow(); check_bounds(data, *index)?; - let fd = data.fd_or_err()?; + let fd = data.fd; let value = bpf_map_lookup_elem(fd, index, flags).map_err(|(_, io_error)| SyscallError { @@ -90,7 +88,7 @@ impl, V: Pod> Array { pub fn set(&mut self, index: u32, value: impl Borrow, flags: u64) -> Result<(), MapError> { let data = self.inner.borrow_mut(); check_bounds(data, index)?; - let fd = data.fd_or_err()?; + let fd = data.fd; bpf_map_update_elem(fd, Some(&index), value.borrow(), flags).map_err(|(_, io_error)| { SyscallError { call: "bpf_map_update_elem", diff --git a/aya/src/maps/array/per_cpu_array.rs b/aya/src/maps/array/per_cpu_array.rs index 7cf1116a..6405af4a 100644 --- a/aya/src/maps/array/per_cpu_array.rs +++ b/aya/src/maps/array/per_cpu_array.rs @@ -58,8 +58,6 @@ impl, V: Pod> PerCpuArray { let data = map.borrow(); check_kv_size::(data)?; - let _fd = data.fd_or_err()?; - Ok(PerCpuArray { inner: map, _v: PhantomData, @@ -82,7 +80,7 @@ impl, V: Pod> PerCpuArray { pub fn get(&self, index: &u32, flags: u64) -> Result, MapError> { let data = self.inner.borrow(); check_bounds(data, *index)?; - let fd = data.fd_or_err()?; + let fd = data.fd; let value = bpf_map_lookup_elem_per_cpu(fd, index, flags).map_err(|(_, io_error)| { SyscallError { @@ -110,7 +108,7 @@ impl, V: Pod> PerCpuArray { pub fn set(&mut self, index: u32, values: PerCpuValues, flags: u64) -> Result<(), MapError> { let data = self.inner.borrow_mut(); check_bounds(data, index)?; - let fd = data.fd_or_err()?; + let fd = data.fd; bpf_map_update_elem_per_cpu(fd, &index, &values, flags).map_err(|(_, io_error)| { SyscallError { diff --git a/aya/src/maps/array/program_array.rs b/aya/src/maps/array/program_array.rs index 7a708527..e2152b56 100644 --- a/aya/src/maps/array/program_array.rs +++ b/aya/src/maps/array/program_array.rs @@ -56,8 +56,6 @@ impl> ProgramArray { let data = map.borrow(); check_kv_size::(data)?; - let _fd = data.fd_or_err()?; - Ok(ProgramArray { inner: map }) } @@ -76,7 +74,7 @@ impl> ProgramArray { pub fn set(&mut self, index: u32, program: &ProgramFd, flags: u64) -> Result<(), MapError> { let data = self.inner.borrow_mut(); check_bounds(data, index)?; - let fd = data.fd_or_err()?; + let fd = data.fd; let prog_fd = program.as_fd(); let prog_fd = prog_fd.as_raw_fd(); @@ -96,7 +94,7 @@ impl> ProgramArray { pub fn clear_index(&mut self, index: &u32) -> Result<(), MapError> { let data = self.inner.borrow_mut(); check_bounds(data, *index)?; - let fd = self.inner.borrow_mut().fd_or_err()?; + let fd = self.inner.borrow_mut().fd; bpf_map_delete_elem(fd, index) .map(|_| ()) diff --git a/aya/src/maps/bloom_filter.rs b/aya/src/maps/bloom_filter.rs index 03374b4e..6fc24ff4 100644 --- a/aya/src/maps/bloom_filter.rs +++ b/aya/src/maps/bloom_filter.rs @@ -41,8 +41,6 @@ impl, V: Pod> BloomFilter { let data = map.borrow(); check_v_size::(data)?; - let _ = data.fd_or_err()?; - Ok(BloomFilter { inner: map, _v: PhantomData, @@ -51,7 +49,7 @@ impl, V: Pod> BloomFilter { /// Query the existence of the element. pub fn contains(&self, mut value: &V, flags: u64) -> Result<(), MapError> { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; bpf_map_lookup_elem_ptr::(fd, None, &mut value, flags) .map_err(|(_, io_error)| SyscallError { @@ -64,7 +62,7 @@ impl, V: Pod> BloomFilter { /// Inserts a value into the map. pub fn insert(&self, value: impl Borrow, flags: u64) -> Result<(), MapError> { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; bpf_map_push_elem(fd, value.borrow(), flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_push_elem", io_error, @@ -106,17 +104,24 @@ mod tests { }) } + fn new_map(obj: obj::Map) -> MapData { + override_syscall(|call| match call { + Syscall::Bpf { + cmd: bpf_cmd::BPF_MAP_CREATE, + .. + } => Ok(1337), + call => panic!("unexpected syscall {:?}", call), + }); + MapData::create(obj, "foo", None).unwrap() + } + fn sys_error(value: i32) -> SysResult { Err((-1, io::Error::from_raw_os_error(value))) } #[test] fn test_wrong_value_size() { - let map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; + let map = new_map(new_obj_map()); assert_matches!( BloomFilter::<_, u16>::new(&map), Err(MapError::InvalidValueSize { @@ -128,25 +133,21 @@ mod tests { #[test] fn test_try_from_wrong_map() { - let map_data = MapData { - obj: obj::Map::Legacy(LegacyMap { - def: bpf_map_def { - map_type: BPF_MAP_TYPE_PERF_EVENT_ARRAY as u32, - key_size: 4, - value_size: 4, - max_entries: 1024, - ..Default::default() - }, - section_index: 0, - section_kind: BpfSectionKind::Maps, - symbol_index: None, - data: Vec::new(), - }), - fd: None, - pinned: false, - }; - - let map = Map::PerfEventArray(map_data); + let map = new_map(obj::Map::Legacy(LegacyMap { + def: bpf_map_def { + map_type: BPF_MAP_TYPE_PERF_EVENT_ARRAY as u32, + key_size: 4, + value_size: 4, + max_entries: 1024, + ..Default::default() + }, + section_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, + data: Vec::new(), + })); + + let map = Map::PerfEventArray(map); assert_matches!( BloomFilter::<_, u32>::try_from(&map), @@ -154,54 +155,28 @@ mod tests { ); } - #[test] - fn test_new_not_created() { - let mut map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; - - assert_matches!( - BloomFilter::<_, u32>::new(&mut map), - Err(MapError::NotCreated { .. }) - ); - } - #[test] fn test_new_ok() { - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let map = new_map(new_obj_map()); - assert!(BloomFilter::<_, u32>::new(&mut map).is_ok()); + assert!(BloomFilter::<_, u32>::new(&map).is_ok()); } #[test] fn test_try_from_ok() { - let map_data = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let map = new_map(new_obj_map()); - let map = Map::BloomFilter(map_data); + let map = Map::BloomFilter(map); assert!(BloomFilter::<_, u32>::try_from(&map).is_ok()) } #[test] fn test_insert_syscall_error() { - override_syscall(|_| sys_error(EFAULT)); - - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let mut map = new_map(new_obj_map()); let bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); + override_syscall(|_| sys_error(EFAULT)); + assert_matches!( bloom_filter.insert(1, 0), Err(MapError::SyscallError(SyscallError { call: "bpf_map_push_elem", io_error })) if io_error.raw_os_error() == Some(EFAULT) @@ -210,6 +185,9 @@ mod tests { #[test] fn test_insert_ok() { + let mut map = new_map(new_obj_map()); + let bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_UPDATE_ELEM, @@ -218,26 +196,16 @@ mod tests { _ => sys_error(EFAULT), }); - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - - let bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); assert!(bloom_filter.insert(0, 42).is_ok()); } #[test] fn test_contains_syscall_error() { - override_syscall(|_| sys_error(EFAULT)); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let map = new_map(new_obj_map()); let bloom_filter = BloomFilter::<_, u32>::new(&map).unwrap(); + override_syscall(|_| sys_error(EFAULT)); + assert_matches!( bloom_filter.contains(&1, 0), Err(MapError::SyscallError(SyscallError { call: "bpf_map_lookup_elem", io_error })) if io_error.raw_os_error() == Some(EFAULT) @@ -246,6 +214,9 @@ mod tests { #[test] fn test_contains_not_found() { + let map = new_map(new_obj_map()); + let bloom_filter = BloomFilter::<_, u32>::new(&map).unwrap(); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_LOOKUP_ELEM, @@ -253,12 +224,6 @@ mod tests { } => sys_error(ENOENT), _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - let bloom_filter = BloomFilter::<_, u32>::new(&map).unwrap(); assert_matches!(bloom_filter.contains(&1, 0), Err(MapError::ElementNotFound)); } diff --git a/aya/src/maps/hash_map/hash_map.rs b/aya/src/maps/hash_map/hash_map.rs index cbd2fc81..b2f9e547 100644 --- a/aya/src/maps/hash_map/hash_map.rs +++ b/aya/src/maps/hash_map/hash_map.rs @@ -42,7 +42,6 @@ impl, K: Pod, V: Pod> HashMap { pub(crate) fn new(map: T) -> Result, MapError> { let data = map.borrow(); check_kv_size::(data)?; - let _ = data.fd_or_err()?; Ok(HashMap { inner: map, @@ -53,7 +52,7 @@ impl, K: Pod, V: Pod> HashMap { /// Returns a copy of the value associated with the key. pub fn get(&self, key: &K, flags: u64) -> Result { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; let value = bpf_map_lookup_elem(fd, key, flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_lookup_elem", io_error, @@ -137,17 +136,24 @@ mod tests { }) } + fn new_map(obj: obj::Map) -> MapData { + override_syscall(|call| match call { + Syscall::Bpf { + cmd: bpf_cmd::BPF_MAP_CREATE, + .. + } => Ok(1337), + call => panic!("unexpected syscall {:?}", call), + }); + MapData::create(obj, "foo", None).unwrap() + } + fn sys_error(value: i32) -> SysResult { Err((-1, io::Error::from_raw_os_error(value))) } #[test] fn test_wrong_key_size() { - let map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; + let map = new_map(new_obj_map()); assert_matches!( HashMap::<_, u8, u32>::new(&map), Err(MapError::InvalidKeySize { @@ -159,11 +165,7 @@ mod tests { #[test] fn test_wrong_value_size() { - let map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; + let map = new_map(new_obj_map()); assert_matches!( HashMap::<_, u32, u16>::new(&map), Err(MapError::InvalidValueSize { @@ -175,13 +177,8 @@ mod tests { #[test] fn test_try_from_wrong_map() { - let map_data = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; - - let map = Map::Array(map_data); + let map = new_map(new_obj_map()); + let map = Map::Array(map); assert_matches!( HashMap::<_, u8, u32>::try_from(&map), Err(MapError::InvalidMapType { .. }) @@ -190,13 +187,8 @@ mod tests { #[test] fn test_try_from_wrong_map_values() { - let map_data = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; - - let map = Map::HashMap(map_data); + let map = new_map(new_obj_map()); + let map = Map::HashMap(map); assert_matches!( HashMap::<_, u32, u16>::try_from(&map), Err(MapError::InvalidValueSize { @@ -206,79 +198,46 @@ mod tests { ); } - #[test] - fn test_new_not_created() { - let mut map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; - - assert_matches!( - HashMap::<_, u32, u32>::new(&mut map), - Err(MapError::NotCreated { .. }) - ); - } - #[test] fn test_new_ok() { - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - - assert!(HashMap::<_, u32, u32>::new(&mut map).is_ok()); + let map = new_map(new_obj_map()); + assert!(HashMap::<_, u32, u32>::new(&map).is_ok()); } #[test] fn test_try_from_ok() { - let map_data = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - - let map = Map::HashMap(map_data); + let map = new_map(new_obj_map()); + let map = Map::HashMap(map); assert!(HashMap::<_, u32, u32>::try_from(&map).is_ok()) } #[test] fn test_try_from_ok_lru() { - let map_data = MapData { - obj: obj::Map::Legacy(LegacyMap { - def: bpf_map_def { - map_type: BPF_MAP_TYPE_LRU_HASH as u32, - key_size: 4, - value_size: 4, - max_entries: 1024, - ..Default::default() - }, - section_index: 0, - section_kind: BpfSectionKind::Maps, - symbol_index: None, - data: Vec::new(), - }), - fd: Some(42), - pinned: false, - }; - - let map = Map::HashMap(map_data); + let map = new_map(obj::Map::Legacy(LegacyMap { + def: bpf_map_def { + map_type: BPF_MAP_TYPE_LRU_HASH as u32, + key_size: 4, + value_size: 4, + max_entries: 1024, + ..Default::default() + }, + section_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, + data: Vec::new(), + })); + let map = Map::HashMap(map); assert!(HashMap::<_, u32, u32>::try_from(&map).is_ok()) } #[test] fn test_insert_syscall_error() { - override_syscall(|_| sys_error(EFAULT)); - - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let mut map = new_map(new_obj_map()); let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); + override_syscall(|_| sys_error(EFAULT)); + assert_matches!( hm.insert(1, 42, 0), Err(MapError::SyscallError(SyscallError { call: "bpf_map_update_elem", io_error })) if io_error.raw_os_error() == Some(EFAULT) @@ -287,6 +246,9 @@ mod tests { #[test] fn test_insert_ok() { + let mut map = new_map(new_obj_map()); + let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_UPDATE_ELEM, @@ -295,18 +257,14 @@ mod tests { _ => sys_error(EFAULT), }); - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); - assert!(hm.insert(1, 42, 0).is_ok()); } #[test] fn test_insert_boxed_ok() { + let mut map = new_map(new_obj_map()); + let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_UPDATE_ELEM, @@ -315,27 +273,16 @@ mod tests { _ => sys_error(EFAULT), }); - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); - assert!(hm.insert(Box::new(1), Box::new(42), 0).is_ok()); } #[test] fn test_remove_syscall_error() { - override_syscall(|_| sys_error(EFAULT)); - - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let mut map = new_map(new_obj_map()); let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); + override_syscall(|_| sys_error(EFAULT)); + assert_matches!( hm.remove(&1), Err(MapError::SyscallError(SyscallError { call: "bpf_map_delete_elem", io_error })) if io_error.raw_os_error() == Some(EFAULT) @@ -344,6 +291,9 @@ mod tests { #[test] fn test_remove_ok() { + let mut map = new_map(new_obj_map()); + let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_DELETE_ELEM, @@ -352,24 +302,13 @@ mod tests { _ => sys_error(EFAULT), }); - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - let mut hm = HashMap::<_, u32, u32>::new(&mut map).unwrap(); - assert!(hm.remove(&1).is_ok()); } #[test] fn test_get_syscall_error() { + let map = new_map(new_obj_map()); override_syscall(|_| sys_error(EFAULT)); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); assert_matches!( @@ -380,6 +319,7 @@ mod tests { #[test] fn test_get_not_found() { + let map = new_map(new_obj_map()); override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_LOOKUP_ELEM, @@ -387,11 +327,6 @@ mod tests { } => sys_error(ENOENT), _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); assert_matches!(hm.get(&1, 0), Err(MapError::KeyNotFound)); @@ -416,6 +351,7 @@ mod tests { #[test] fn test_keys_empty() { + let map = new_map(new_obj_map()); override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_GET_NEXT_KEY, @@ -423,11 +359,6 @@ mod tests { } => sys_error(ENOENT), _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let keys = hm.keys().collect::, _>>(); assert_matches!(keys, Ok(ks) if ks.is_empty()) @@ -463,6 +394,8 @@ mod tests { // to support stable as well. #[cfg_attr(miri, ignore)] fn test_keys() { + let map = new_map(new_obj_map()); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_GET_NEXT_KEY, @@ -471,11 +404,6 @@ mod tests { _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let keys = hm.keys().collect::, _>>().unwrap(); @@ -488,6 +416,7 @@ mod tests { // to support stable as well. #[cfg_attr(miri, ignore)] fn test_keys_error() { + let map = new_map(new_obj_map()); override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_GET_NEXT_KEY, @@ -503,11 +432,6 @@ mod tests { } _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let mut keys = hm.keys(); @@ -529,6 +453,7 @@ mod tests { // to support stable as well. #[cfg_attr(miri, ignore)] fn test_iter() { + let map = new_map(new_obj_map()); override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_GET_NEXT_KEY, @@ -540,11 +465,6 @@ mod tests { } => lookup_elem(attr), _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let items = hm.iter().collect::, _>>().unwrap(); assert_eq!(&items, &[(10, 100), (20, 200), (30, 300)]) @@ -556,6 +476,7 @@ mod tests { // to support stable as well. #[cfg_attr(miri, ignore)] fn test_iter_key_deleted() { + let map = new_map(new_obj_map()); override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_GET_NEXT_KEY, @@ -577,11 +498,6 @@ mod tests { } _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let items = hm.iter().collect::, _>>().unwrap(); @@ -594,6 +510,7 @@ mod tests { // to support stable as well. #[cfg_attr(miri, ignore)] fn test_iter_key_error() { + let map = new_map(new_obj_map()); override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_GET_NEXT_KEY, @@ -615,11 +532,6 @@ mod tests { } => lookup_elem(attr), _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let mut iter = hm.iter(); @@ -641,6 +553,7 @@ mod tests { // to support stable as well. #[cfg_attr(miri, ignore)] fn test_iter_value_error() { + let map = new_map(new_obj_map()); override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_GET_NEXT_KEY, @@ -662,11 +575,6 @@ mod tests { } _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; let hm = HashMap::<_, u32, u32>::new(&map).unwrap(); let mut iter = hm.iter(); diff --git a/aya/src/maps/hash_map/mod.rs b/aya/src/maps/hash_map/mod.rs index 62d3fc55..87fc0d70 100644 --- a/aya/src/maps/hash_map/mod.rs +++ b/aya/src/maps/hash_map/mod.rs @@ -20,7 +20,7 @@ pub(crate) fn insert( value: &V, flags: u64, ) -> Result<(), MapError> { - let fd = map.fd_or_err()?; + let fd = map.fd; bpf_map_update_elem(fd, Some(key), value, flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_update_elem", io_error, @@ -30,7 +30,7 @@ pub(crate) fn insert( } pub(crate) fn remove(map: &MapData, key: &K) -> Result<(), MapError> { - let fd = map.fd_or_err()?; + let fd = map.fd; bpf_map_delete_elem(fd, key) .map(|_| ()) .map_err(|(_, io_error)| { 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 979970cc..7fd90ec0 100644 --- a/aya/src/maps/hash_map/per_cpu_hash_map.rs +++ b/aya/src/maps/hash_map/per_cpu_hash_map.rs @@ -52,8 +52,6 @@ impl, K: Pod, V: Pod> PerCpuHashMap { let data = map.borrow(); check_kv_size::(data)?; - let _ = data.fd_or_err()?; - Ok(PerCpuHashMap { inner: map, _k: PhantomData, @@ -63,7 +61,7 @@ impl, K: Pod, V: Pod> PerCpuHashMap { /// Returns a slice of values - one for each CPU - associated with the key. pub fn get(&self, key: &K, flags: u64) -> Result, MapError> { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; let values = bpf_map_lookup_elem_per_cpu(fd, key, flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_lookup_elem", @@ -120,7 +118,7 @@ impl, K: Pod, V: Pod> PerCpuHashMap { values: PerCpuValues, flags: u64, ) -> Result<(), MapError> { - let fd = self.inner.borrow_mut().fd_or_err()?; + let fd = self.inner.borrow_mut().fd; bpf_map_update_elem_per_cpu(fd, key.borrow(), &values, flags).map_err( |(_, io_error)| SyscallError { call: "bpf_map_update_elem", diff --git a/aya/src/maps/lpm_trie.rs b/aya/src/maps/lpm_trie.rs index 864eb431..d05ec2cf 100644 --- a/aya/src/maps/lpm_trie.rs +++ b/aya/src/maps/lpm_trie.rs @@ -117,8 +117,6 @@ impl, K: Pod, V: Pod> LpmTrie { let data = map.borrow(); check_kv_size::, V>(data)?; - let _ = data.fd_or_err()?; - Ok(LpmTrie { inner: map, _k: PhantomData, @@ -128,7 +126,7 @@ impl, K: Pod, V: Pod> LpmTrie { /// Returns a copy of the value associated with the longest prefix matching key in the LpmTrie. pub fn get(&self, key: &Key, flags: u64) -> Result { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; let value = bpf_map_lookup_elem(fd, key, flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_lookup_elem", io_error, @@ -157,7 +155,7 @@ impl, K: Pod, V: Pod> LpmTrie { value: impl Borrow, flags: u64, ) -> Result<(), MapError> { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; bpf_map_update_elem(fd, Some(key), value.borrow(), flags).map_err(|(_, io_error)| { SyscallError { call: "bpf_map_update_elem", @@ -172,7 +170,7 @@ impl, K: Pod, V: Pod> LpmTrie { /// /// Both the prefix and data must match exactly - this method does not do a longest prefix match. pub fn remove(&mut self, key: &Key) -> Result<(), MapError> { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; bpf_map_delete_elem(fd, key) .map(|_| ()) .map_err(|(_, io_error)| { @@ -228,17 +226,24 @@ mod tests { }) } + fn new_map(obj: obj::Map) -> MapData { + override_syscall(|call| match call { + Syscall::Bpf { + cmd: bpf_cmd::BPF_MAP_CREATE, + .. + } => Ok(1337), + call => panic!("unexpected syscall {:?}", call), + }); + MapData::create(obj, "foo", None).unwrap() + } + fn sys_error(value: i32) -> SysResult { Err((-1, io::Error::from_raw_os_error(value))) } #[test] fn test_wrong_key_size() { - let map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; + let map = new_map(new_obj_map()); assert_matches!( LpmTrie::<_, u16, u32>::new(&map), Err(MapError::InvalidKeySize { @@ -250,11 +255,7 @@ mod tests { #[test] fn test_wrong_value_size() { - let map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; + let map = new_map(new_obj_map()); assert_matches!( LpmTrie::<_, u32, u16>::new(&map), Err(MapError::InvalidValueSize { @@ -266,25 +267,21 @@ mod tests { #[test] fn test_try_from_wrong_map() { - let map_data = MapData { - obj: obj::Map::Legacy(LegacyMap { - def: bpf_map_def { - map_type: BPF_MAP_TYPE_PERF_EVENT_ARRAY as u32, - key_size: 4, - value_size: 4, - max_entries: 1024, - ..Default::default() - }, - section_index: 0, - section_kind: BpfSectionKind::Maps, - symbol_index: None, - data: Vec::new(), - }), - fd: None, - pinned: false, - }; - - let map = Map::PerfEventArray(map_data); + let map = new_map(obj::Map::Legacy(LegacyMap { + def: bpf_map_def { + map_type: BPF_MAP_TYPE_PERF_EVENT_ARRAY as u32, + key_size: 4, + value_size: 4, + max_entries: 1024, + ..Default::default() + }, + section_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, + data: Vec::new(), + })); + + let map = Map::PerfEventArray(map); assert_matches!( LpmTrie::<_, u32, u32>::try_from(&map), @@ -292,55 +289,30 @@ mod tests { ); } - #[test] - fn test_new_not_created() { - let mut map = MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - }; - - assert_matches!( - LpmTrie::<_, u32, u32>::new(&mut map), - Err(MapError::NotCreated { .. }) - ); - } - #[test] fn test_new_ok() { - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let map = new_map(new_obj_map()); - assert!(LpmTrie::<_, u32, u32>::new(&mut map).is_ok()); + assert!(LpmTrie::<_, u32, u32>::new(&map).is_ok()); } #[test] fn test_try_from_ok() { - let map_data = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let map = new_map(new_obj_map()); - let map = Map::LpmTrie(map_data); + let map = Map::LpmTrie(map); assert!(LpmTrie::<_, u32, u32>::try_from(&map).is_ok()) } #[test] fn test_insert_syscall_error() { - override_syscall(|_| sys_error(EFAULT)); - - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let mut map = new_map(new_obj_map()); let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); let ipaddr = Ipv4Addr::new(8, 8, 8, 8); let key = Key::new(16, u32::from(ipaddr).to_be()); + + override_syscall(|_| sys_error(EFAULT)); + assert_matches!( trie.insert(&key, 1, 0), Err(MapError::SyscallError(SyscallError { call: "bpf_map_update_elem", io_error })) if io_error.raw_os_error() == Some(EFAULT) @@ -349,6 +321,11 @@ mod tests { #[test] fn test_insert_ok() { + let mut map = new_map(new_obj_map()); + let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); + let ipaddr = Ipv4Addr::new(8, 8, 8, 8); + let key = Key::new(16, u32::from(ipaddr).to_be()); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_UPDATE_ELEM, @@ -357,30 +334,18 @@ mod tests { _ => sys_error(EFAULT), }); - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - - let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); - let ipaddr = Ipv4Addr::new(8, 8, 8, 8); - let key = Key::new(16, u32::from(ipaddr).to_be()); assert!(trie.insert(&key, 1, 0).is_ok()); } #[test] fn test_remove_syscall_error() { - override_syscall(|_| sys_error(EFAULT)); - - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let mut map = new_map(new_obj_map()); let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); let ipaddr = Ipv4Addr::new(8, 8, 8, 8); let key = Key::new(16, u32::from(ipaddr).to_be()); + + override_syscall(|_| sys_error(EFAULT)); + assert_matches!( trie.remove(&key), Err(MapError::SyscallError(SyscallError { call: "bpf_map_delete_elem", io_error })) if io_error.raw_os_error() == Some(EFAULT) @@ -389,6 +354,11 @@ mod tests { #[test] fn test_remove_ok() { + let mut map = new_map(new_obj_map()); + let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); + let ipaddr = Ipv4Addr::new(8, 8, 8, 8); + let key = Key::new(16, u32::from(ipaddr).to_be()); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_DELETE_ELEM, @@ -397,29 +367,18 @@ mod tests { _ => sys_error(EFAULT), }); - let mut map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - let mut trie = LpmTrie::<_, u32, u32>::new(&mut map).unwrap(); - let ipaddr = Ipv4Addr::new(8, 8, 8, 8); - let key = Key::new(16, u32::from(ipaddr).to_be()); assert!(trie.remove(&key).is_ok()); } #[test] fn test_get_syscall_error() { - override_syscall(|_| sys_error(EFAULT)); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; + let map = new_map(new_obj_map()); let trie = LpmTrie::<_, u32, u32>::new(&map).unwrap(); let ipaddr = Ipv4Addr::new(8, 8, 8, 8); let key = Key::new(16, u32::from(ipaddr).to_be()); + override_syscall(|_| sys_error(EFAULT)); + assert_matches!( trie.get(&key, 0), Err(MapError::SyscallError(SyscallError { call: "bpf_map_lookup_elem", io_error })) if io_error.raw_os_error() == Some(EFAULT) @@ -428,6 +387,11 @@ mod tests { #[test] fn test_get_not_found() { + let map = new_map(new_obj_map()); + let trie = LpmTrie::<_, u32, u32>::new(&map).unwrap(); + let ipaddr = Ipv4Addr::new(8, 8, 8, 8); + let key = Key::new(16, u32::from(ipaddr).to_be()); + override_syscall(|call| match call { Syscall::Bpf { cmd: bpf_cmd::BPF_MAP_LOOKUP_ELEM, @@ -435,14 +399,6 @@ mod tests { } => sys_error(ENOENT), _ => sys_error(EFAULT), }); - let map = MapData { - obj: new_obj_map(), - fd: Some(42), - pinned: false, - }; - let trie = LpmTrie::<_, u32, u32>::new(&map).unwrap(); - let ipaddr = Ipv4Addr::new(8, 8, 8, 8); - let key = Key::new(16, u32::from(ipaddr).to_be()); assert_matches!(trie.get(&key, 0), Err(MapError::KeyNotFound)); } diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index 28fd688e..8458989c 100644 --- a/aya/src/maps/mod.rs +++ b/aya/src/maps/mod.rs @@ -103,17 +103,6 @@ pub enum MapError { name: String, }, - /// The map has not been created - #[error("the map has not been created")] - NotCreated, - - /// The map has already been created - #[error("the map `{name}` has already been created")] - AlreadyCreated { - /// Map name - name: String, - }, - /// Failed to create map #[error("failed to create map `{name}` with code {code}")] CreateError { @@ -480,7 +469,7 @@ pub(crate) fn check_v_size(map: &MapData) -> Result<(), MapError> { #[derive(Debug)] pub struct MapData { pub(crate) obj: obj::Map, - pub(crate) fd: Option, + pub(crate) fd: RawFd, /// Indicates if this map has been pinned to bpffs pub pinned: bool, } @@ -488,22 +477,18 @@ pub struct MapData { impl MapData { /// Creates a new map with the provided `name` pub fn create( - &mut self, + obj: obj::Map, name: &str, btf_fd: Option>, - ) -> Result { - if self.fd.is_some() { - return Err(MapError::AlreadyCreated { name: name.into() }); - } - + ) -> Result { let c_name = CString::new(name).map_err(|_| MapError::InvalidName { name: name.into() })?; #[cfg(not(test))] let kernel_version = KernelVersion::current().unwrap(); #[cfg(test)] let kernel_version = KernelVersion::new(0xff, 0xff, 0xff); - let fd = bpf_create_map(&c_name, &self.obj, btf_fd, kernel_version).map_err( - |(code, io_error)| { + let fd = + bpf_create_map(&c_name, &obj, btf_fd, kernel_version).map_err(|(code, io_error)| { if kernel_version < KernelVersion::new(5, 11, 0) { maybe_warn_rlimit(); } @@ -513,28 +498,42 @@ impl MapData { code, io_error, } - }, - )?; + })?; - Ok(*self.fd.insert(fd as RawFd)) + Ok(Self { + obj, + fd: fd as RawFd, + pinned: false, + }) } - pub(crate) fn open_pinned>( - &mut self, - name: &str, + pub(crate) fn create_pinned>( path: P, - ) -> Result { - if self.fd.is_some() { - return Err(MapError::AlreadyCreated { name: name.into() }); - } + obj: obj::Map, + name: &str, + btf_fd: Option>, + ) -> Result { + // try to open map in case it's already pinned let map_path = path.as_ref().join(name); let path_string = CString::new(map_path.to_str().unwrap()).unwrap(); - let fd = bpf_get_object(&path_string).map_err(|(_, io_error)| SyscallError { + match bpf_get_object(&path_string).map_err(|(_, io_error)| SyscallError { call: "BPF_OBJ_GET", io_error, - })?; - - Ok(*self.fd.insert(fd.into_raw_fd())) + }) { + Ok(fd) => Ok(Self { + obj, + fd: fd.into_raw_fd(), + pinned: false, + }), + Err(_) => { + let mut map = Self::create(obj, name, btf_fd)?; + map.pin(name, path).map_err(|error| MapError::PinError { + name: Some(name.into()), + error, + })?; + Ok(map) + } + } } /// Loads a map from a pinned path in bpffs. @@ -558,7 +557,7 @@ impl MapData { Ok(MapData { obj: parse_map_info(info, PinningType::ByName), - fd: Some(fd.into_raw_fd()), + fd: fd.into_raw_fd(), pinned: true, }) } @@ -573,59 +572,54 @@ impl MapData { Ok(MapData { obj: parse_map_info(info, PinningType::None), - fd: Some(fd.into_raw_fd()), + fd: fd.into_raw_fd(), pinned: false, }) } - pub(crate) fn fd_or_err(&self) -> Result { - self.fd.ok_or(MapError::NotCreated) - } - pub(crate) fn pin>(&mut self, name: &str, path: P) -> Result<(), PinError> { - if self.pinned { + let Self { fd, pinned, obj: _ } = self; + if *pinned { return Err(PinError::AlreadyPinned { name: name.into() }); } let map_path = path.as_ref().join(name); - let fd = self.fd.ok_or(PinError::NoFd { - name: name.to_string(), - })?; let path_string = CString::new(map_path.to_string_lossy().into_owned()).map_err(|e| { PinError::InvalidPinPath { error: e.to_string(), } })?; - bpf_pin_object(fd, &path_string).map_err(|(_, io_error)| SyscallError { + bpf_pin_object(*fd, &path_string).map_err(|(_, io_error)| SyscallError { call: "BPF_OBJ_PIN", io_error, })?; - self.pinned = true; + *pinned = true; Ok(()) } /// Returns the file descriptor of the map. /// /// Can be converted to [`RawFd`] using [`AsRawFd`]. - pub fn fd(&self) -> Option { - self.fd.map(MapFd) + pub fn fd(&self) -> MapFd { + MapFd(self.fd) } } impl Drop for MapData { fn drop(&mut self) { // TODO: Replace this with an OwnedFd once that is stabilized. - if let Some(fd) = self.fd.take() { - unsafe { libc::close(fd) }; - } + // + // SAFETY: `drop` is only called once. + unsafe { libc::close(self.fd) }; } } impl Clone for MapData { - fn clone(&self) -> MapData { - MapData { - obj: self.obj.clone(), - fd: self.fd.map(|fd| unsafe { libc::dup(fd) }), - pinned: self.pinned, + fn clone(&self) -> Self { + let Self { obj, fd, pinned } = self; + Self { + obj: obj.clone(), + fd: unsafe { libc::dup(*fd) }, + pinned: *pinned, } } } @@ -664,14 +658,7 @@ impl Iterator for MapKeys<'_, K> { return None; } - let fd = match self.map.fd_or_err() { - Ok(fd) => fd, - Err(e) => { - self.err = true; - return Some(Err(e)); - } - }; - + let fd = self.map.fd; let key = bpf_map_get_next_key(fd, self.key.as_ref()).map_err(|(_, io_error)| SyscallError { call: "bpf_map_get_next_key", @@ -854,14 +841,6 @@ mod tests { }) } - fn new_map() -> MapData { - MapData { - obj: new_obj_map(), - fd: None, - pinned: false, - } - } - #[test] fn test_create() { override_syscall(|call| match call { @@ -872,12 +851,13 @@ mod tests { _ => Err((-1, io::Error::from_raw_os_error(EFAULT))), }); - let mut map = new_map(); - assert_matches!(map.create("foo", None), Ok(42)); - assert_eq!(map.fd, Some(42)); assert_matches!( - map.create("foo", None), - Err(MapError::AlreadyCreated { .. }) + MapData::create(new_obj_map(), "foo", None), + Ok(MapData { + obj: _, + fd: 42, + pinned: false + }) ); } @@ -885,19 +865,13 @@ mod tests { fn test_create_failed() { override_syscall(|_| Err((-42, io::Error::from_raw_os_error(EFAULT)))); - let mut map = new_map(); - let ret = map.create("foo", None); - assert_matches!(ret, Err(MapError::CreateError { .. })); - if let Err(MapError::CreateError { - name, - code, - io_error, - }) = ret - { - assert_eq!(name, "foo"); - assert_eq!(code, -42); - assert_eq!(io_error.raw_os_error(), Some(EFAULT)); - } - assert_eq!(map.fd, None); + assert_matches!( + MapData::create(new_obj_map(), "foo", None), + Err(MapError::CreateError { name, code, io_error }) => { + assert_eq!(name, "foo"); + assert_eq!(code, -42); + assert_eq!(io_error.raw_os_error(), Some(EFAULT)); + } + ); } } diff --git a/aya/src/maps/perf/perf_event_array.rs b/aya/src/maps/perf/perf_event_array.rs index 3d6a8103..7d647164 100644 --- a/aya/src/maps/perf/perf_event_array.rs +++ b/aya/src/maps/perf/perf_event_array.rs @@ -162,8 +162,6 @@ pub struct PerfEventArray { impl> PerfEventArray { pub(crate) fn new(map: T) -> Result, MapError> { - let _fd = map.borrow().fd_or_err()?; - Ok(PerfEventArray { map: Arc::new(map), page_size: page_size(), @@ -184,7 +182,7 @@ impl + Borrow> PerfEventArray { // this cannot fail as new() checks that the fd is open let map_data: &MapData = self.map.deref().borrow(); - let map_fd = map_data.fd_or_err().unwrap(); + let map_fd = map_data.fd; let buf = PerfBuffer::open(index, self.page_size, page_count.unwrap_or(2))?; bpf_map_update_elem(map_fd, Some(&index), &buf.as_raw_fd(), 0) .map_err(|(_, io_error)| io_error)?; diff --git a/aya/src/maps/queue.rs b/aya/src/maps/queue.rs index 6b635dcb..78b81ec2 100644 --- a/aya/src/maps/queue.rs +++ b/aya/src/maps/queue.rs @@ -38,8 +38,6 @@ impl, V: Pod> Queue { let data = map.borrow(); check_kv_size::<(), V>(data)?; - let _fd = data.fd_or_err()?; - Ok(Queue { inner: map, _v: PhantomData, @@ -62,7 +60,7 @@ impl, V: Pod> Queue { /// Returns [`MapError::ElementNotFound`] if the queue is empty, [`MapError::SyscallError`] /// if `bpf_map_lookup_and_delete_elem` fails. pub fn pop(&mut self, flags: u64) -> Result { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; let value = bpf_map_lookup_and_delete_elem::(fd, None, flags).map_err( |(_, io_error)| SyscallError { @@ -79,7 +77,7 @@ impl, V: Pod> Queue { /// /// [`MapError::SyscallError`] if `bpf_map_update_elem` fails. pub fn push(&mut self, value: impl Borrow, flags: u64) -> Result<(), MapError> { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; bpf_map_push_elem(fd, value.borrow(), flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_push_elem", io_error, diff --git a/aya/src/maps/sock/sock_hash.rs b/aya/src/maps/sock/sock_hash.rs index 0e87aa98..1904ec1e 100644 --- a/aya/src/maps/sock/sock_hash.rs +++ b/aya/src/maps/sock/sock_hash.rs @@ -72,7 +72,6 @@ impl, K: Pod> SockHash { pub(crate) fn new(map: T) -> Result, MapError> { let data = map.borrow(); check_kv_size::(data)?; - let _ = data.fd_or_err()?; Ok(SockHash { inner: map, @@ -82,7 +81,7 @@ impl, K: Pod> SockHash { /// Returns the fd of the socket stored at the given key. pub fn get(&self, key: &K, flags: u64) -> Result { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; let value = bpf_map_lookup_elem(fd, key, flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_lookup_elem", io_error, @@ -107,7 +106,7 @@ impl, K: Pod> SockHash { /// The returned file descriptor can be used to attach programs that work with /// socket maps, like [`SkMsg`](crate::programs::SkMsg) and [`SkSkb`](crate::programs::SkSkb). pub fn fd(&self) -> Result { - Ok(SockMapFd(self.inner.borrow().fd_or_err()?)) + Ok(SockMapFd(self.inner.borrow().fd)) } } diff --git a/aya/src/maps/sock/sock_map.rs b/aya/src/maps/sock/sock_map.rs index b6877827..52574f23 100644 --- a/aya/src/maps/sock/sock_map.rs +++ b/aya/src/maps/sock/sock_map.rs @@ -49,8 +49,6 @@ impl> SockMap { let data = map.borrow(); check_kv_size::(data)?; - let _fd = data.fd_or_err()?; - Ok(SockMap { inner: map }) } @@ -65,7 +63,7 @@ impl> SockMap { /// The returned file descriptor can be used to attach programs that work with /// socket maps, like [`SkMsg`](crate::programs::SkMsg) and [`SkSkb`](crate::programs::SkSkb). pub fn fd(&self) -> Result { - Ok(SockMapFd(self.inner.borrow().fd_or_err()?)) + Ok(SockMapFd(self.inner.borrow().fd)) } } @@ -73,7 +71,7 @@ impl> SockMap { /// Stores a socket into the map. pub fn set(&mut self, index: u32, socket: &I, flags: u64) -> Result<(), MapError> { let data = self.inner.borrow_mut(); - let fd = data.fd_or_err()?; + let fd = data.fd; check_bounds(data, index)?; bpf_map_update_elem(fd, Some(&index), &socket.as_raw_fd(), flags).map_err( |(_, io_error)| SyscallError { @@ -87,7 +85,7 @@ impl> SockMap { /// Removes the socket stored at `index` from the map. pub fn clear_index(&mut self, index: &u32) -> Result<(), MapError> { let data = self.inner.borrow_mut(); - let fd = data.fd_or_err()?; + let fd = data.fd; check_bounds(data, *index)?; bpf_map_delete_elem(fd, index) .map(|_| ()) diff --git a/aya/src/maps/stack.rs b/aya/src/maps/stack.rs index 8004e486..0cc9b1d8 100644 --- a/aya/src/maps/stack.rs +++ b/aya/src/maps/stack.rs @@ -38,8 +38,6 @@ impl, V: Pod> Stack { let data = map.borrow(); check_kv_size::<(), V>(data)?; - let _fd = data.fd_or_err()?; - Ok(Stack { inner: map, _v: PhantomData, @@ -62,7 +60,7 @@ impl, V: Pod> Stack { /// Returns [`MapError::ElementNotFound`] if the stack is empty, [`MapError::SyscallError`] /// if `bpf_map_lookup_and_delete_elem` fails. pub fn pop(&mut self, flags: u64) -> Result { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; let value = bpf_map_lookup_and_delete_elem::(fd, None, flags).map_err( |(_, io_error)| SyscallError { @@ -79,7 +77,7 @@ impl, V: Pod> Stack { /// /// [`MapError::SyscallError`] if `bpf_map_update_elem` fails. pub fn push(&mut self, value: impl Borrow, flags: u64) -> Result<(), MapError> { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; bpf_map_update_elem(fd, None::<&u32>, value.borrow(), flags).map_err(|(_, io_error)| { SyscallError { call: "bpf_map_update_elem", diff --git a/aya/src/maps/stack_trace.rs b/aya/src/maps/stack_trace.rs index 8d3add5a..8f84a09e 100644 --- a/aya/src/maps/stack_trace.rs +++ b/aya/src/maps/stack_trace.rs @@ -89,7 +89,6 @@ impl> StackTraceMap { if size > max_stack_depth * mem::size_of::() { return Err(MapError::InvalidValueSize { size, expected }); } - let _fd = data.fd_or_err()?; Ok(StackTraceMap { inner: map, @@ -104,7 +103,7 @@ impl> StackTraceMap { /// Returns [`MapError::KeyNotFound`] if there is no stack trace with the /// given `stack_id`, or [`MapError::SyscallError`] if `bpf_map_lookup_elem` fails. pub fn get(&self, stack_id: &u32, flags: u64) -> Result { - let fd = self.inner.borrow().fd_or_err()?; + let fd = self.inner.borrow().fd; let mut frames = vec![0; self.max_stack_depth]; bpf_map_lookup_elem_ptr(fd, Some(stack_id), frames.as_mut_ptr(), flags) diff --git a/aya/src/sys/bpf.rs b/aya/src/sys/bpf.rs index 546bf0c4..23fcf8e3 100644 --- a/aya/src/sys/bpf.rs +++ b/aya/src/sys/bpf.rs @@ -704,8 +704,8 @@ pub(crate) fn is_bpf_global_data_supported() -> bool { let mut insns = copy_instructions(prog).unwrap(); - let mut map_data = MapData { - obj: obj::Map::Legacy(LegacyMap { + let map = MapData::create( + obj::Map::Legacy(LegacyMap { def: bpf_map_def { map_type: bpf_map_type::BPF_MAP_TYPE_ARRAY as u32, key_size: 4, @@ -718,12 +718,12 @@ pub(crate) fn is_bpf_global_data_supported() -> bool { symbol_index: None, data: Vec::new(), }), - fd: None, - pinned: false, - }; + "aya_global", + None, + ); - if let Ok(map_fd) = map_data.create("aya_global", None) { - insns[0].imm = map_fd; + if let Ok(map) = map { + insns[0].imm = map.fd; let gpl = b"GPL\0"; u.license = gpl.as_ptr() as u64; diff --git a/test/integration-test/src/tests/rbpf.rs b/test/integration-test/src/tests/rbpf.rs index 44e648d1..0a85adbf 100644 --- a/test/integration-test/src/tests/rbpf.rs +++ b/test/integration-test/src/tests/rbpf.rs @@ -74,7 +74,7 @@ fn use_map_with_rbpf() { object .relocate_maps( maps.iter() - .map(|(s, (fd, map))| (s.as_ref() as &str, Some(*fd), map)), + .map(|(s, (fd, map))| (s.as_ref() as &str, *fd, map)), &text_sections, ) .expect("Relocation failed"); diff --git a/xtask/public-api/aya-obj.txt b/xtask/public-api/aya-obj.txt index a2f9d001..574862f4 100644 --- a/xtask/public-api/aya-obj.txt +++ b/xtask/public-api/aya-obj.txt @@ -5972,7 +5972,7 @@ impl aya_obj::Object pub fn aya_obj::Object::relocate_btf(&mut self, target_btf: &aya_obj::btf::Btf) -> core::result::Result<(), aya_obj::btf::BtfRelocationError> impl aya_obj::Object pub fn aya_obj::Object::relocate_calls(&mut self, text_sections: &std::collections::hash::set::HashSet) -> core::result::Result<(), aya_obj::relocation::BpfRelocationError> -pub fn aya_obj::Object::relocate_maps<'a, I: core::iter::traits::iterator::Iterator, &'a aya_obj::maps::Map)>>(&mut self, maps: I, text_sections: &std::collections::hash::set::HashSet) -> core::result::Result<(), aya_obj::relocation::BpfRelocationError> +pub fn aya_obj::Object::relocate_maps<'a, I: core::iter::traits::iterator::Iterator>(&mut self, maps: I, text_sections: &std::collections::hash::set::HashSet) -> core::result::Result<(), aya_obj::relocation::BpfRelocationError> impl core::clone::Clone for aya_obj::Object pub fn aya_obj::Object::clone(&self) -> aya_obj::Object impl core::fmt::Debug for aya_obj::Object @@ -6288,9 +6288,6 @@ pub enum aya_obj::relocation::RelocationError pub aya_obj::relocation::RelocationError::InvalidRelocationOffset pub aya_obj::relocation::RelocationError::InvalidRelocationOffset::offset: u64 pub aya_obj::relocation::RelocationError::InvalidRelocationOffset::relocation_number: usize -pub aya_obj::relocation::RelocationError::MapNotCreated -pub aya_obj::relocation::RelocationError::MapNotCreated::name: alloc::string::String -pub aya_obj::relocation::RelocationError::MapNotCreated::section_index: usize pub aya_obj::relocation::RelocationError::SectionNotFound pub aya_obj::relocation::RelocationError::SectionNotFound::section_index: usize pub aya_obj::relocation::RelocationError::SectionNotFound::symbol_index: usize @@ -6687,7 +6684,7 @@ impl aya_obj::Object pub fn aya_obj::Object::relocate_btf(&mut self, target_btf: &aya_obj::btf::Btf) -> core::result::Result<(), aya_obj::btf::BtfRelocationError> impl aya_obj::Object pub fn aya_obj::Object::relocate_calls(&mut self, text_sections: &std::collections::hash::set::HashSet) -> core::result::Result<(), aya_obj::relocation::BpfRelocationError> -pub fn aya_obj::Object::relocate_maps<'a, I: core::iter::traits::iterator::Iterator, &'a aya_obj::maps::Map)>>(&mut self, maps: I, text_sections: &std::collections::hash::set::HashSet) -> core::result::Result<(), aya_obj::relocation::BpfRelocationError> +pub fn aya_obj::Object::relocate_maps<'a, I: core::iter::traits::iterator::Iterator>(&mut self, maps: I, text_sections: &std::collections::hash::set::HashSet) -> core::result::Result<(), aya_obj::relocation::BpfRelocationError> impl core::clone::Clone for aya_obj::Object pub fn aya_obj::Object::clone(&self) -> aya_obj::Object impl core::fmt::Debug for aya_obj::Object diff --git a/xtask/public-api/aya.txt b/xtask/public-api/aya.txt index 0b2a7374..28813796 100644 --- a/xtask/public-api/aya.txt +++ b/xtask/public-api/aya.txt @@ -966,8 +966,6 @@ pub fn aya::maps::Map::borrow_mut(&mut self) -> &mut T impl core::convert::From for aya::maps::Map pub fn aya::maps::Map::from(t: T) -> T pub enum aya::maps::MapError -pub aya::maps::MapError::AlreadyCreated -pub aya::maps::MapError::AlreadyCreated::name: alloc::string::String pub aya::maps::MapError::CreateError pub aya::maps::MapError::CreateError::code: libc::unix::linux_like::linux::gnu::b64::x86_64::not_x32::c_long pub aya::maps::MapError::CreateError::io_error: std::io::error::Error @@ -984,7 +982,6 @@ pub aya::maps::MapError::InvalidValueSize pub aya::maps::MapError::InvalidValueSize::expected: usize pub aya::maps::MapError::InvalidValueSize::size: usize pub aya::maps::MapError::KeyNotFound -pub aya::maps::MapError::NotCreated pub aya::maps::MapError::OutOfBounds pub aya::maps::MapError::OutOfBounds::index: u32 pub aya::maps::MapError::OutOfBounds::max_entries: u32 @@ -1226,12 +1223,12 @@ pub fn aya::maps::lpm_trie::LpmTrie::from(t: T) -> T pub struct aya::maps::MapData pub aya::maps::MapData::pinned: bool impl aya::maps::MapData -pub fn aya::maps::MapData::create(&mut self, name: &str, btf_fd: core::option::Option>) -> core::result::Result -pub fn aya::maps::MapData::fd(&self) -> core::option::Option +pub fn aya::maps::MapData::create(obj: aya_obj::maps::Map, name: &str, btf_fd: core::option::Option>) -> core::result::Result +pub fn aya::maps::MapData::fd(&self) -> aya::maps::MapFd pub fn aya::maps::MapData::from_fd(fd: std::os::fd::owned::OwnedFd) -> core::result::Result pub fn aya::maps::MapData::from_pin>(path: P) -> core::result::Result impl core::clone::Clone for aya::maps::MapData -pub fn aya::maps::MapData::clone(&self) -> aya::maps::MapData +pub fn aya::maps::MapData::clone(&self) -> Self impl core::ops::drop::Drop for aya::maps::MapData pub fn aya::maps::MapData::drop(&mut self) impl core::fmt::Debug for aya::maps::MapData From a31544b6e77d6868d950820ad31fc1fe8ed3666b Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 16 Aug 2023 12:20:32 -0400 Subject: [PATCH 2/2] maps: BloomFilter::insert takes &mut self This is consistent with all the other maps. --- aya/src/maps/bloom_filter.rs | 15 ++++++++++----- xtask/public-api/aya.txt | 6 ++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/aya/src/maps/bloom_filter.rs b/aya/src/maps/bloom_filter.rs index 6fc24ff4..84bff986 100644 --- a/aya/src/maps/bloom_filter.rs +++ b/aya/src/maps/bloom_filter.rs @@ -1,5 +1,8 @@ //! A Bloom Filter. -use std::{borrow::Borrow, marker::PhantomData}; +use std::{ + borrow::{Borrow, BorrowMut}, + marker::PhantomData, +}; use crate::{ maps::{check_v_size, MapData, MapError}, @@ -59,10 +62,12 @@ impl, V: Pod> BloomFilter { .ok_or(MapError::ElementNotFound)?; Ok(()) } +} +impl, V: Pod> BloomFilter { /// Inserts a value into the map. - pub fn insert(&self, value: impl Borrow, flags: u64) -> Result<(), MapError> { - let fd = self.inner.borrow().fd; + pub fn insert(&mut self, value: impl Borrow, flags: u64) -> Result<(), MapError> { + let fd = self.inner.borrow_mut().fd; bpf_map_push_elem(fd, value.borrow(), flags).map_err(|(_, io_error)| SyscallError { call: "bpf_map_push_elem", io_error, @@ -173,7 +178,7 @@ mod tests { #[test] fn test_insert_syscall_error() { let mut map = new_map(new_obj_map()); - let bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); + let mut bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); override_syscall(|_| sys_error(EFAULT)); @@ -186,7 +191,7 @@ mod tests { #[test] fn test_insert_ok() { let mut map = new_map(new_obj_map()); - let bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); + let mut bloom_filter = BloomFilter::<_, u32>::new(&mut map).unwrap(); override_syscall(|call| match call { Syscall::Bpf { diff --git a/xtask/public-api/aya.txt b/xtask/public-api/aya.txt index 28813796..ebf7699f 100644 --- a/xtask/public-api/aya.txt +++ b/xtask/public-api/aya.txt @@ -126,7 +126,8 @@ pub mod aya::maps::bloom_filter pub struct aya::maps::bloom_filter::BloomFilter impl, V: aya::Pod> aya::maps::bloom_filter::BloomFilter pub fn aya::maps::bloom_filter::BloomFilter::contains(&self, value: &V, flags: u64) -> core::result::Result<(), aya::maps::MapError> -pub fn aya::maps::bloom_filter::BloomFilter::insert(&self, value: impl core::borrow::Borrow, flags: u64) -> core::result::Result<(), aya::maps::MapError> +impl, V: aya::Pod> aya::maps::bloom_filter::BloomFilter +pub fn aya::maps::bloom_filter::BloomFilter::insert(&mut self, value: impl core::borrow::Borrow, flags: u64) -> core::result::Result<(), aya::maps::MapError> impl<'a, V: aya::Pod> core::convert::TryFrom<&'a aya::maps::Map> for aya::maps::bloom_filter::BloomFilter<&'a aya::maps::MapData, V> pub type aya::maps::bloom_filter::BloomFilter<&'a aya::maps::MapData, V>::Error = aya::maps::MapError pub fn aya::maps::bloom_filter::BloomFilter<&'a aya::maps::MapData, V>::try_from(map: &'a aya::maps::Map) -> core::result::Result, aya::maps::MapError> @@ -1101,7 +1102,8 @@ pub fn aya::maps::perf::AsyncPerfEventArray::from(t: T) -> T pub struct aya::maps::BloomFilter impl, V: aya::Pod> aya::maps::bloom_filter::BloomFilter pub fn aya::maps::bloom_filter::BloomFilter::contains(&self, value: &V, flags: u64) -> core::result::Result<(), aya::maps::MapError> -pub fn aya::maps::bloom_filter::BloomFilter::insert(&self, value: impl core::borrow::Borrow, flags: u64) -> core::result::Result<(), aya::maps::MapError> +impl, V: aya::Pod> aya::maps::bloom_filter::BloomFilter +pub fn aya::maps::bloom_filter::BloomFilter::insert(&mut self, value: impl core::borrow::Borrow, flags: u64) -> core::result::Result<(), aya::maps::MapError> impl<'a, V: aya::Pod> core::convert::TryFrom<&'a aya::maps::Map> for aya::maps::bloom_filter::BloomFilter<&'a aya::maps::MapData, V> pub type aya::maps::bloom_filter::BloomFilter<&'a aya::maps::MapData, V>::Error = aya::maps::MapError pub fn aya::maps::bloom_filter::BloomFilter<&'a aya::maps::MapData, V>::try_from(map: &'a aya::maps::Map) -> core::result::Result, aya::maps::MapError>