From 3d57d358e40591acf23dfde740697fbfff026410 Mon Sep 17 00:00:00 2001 From: Dave Tucker Date: Mon, 29 Jul 2024 21:51:02 +0100 Subject: [PATCH] fix(aya): Fix PerfEventArray resize logic There was a logic bug in the previously merged patch where we set the correctly calculated max_entries size with the original. To fix this and prevent regressions a unit test was added. This highlighted that the original map definition needs to be mutated in order for the max_entries change to be properly applied. As such, this resize logic moved out of aya::sys into aya::maps Signed-off-by: Dave Tucker --- aya/src/maps/bloom_filter.rs | 3 +- aya/src/maps/lpm_trie.rs | 3 +- aya/src/maps/mod.rs | 103 ++++++++++++++++++++++++++++++++++- aya/src/sys/bpf.rs | 22 +------- xtask/public-api/aya.txt | 3 + 5 files changed, 108 insertions(+), 26 deletions(-) diff --git a/aya/src/maps/bloom_filter.rs b/aya/src/maps/bloom_filter.rs index bc060d87..ca295531 100644 --- a/aya/src/maps/bloom_filter.rs +++ b/aya/src/maps/bloom_filter.rs @@ -120,8 +120,7 @@ mod tests { #[test] fn test_try_from_wrong_map() { - // This is necessary to stop miri tripping over the PERF_EVENT_ARRAY - // logic and attempting to open a file to read number of online CPUs. + // Use any map type here other than BPF_MAP_TYPE_PERF_EVENT_ARRAY as it will trip miri let map = new_map(test_utils::new_obj_map::(BPF_MAP_TYPE_ARRAY)); let map = Map::Array(map); diff --git a/aya/src/maps/lpm_trie.rs b/aya/src/maps/lpm_trie.rs index 29ab5c29..e9a4b0b5 100644 --- a/aya/src/maps/lpm_trie.rs +++ b/aya/src/maps/lpm_trie.rs @@ -249,8 +249,7 @@ mod tests { #[test] fn test_try_from_wrong_map() { - // This is necessary to stop miri tripping over the PERF_EVENT_ARRAY - // logic and attempting to open a file to read number of online CPUs. + // Use any map type here other than BPF_MAP_TYPE_PERF_EVENT_ARRAY as it will trip miri let map = new_map(test_utils::new_obj_map::(BPF_MAP_TYPE_ARRAY)); let map = Map::Array(map); diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index 356662d8..431064d8 100644 --- a/aya/src/maps/mod.rs +++ b/aya/src/maps/mod.rs @@ -59,6 +59,7 @@ use std::{ ptr, }; +use aya_obj::generated::bpf_map_type; use libc::{getrlimit, rlim_t, rlimit, RLIMIT_MEMLOCK, RLIM_INFINITY}; use log::warn; use obj::maps::InvalidMapTypeError; @@ -172,6 +173,10 @@ pub enum MapError { #[error("the program is not loaded")] ProgramNotLoaded, + /// An IO error occurred + #[error(transparent)] + IoError(#[from] io::Error), + /// Syscall failed #[error(transparent)] SyscallError(#[from] SyscallError), @@ -550,12 +555,30 @@ pub struct MapData { impl MapData { /// Creates a new map with the provided `name` pub fn create( - obj: obj::Map, + mut obj: obj::Map, name: &str, btf_fd: Option>, ) -> Result { let c_name = CString::new(name).map_err(|_| MapError::InvalidName { name: name.into() })?; + // BPF_MAP_TYPE_PERF_EVENT_ARRAY's max_entries should not exceed the number of + // CPUs. + // + // By default, the newest versions of Aya, libbpf and cilium/ebpf define `max_entries` of + // `PerfEventArray` as `0`, with an intention to get it replaced with a correct value + // by the loader. + // + // We allow custom values (potentially coming either from older versions of aya-ebpf or + // programs written in C) as long as they don't exceed the number of CPUs. + // + // Otherwise, when the value is `0` or too large, we set it to the number of CPUs. + if obj.map_type() == bpf_map_type::BPF_MAP_TYPE_PERF_EVENT_ARRAY as u32 { + let ncpus = nr_cpus().map_err(MapError::IoError)? as u32; + if obj.max_entries() == 0 || obj.max_entries() > ncpus { + obj.set_max_entries(ncpus); + } + }; + #[cfg(not(test))] let kernel_version = KernelVersion::current().unwrap(); #[cfg(test)] @@ -1077,6 +1100,25 @@ mod test_utils { symbol_index: None, }) } + + pub(super) fn new_obj_map_with_max_entries( + map_type: bpf_map_type, + max_entries: u32, + ) -> obj::Map { + obj::Map::Legacy(LegacyMap { + def: bpf_map_def { + map_type: map_type as u32, + key_size: std::mem::size_of::() as u32, + value_size: 4, + max_entries, + ..Default::default() + }, + section_index: 0, + section_kind: EbpfSectionKind::Maps, + data: Vec::new(), + symbol_index: None, + }) + } } #[cfg(test)] @@ -1150,6 +1192,65 @@ mod tests { ); } + #[test] + #[cfg_attr(miri, ignore = "nr_cpus() opens a file on procfs that upsets miri")] + fn test_create_perf_event_array() { + override_syscall(|call| match call { + Syscall::Ebpf { + cmd: bpf_cmd::BPF_MAP_CREATE, + .. + } => Ok(crate::MockableFd::mock_signed_fd().into()), + _ => Err((-1, io::Error::from_raw_os_error(EFAULT))), + }); + + let ncpus = nr_cpus().unwrap(); + + // Create with max_entries > ncpus is clamped to ncpus + assert_matches!( + MapData::create(test_utils::new_obj_map_with_max_entries::( + crate::generated::bpf_map_type::BPF_MAP_TYPE_PERF_EVENT_ARRAY, + 65535, + ), "foo", None), + Ok(MapData { + obj, + fd, + }) => { + assert_eq!(fd.as_fd().as_raw_fd(), crate::MockableFd::mock_signed_fd()); + assert_eq!(obj.max_entries(), ncpus as u32) + } + ); + + // Create with max_entries = 0 is set to ncpus + assert_matches!( + MapData::create(test_utils::new_obj_map_with_max_entries::( + crate::generated::bpf_map_type::BPF_MAP_TYPE_PERF_EVENT_ARRAY, + 0, + ), "foo", None), + Ok(MapData { + obj, + fd, + }) => { + assert_eq!(fd.as_fd().as_raw_fd(), crate::MockableFd::mock_signed_fd()); + assert_eq!(obj.max_entries(), ncpus as u32) + } + ); + + // Create with max_entries < ncpus is unchanged + assert_matches!( + MapData::create(test_utils::new_obj_map_with_max_entries::( + crate::generated::bpf_map_type::BPF_MAP_TYPE_PERF_EVENT_ARRAY, + 1, + ), "foo", None), + Ok(MapData { + obj, + fd, + }) => { + assert_eq!(fd.as_fd().as_raw_fd(), crate::MockableFd::mock_signed_fd()); + assert_eq!(obj.max_entries(), 1) + } + ); + } + #[test] #[cfg_attr( miri, diff --git a/aya/src/sys/bpf.rs b/aya/src/sys/bpf.rs index f305f56b..f327d2bb 100644 --- a/aya/src/sys/bpf.rs +++ b/aya/src/sys/bpf.rs @@ -30,7 +30,7 @@ use crate::{ copy_instructions, }, sys::{syscall, SysResult, Syscall, SyscallError}, - util::{nr_cpus, KernelVersion}, + util::KernelVersion, Btf, Pod, VerifierLogLevel, BPF_OBJ_NAME_LEN, }; @@ -46,26 +46,6 @@ pub(crate) fn bpf_create_map( u.map_type = def.map_type(); u.key_size = def.key_size(); u.value_size = def.value_size(); - u.max_entries = def.max_entries(); - - // BPF_MAP_TYPE_PERF_EVENT_ARRAY's max_entries should not exceed the number of - // CPUs. - // - // By default, the newest versions of Aya, libbpf and cilium/ebpf define `max_entries` of - // `PerfEventArray` as `0`, with an intention to get it replaced with a correct value - // by the loader. - // - // We allow custom values (potentially coming either from older versions of aya-ebpf or - // programs written in C) as long as they don't exceed the number of CPUs. - // - // Otherwise, when the value is `0` or too large, we set it to the number of CPUs. - if def.map_type() == bpf_map_type::BPF_MAP_TYPE_PERF_EVENT_ARRAY as u32 { - let ncpus = nr_cpus().map_err(|e| (-1i64, e))? as u32; - if u.max_entries == 0 || u.max_entries > ncpus { - u.max_entries = ncpus; - } - }; - u.max_entries = def.max_entries(); u.map_flags = def.map_flags(); diff --git a/xtask/public-api/aya.txt b/xtask/public-api/aya.txt index 6515c488..ceba538f 100644 --- a/xtask/public-api/aya.txt +++ b/xtask/public-api/aya.txt @@ -1352,6 +1352,7 @@ pub aya::maps::MapError::InvalidName::name: alloc::string::String pub aya::maps::MapError::InvalidValueSize pub aya::maps::MapError::InvalidValueSize::expected: usize pub aya::maps::MapError::InvalidValueSize::size: usize +pub aya::maps::MapError::IoError(std::io::error::Error) pub aya::maps::MapError::KeyNotFound pub aya::maps::MapError::OutOfBounds pub aya::maps::MapError::OutOfBounds::index: u32 @@ -1372,6 +1373,8 @@ impl core::convert::From for aya::programs::ProgramError pub fn aya::programs::ProgramError::from(source: aya::maps::MapError) -> Self impl core::convert::From for aya::maps::MapError pub fn aya::maps::MapError::from(e: aya_obj::maps::InvalidMapTypeError) -> Self +impl core::convert::From for aya::maps::MapError +pub fn aya::maps::MapError::from(source: std::io::error::Error) -> Self impl core::error::Error for aya::maps::MapError pub fn aya::maps::MapError::source(&self) -> core::option::Option<&(dyn core::error::Error + 'static)> impl core::fmt::Debug for aya::maps::MapError