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

@@ -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::<u32>() * 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<usize> {
         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<usize>,
     /// The map data
     pub data: Vec<u8>,
-    /// 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<u8>,
 }

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(&section, section.name)?);
+                    .insert(section.name.to_string(), parse_data_map_section(&section)?);
             }
             BpfSectionKind::Text => self.parse_text_section(section)?,
             BpfSectionKind::Btf => self.parse_btf(&section)?,
@@ -909,16 +908,16 @@ fn parse_maps_section<'a, I: Iterator<Item = &'a Symbol>>(
             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<KernelVersion> for u32 {
     }
 }
 
-fn parse_map(section: &Section, name: &str) -> Result<Map, ParseError> {
-    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<Map, ParseError> {
+    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::<u32>() as u32,
@@ -1183,7 +1198,7 @@ fn parse_map(section: &Section, name: &str) -> Result<Map, ParseError> {
                 // .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<Map, ParseError> {
             };
             (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(

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::<bpf_insn>();
@@ -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<Item = &'a Relocation>>(
                 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<Item = &'a Relocation>>(
             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(&section_index)
-                .ok_or(RelocationError::SectionNotFound {
+            let Some(m) = maps_by_section.get(&section_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,
         })
     }
 

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,

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,

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,

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,

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,
         })
     }