From 401ea5e8482ece34b6c88de85ec474bdfc577fd4 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sun, 9 Apr 2023 22:14:28 +1000 Subject: [PATCH] aya, aya-obj: refactor map relocations Clearly split the code between `.maps`, `maps` and data maps (bss, data, rodata). Sprinkle comments. Remove MapKind which was effectively only needed since we used to have one variant - BpfSectionKind::Data - to represent all data maps. Instead add explicit BpfSectionKind::{Data, Rodata, Bss} variants and match on those when we initialize maps. --- aya-obj/src/maps.rs | 70 +++++++----------- aya-obj/src/obj.rs | 114 +++++++++++------------------- aya-obj/src/relocation.rs | 71 ++++++++++++------- aya/src/bpf.rs | 6 +- aya/src/maps/bloom_filter.rs | 13 ++-- aya/src/maps/hash_map/hash_map.rs | 13 ++-- aya/src/maps/lpm_trie.rs | 13 ++-- aya/src/maps/mod.rs | 6 +- 8 files changed, 133 insertions(+), 173 deletions(-) diff --git a/aya-obj/src/maps.rs b/aya-obj/src/maps.rs index a6c3c96c..22a88da9 100644 --- a/aya-obj/src/maps.rs +++ b/aya-obj/src/maps.rs @@ -2,7 +2,10 @@ use core::mem; -use crate::thiserror::{self, Error}; +use crate::{ + thiserror::{self, Error}, + BpfSectionKind, +}; use alloc::vec::Vec; /// Invalid map type encontered @@ -139,33 +142,6 @@ pub struct bpf_map_def { /// The first five __u32 of `bpf_map_def` must be defined. pub(crate) const MINIMUM_MAP_SIZE: usize = mem::size_of::() * 5; -/// Kinds of maps -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub enum MapKind { - /// A map holding `.bss` section data - Bss, - /// A map holding `.data` section data - Data, - /// A map holding `.rodata` section data - Rodata, - /// Other maps - Other, -} - -impl From<&str> for MapKind { - fn from(s: &str) -> Self { - if s == ".bss" { - MapKind::Bss - } else if s.starts_with(".data") { - MapKind::Data - } else if s.starts_with(".rodata") { - MapKind::Rodata - } else { - MapKind::Other - } - } -} - /// Map data defined in `maps` or `.maps` sections #[derive(Debug, Clone)] pub enum Map { @@ -248,14 +224,6 @@ impl Map { } } - /// Returns the map kind - pub fn kind(&self) -> MapKind { - match self { - Map::Legacy(m) => m.kind, - Map::Btf(m) => m.kind, - } - } - /// Returns the section index pub fn section_index(&self) -> usize { match self { @@ -264,11 +232,22 @@ impl Map { } } - /// Returns the symbol index - pub fn symbol_index(&self) -> usize { + /// Returns the section kind. + pub fn section_kind(&self) -> BpfSectionKind { + match self { + Map::Legacy(m) => m.section_kind, + Map::Btf(_) => BpfSectionKind::BtfMaps, + } + } + + /// Returns the symbol index. + /// + /// This is `None` for data maps (.bss, .data and .rodata) since those don't + /// need symbols in order to be relocated. + pub fn symbol_index(&self) -> Option { match self { Map::Legacy(m) => m.symbol_index, - Map::Btf(m) => m.symbol_index, + Map::Btf(m) => Some(m.symbol_index), } } } @@ -283,12 +262,16 @@ pub struct LegacyMap { pub def: bpf_map_def, /// The section index pub section_index: usize, - /// The symbol index - pub symbol_index: usize, + /// The section kind + pub section_kind: BpfSectionKind, + /// The symbol index. + /// + /// This is None for data maps (.bss .data and .rodata). We don't need + /// symbols to relocate those since they don't contain multiple maps, but + /// are just a flat array of bytes. + pub symbol_index: Option, /// The map data pub data: Vec, - /// The map kind - pub kind: MapKind, } /// A BTF-defined map, most likely from a `.maps` section. @@ -298,6 +281,5 @@ pub struct BtfMap { pub def: BtfMapDef, pub(crate) section_index: usize, pub(crate) symbol_index: usize, - pub(crate) kind: MapKind, pub(crate) data: Vec, } diff --git a/aya-obj/src/obj.rs b/aya-obj/src/obj.rs index 7f040b24..0d110fff 100644 --- a/aya-obj/src/obj.rs +++ b/aya-obj/src/obj.rs @@ -15,7 +15,7 @@ use object::{ }; use crate::{ - maps::{BtfMap, LegacyMap, Map, MapKind, MINIMUM_MAP_SIZE}, + maps::{BtfMap, LegacyMap, Map, MINIMUM_MAP_SIZE}, relocation::*, thiserror::{self, Error}, util::HashMap, @@ -794,7 +794,6 @@ impl Object { def, section_index: section.index.0, symbol_index, - kind: MapKind::Other, data: Vec::new(), }), ); @@ -820,9 +819,9 @@ impl Object { self.section_sizes .insert(section.name.to_owned(), section.size); match section.kind { - BpfSectionKind::Data => { + BpfSectionKind::Data | BpfSectionKind::Rodata | BpfSectionKind::Bss => { self.maps - .insert(section.name.to_string(), parse_map(§ion, section.name)?); + .insert(section.name.to_string(), parse_data_map_section(§ion)?); } BpfSectionKind::Text => self.parse_text_section(section)?, BpfSectionKind::Btf => self.parse_btf(§ion)?, @@ -909,16 +908,16 @@ fn parse_maps_section<'a, I: Iterator>( name.to_string(), Map::Legacy(LegacyMap { section_index: section.index.0, - symbol_index: sym.index, + section_kind: section.kind, + symbol_index: Some(sym.index), def, data: Vec::new(), - kind: MapKind::Other, }), ); have_symbols = true; } if !have_symbols { - return Err(ParseError::NoSymbolsForMapSection); + return Err(ParseError::NoSymbolsForMapsSection); } Ok(()) @@ -995,17 +994,32 @@ pub enum ParseError { NoBTF, } -#[derive(Debug)] -enum BpfSectionKind { +/// The kind of an ELF section. +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub enum BpfSectionKind { + /// Undefined Undefined, + /// `maps` Maps, + /// `.maps` BtfMaps, + /// A program section Program, + /// `.data` Data, + /// `.rodata` + Rodata, + /// `.bss` + Bss, + /// `.text` Text, + /// `.BTF` Btf, + /// `.BTF.ext` BtfExt, + /// `license` License, + /// `version` Version, } @@ -1172,10 +1186,11 @@ impl From for u32 { } } -fn parse_map(section: &Section, name: &str) -> Result { - let kind = MapKind::from(name); - let (def, data) = match kind { - MapKind::Bss | MapKind::Data | MapKind::Rodata => { +// Parsed '.bss' '.data' and '.rodata' sections. These sections are arrays of +// bytes and are relocated based on their section index. +fn parse_data_map_section(section: &Section) -> Result { + let (def, data) = match section.kind { + BpfSectionKind::Bss | BpfSectionKind::Data | BpfSectionKind::Rodata => { let def = bpf_map_def { map_type: BPF_MAP_TYPE_ARRAY as u32, key_size: mem::size_of::() as u32, @@ -1183,7 +1198,7 @@ fn parse_map(section: &Section, name: &str) -> Result { // .bss will always have data.len() == 0 value_size: section.size as u32, max_entries: 1, - map_flags: if kind == MapKind::Rodata { + map_flags: if section.kind == BpfSectionKind::Rodata { BPF_F_RDONLY_PROG } else { 0 @@ -1192,14 +1207,15 @@ fn parse_map(section: &Section, name: &str) -> Result { }; (def, section.data.to_vec()) } - MapKind::Other => (parse_map_def(name, section.data)?, Vec::new()), + _ => unreachable!(), }; Ok(Map::Legacy(LegacyMap { section_index: section.index.0, - symbol_index: 0, + section_kind: section.kind, + // Data maps don't require symbols to be relocated + symbol_index: None, def, data, - kind, })) } @@ -1319,8 +1335,6 @@ pub fn parse_map_info(info: bpf_map_info, pinned: PinningType) -> Map { section_index: 0, symbol_index: 0, data: Vec::new(), - // We should never be loading the .bss or .data or .rodata FDs - kind: MapKind::Other, }) } else { Map::Legacy(LegacyMap { @@ -1334,10 +1348,9 @@ pub fn parse_map_info(info: bpf_map_info, pinned: PinningType) -> Map { id: info.id, }, section_index: 0, - symbol_index: 0, + symbol_index: None, + section_kind: BpfSectionKind::Undefined, data: Vec::new(), - // We should never be loading the .bss or .data or .rodata FDs - kind: MapKind::Other, }) } } @@ -1523,65 +1536,21 @@ mod tests { assert_eq!(parse_map_def("foo", &buf).unwrap(), def); } - #[test] - fn test_parse_map_error() { - assert!(matches!( - parse_map(&fake_section(BpfSectionKind::Maps, "maps/foo", &[]), "foo",), - Err(ParseError::InvalidMapDefinition { .. }) - )); - } - - #[test] - fn test_parse_map() { - assert!(matches!( - parse_map( - &fake_section( - BpfSectionKind::Maps, - "maps/foo", - bytes_of(&bpf_map_def { - map_type: 1, - key_size: 2, - value_size: 3, - max_entries: 4, - map_flags: 5, - id: 0, - pinning: PinningType::None, - }) - ), - "foo" - ), - Ok(Map::Legacy(LegacyMap{ - section_index: 0, - def: bpf_map_def { - map_type: 1, - key_size: 2, - value_size: 3, - max_entries: 4, - map_flags: 5, - id: 0, - pinning: PinningType::None, - }, - data, - .. - })) if data.is_empty() - )) - } - #[test] fn test_parse_map_data() { let map_data = b"map data"; assert!(matches!( - parse_map( + parse_data_map_section( &fake_section( BpfSectionKind::Data, ".bss", map_data, ), - ".bss" ), Ok(Map::Legacy(LegacyMap { section_index: 0, - symbol_index: 0, + section_kind: BpfSectionKind::Data, + symbol_index: None, def: bpf_map_def { map_type: _map_type, key_size: 4, @@ -1592,8 +1561,7 @@ mod tests { pinning: PinningType::None, }, data, - kind - })) if data == map_data && value_size == map_data.len() as u32 && kind == MapKind::Bss + })) if data == map_data && value_size == map_data.len() as u32 )) } @@ -2240,9 +2208,9 @@ mod tests { pinning: PinningType::None, }, section_index: 1, - symbol_index: 1, + section_kind: BpfSectionKind::Rodata, + symbol_index: Some(1), data: vec![0, 0, 0], - kind: MapKind::Rodata, }), ); obj.symbols_by_index.insert( diff --git a/aya-obj/src/relocation.rs b/aya-obj/src/relocation.rs index 8b0b9170..6b41b9eb 100644 --- a/aya-obj/src/relocation.rs +++ b/aya-obj/src/relocation.rs @@ -15,6 +15,7 @@ use crate::{ obj::{Function, Object, Program}, thiserror::{self, Error}, util::HashMap, + BpfSectionKind, }; pub(crate) const INS_SIZE: usize = mem::size_of::(); @@ -109,7 +110,9 @@ impl Object { let mut maps_by_symbol = HashMap::new(); for (name, fd, map) in maps { maps_by_section.insert(map.section_index(), (name, fd, map)); - maps_by_symbol.insert(map.symbol_index(), (name, fd, map)); + if let Some(index) = map.symbol_index() { + maps_by_symbol.insert(index, (name, fd, map)); + } } let functions = self @@ -193,10 +196,9 @@ fn relocate_maps<'a, I: Iterator>( index: rel.symbol_index, })?; - let section_index = match sym.section_index { - Some(index) => index, + let Some(section_index) = sym.section_index else { // this is not a map relocation - None => continue, + continue; }; // calls and relocation to .text symbols are handled in a separate step @@ -204,23 +206,42 @@ fn relocate_maps<'a, I: Iterator>( continue; } - let (name, fd, map) = if maps_by_symbol.contains_key(&rel.symbol_index) { - maps_by_symbol - .get(&rel.symbol_index) - .ok_or(RelocationError::SectionNotFound { - symbol_index: rel.symbol_index, - symbol_name: sym.name.clone(), - section_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 {:?}", + map.section_index(), + map.section_kind() + ); + debug_assert_eq!(map.symbol_index().unwrap(), rel.symbol_index); + m } else { - maps_by_section - .get(§ion_index) - .ok_or(RelocationError::SectionNotFound { + let Some(m) = maps_by_section.get(§ion_index) else { + debug!( + "failed relocating map by section index {}", + section_index + ); + return Err(RelocationError::SectionNotFound { symbol_index: rel.symbol_index, symbol_name: sym.name.clone(), section_index, - })? + }); + }; + let map = &m.2; + debug!( + "relocating map by section index {}, kind {:?}", + map.section_index(), + map.section_kind() + ); + + debug_assert_eq!(map.symbol_index(), None); + debug_assert!(matches!( + map.section_kind(), + BpfSectionKind::Bss | BpfSectionKind::Data | BpfSectionKind::Rodata + ),); + m }; + debug_assert_eq!(map.section_index(), section_index); let map_fd = fd.ok_or_else(|| RelocationError::MapNotCreated { name: (*name).into(), @@ -476,7 +497,10 @@ fn insn_is_call(ins: &bpf_insn) -> bool { mod test { use alloc::{string::ToString, vec, vec::Vec}; - use crate::maps::{bpf_map_def, BtfMap, BtfMapDef, LegacyMap, Map, MapKind}; + use crate::{ + maps::{bpf_map_def, BtfMap, BtfMapDef, LegacyMap, Map}, + BpfSectionKind, + }; use super::*; @@ -498,25 +522,20 @@ mod test { fn fake_legacy_map(symbol_index: usize) -> Map { Map::Legacy(LegacyMap { - def: bpf_map_def { - ..Default::default() - }, + def: Default::default(), section_index: 0, - symbol_index, + section_kind: BpfSectionKind::Undefined, + symbol_index: Some(symbol_index), data: Vec::new(), - kind: MapKind::Other, }) } fn fake_btf_map(symbol_index: usize) -> Map { Map::Btf(BtfMap { - def: BtfMapDef { - ..Default::default() - }, + def: Default::default(), section_index: 0, symbol_index, data: Vec::new(), - kind: MapKind::Other, }) } diff --git a/aya/src/bpf.rs b/aya/src/bpf.rs index e94b2f6c..81d6051f 100644 --- a/aya/src/bpf.rs +++ b/aya/src/bpf.rs @@ -11,6 +11,7 @@ use aya_obj::{ btf::{BtfFeatures, BtfRelocationError}, generated::BPF_F_XDP_HAS_FRAGS, relocation::BpfRelocationError, + BpfSectionKind, }; use log::debug; use thiserror::Error; @@ -23,7 +24,6 @@ use crate::{ maps::{Map, MapData, MapError}, obj::{ btf::{Btf, BtfError}, - maps::MapKind, Object, ParseError, ProgramSection, }, programs::{ @@ -415,14 +415,14 @@ impl<'a> BpfLoader<'a> { } PinningType::None => map.create(&name)?, }; - if !map.obj.data().is_empty() && map.obj.kind() != MapKind::Bss { + 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)| MapError::SyscallError { call: "bpf_map_update_elem".to_owned(), io_error, })?; } - if map.obj.kind() == MapKind::Rodata { + if map.obj.section_kind() == BpfSectionKind::Rodata { bpf_map_freeze(fd).map_err(|(_, io_error)| MapError::SyscallError { call: "bpf_map_freeze".to_owned(), io_error, diff --git a/aya/src/maps/bloom_filter.rs b/aya/src/maps/bloom_filter.rs index 16eb1aa3..a58b9b44 100644 --- a/aya/src/maps/bloom_filter.rs +++ b/aya/src/maps/bloom_filter.rs @@ -84,10 +84,7 @@ mod tests { bpf_map_type::{BPF_MAP_TYPE_BLOOM_FILTER, BPF_MAP_TYPE_PERF_EVENT_ARRAY}, }, maps::{Map, MapData}, - obj::{ - self, - maps::{LegacyMap, MapKind}, - }, + obj::{self, maps::LegacyMap, BpfSectionKind}, sys::{override_syscall, SysResult, Syscall}, }; use libc::{EFAULT, ENOENT}; @@ -103,9 +100,9 @@ mod tests { ..Default::default() }, section_index: 0, - symbol_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, data: Vec::new(), - kind: MapKind::Other, }) } @@ -142,9 +139,9 @@ mod tests { ..Default::default() }, section_index: 0, - symbol_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, data: Vec::new(), - kind: MapKind::Other, }), fd: None, pinned: false, diff --git a/aya/src/maps/hash_map/hash_map.rs b/aya/src/maps/hash_map/hash_map.rs index a67cdead..14f5e73f 100644 --- a/aya/src/maps/hash_map/hash_map.rs +++ b/aya/src/maps/hash_map/hash_map.rs @@ -117,10 +117,7 @@ mod tests { bpf_map_type::{BPF_MAP_TYPE_HASH, BPF_MAP_TYPE_LRU_HASH}, }, maps::{Map, MapData}, - obj::{ - self, - maps::{LegacyMap, MapKind}, - }, + obj::{self, maps::LegacyMap, BpfSectionKind}, sys::{override_syscall, SysResult, Syscall}, }; @@ -136,9 +133,9 @@ mod tests { ..Default::default() }, section_index: 0, + section_kind: BpfSectionKind::Maps, data: Vec::new(), - kind: MapKind::Other, - symbol_index: 0, + symbol_index: None, }) } @@ -267,9 +264,9 @@ mod tests { ..Default::default() }, section_index: 0, - symbol_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, data: Vec::new(), - kind: MapKind::Other, }), fd: Some(42), pinned: false, diff --git a/aya/src/maps/lpm_trie.rs b/aya/src/maps/lpm_trie.rs index 4a6cc94f..d8f3b66b 100644 --- a/aya/src/maps/lpm_trie.rs +++ b/aya/src/maps/lpm_trie.rs @@ -247,10 +247,7 @@ mod tests { bpf_map_type::{BPF_MAP_TYPE_LPM_TRIE, BPF_MAP_TYPE_PERF_EVENT_ARRAY}, }, maps::{Map, MapData}, - obj::{ - self, - maps::{LegacyMap, MapKind}, - }, + obj::{self, maps::LegacyMap, BpfSectionKind}, sys::{override_syscall, SysResult, Syscall}, }; use libc::{EFAULT, ENOENT}; @@ -266,9 +263,9 @@ mod tests { ..Default::default() }, section_index: 0, - symbol_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, data: Vec::new(), - kind: MapKind::Other, }) } @@ -322,9 +319,9 @@ mod tests { ..Default::default() }, section_index: 0, - symbol_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: None, data: Vec::new(), - kind: MapKind::Other, }), fd: None, btf_fd: None, diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index 505adafd..271cbab8 100644 --- a/aya/src/maps/mod.rs +++ b/aya/src/maps/mod.rs @@ -845,7 +845,7 @@ mod tests { bpf_map_def, generated::{bpf_cmd, bpf_map_type::BPF_MAP_TYPE_HASH}, maps::MapData, - obj::maps::{LegacyMap, MapKind}, + obj::{maps::LegacyMap, BpfSectionKind}, sys::{override_syscall, Syscall}, }; @@ -861,9 +861,9 @@ mod tests { ..Default::default() }, section_index: 0, - symbol_index: 0, + section_kind: BpfSectionKind::Maps, + symbol_index: Some(0), data: Vec::new(), - kind: MapKind::Other, }) }