From 54637eab049eef844a86c75a74b83e37761965e7 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sat, 6 Feb 2021 09:32:54 +0000 Subject: [PATCH] Improve relocation errors Use BpfError::RelocationError for both maps and BTF relocations. The error includes the name of the program that failed, and the source error stored as Box. This hides the implementation details of the source errors - which are unrecoverable anyway - while still allowing fine grained error messages. --- src/bpf.rs | 14 +-- src/obj/btf/btf.rs | 4 +- src/obj/btf/relocation.rs | 216 ++++++++++++++++++++++---------------- src/obj/relocation.rs | 122 +++++++++++---------- 4 files changed, 201 insertions(+), 155 deletions(-) diff --git a/src/bpf.rs b/src/bpf.rs index 0573cca1..74444749 100644 --- a/src/bpf.rs +++ b/src/bpf.rs @@ -2,6 +2,7 @@ use std::{ cell::{Ref, RefCell, RefMut}, collections::HashMap, convert::TryFrom, + error::Error, io, }; @@ -9,8 +10,7 @@ use thiserror::Error; use crate::{ maps::{Map, MapError}, - obj::btf::RelocationError as BtfRelocationError, - obj::{btf::BtfError, Object, ParseError, RelocationError}, + obj::{btf::BtfError, Object, ParseError}, programs::{KProbe, Program, ProgramData, ProgramError, SocketFilter, TracePoint, UProbe, Xdp}, syscalls::bpf_map_update_elem_ptr, }; @@ -152,11 +152,11 @@ pub enum BpfError { #[error("BTF error: {0}")] BtfError(#[from] BtfError), - #[error("error relocating BPF object: {0}")] - RelocationError(#[from] RelocationError), - - #[error(transparent)] - BtfRelocationError(#[from] BtfRelocationError), + #[error("error relocating BPF program `{program_name}`: {error}")] + RelocationError { + program_name: String, + error: Box, + }, #[error("map error: {0}")] MapError(#[from] MapError), diff --git a/src/obj/btf/btf.rs b/src/obj/btf/btf.rs index abb3f389..d2477fc1 100644 --- a/src/obj/btf/btf.rs +++ b/src/obj/btf/btf.rs @@ -22,10 +22,10 @@ pub enum BtfError { #[error("error parsing BTF header")] InvalidHeader, - #[error("invalid type info segment")] + #[error("invalid BTF type info segment")] InvalidTypeInfo, - #[error("invalid relocation info segment")] + #[error("invalid BTF relocation info segment")] InvalidRelocationInfo, #[error("invalid BTF type kind `{kind}`")] diff --git a/src/obj/btf/relocation.rs b/src/obj/btf/relocation.rs index 4ab2fb10..5f0f227f 100644 --- a/src/obj/btf/relocation.rs +++ b/src/obj/btf/relocation.rs @@ -27,17 +27,16 @@ pub enum RelocationError { #[error(transparent)] IOError(#[from] io::Error), - #[error("section `{name}` not found")] - SectionNotFound { name: String }, + #[error("program not found")] + ProgramNotFound, #[error("invalid relocation access string {access_str}")] InvalidAccessString { access_str: String }, - #[error("invalid instruction index #{index} referenced by relocation #{relocation_number} in section `{section_name}`")] + #[error("invalid instruction index #{index} referenced by relocation #{relocation_number}, the program contains {num_instructions} instructions")] InvalidInstructionIndex { index: usize, num_instructions: usize, - section_name: String, relocation_number: usize, }, @@ -170,7 +169,6 @@ impl Object { })?; let mut candidates_cache = HashMap::>::new(); - for (sec_name_off, relos) in btf_ext.relocations() { let section_name = local_btf.string_at(*sec_name_off)?; @@ -180,96 +178,120 @@ impl Object { continue; } let section_name = parts[1]; - let program = self.programs.get_mut(section_name).ok_or_else(|| { - RelocationError::SectionNotFound { - name: section_name.to_string(), + let program = self + .programs + .get_mut(section_name) + .ok_or(BpfError::RelocationError { + program_name: section_name.to_owned(), + error: Box::new(RelocationError::ProgramNotFound), + })?; + match relocate_btf_program( + program, + relos, + local_btf, + &target_btf, + &mut candidates_cache, + ) { + Ok(_) => {} + Err(ErrorWrapper::BtfError(e)) => return Err(e)?, + Err(ErrorWrapper::RelocationError(error)) => { + return Err(BpfError::RelocationError { + program_name: section_name.to_owned(), + error: Box::new(error), + }) } - })?; + } + } - for rel in relos { - let instructions = &mut program.instructions; - let ins_index = rel.ins_offset as usize / std::mem::size_of::(); - if ins_index >= instructions.len() { - return Err(RelocationError::InvalidInstructionIndex { - index: ins_index, - num_instructions: instructions.len(), - section_name: section_name.to_string(), - relocation_number: rel.number, - })?; - } + Ok(()) + } +} - let local_ty = local_btf.type_by_id(rel.type_id)?; - let local_name = &*local_btf.type_name(local_ty)?.unwrap(); - let access_str = &*local_btf.string_at(rel.access_str_offset)?; - let local_spec = AccessSpec::new(local_btf, rel.type_id, access_str, *rel)?; - - let mut matches = match rel.kind { - RelocationKind::TypeIdLocal => Vec::new(), // we don't need to look at target types to relocate this value - _ => { - let candidates = match candidates_cache.get(&rel.type_id) { - Some(cands) => cands, - None => { - candidates_cache.insert( - rel.type_id, - find_candidates(local_ty, local_name, &target_btf)?, - ); - candidates_cache.get(&rel.type_id).unwrap() - } - }; +fn relocate_btf_program<'target>( + program: &mut Program, + relos: &[Relocation], + local_btf: &Btf, + target_btf: &'target Btf, + candidates_cache: &mut HashMap>>, +) -> Result<(), ErrorWrapper> { + for rel in relos { + let instructions = &mut program.instructions; + let ins_index = rel.ins_offset as usize / std::mem::size_of::(); + if ins_index >= instructions.len() { + return Err(RelocationError::InvalidInstructionIndex { + index: ins_index, + num_instructions: instructions.len(), + relocation_number: rel.number, + })?; + } - let mut matches = Vec::new(); - for candidate in candidates { - if let Some(candidate_spec) = match_candidate(&local_spec, candidate)? { - let comp_rel = ComputedRelocation::new( - rel, - &local_spec, - Some(&candidate_spec), - )?; - matches.push((candidate.name.clone(), candidate_spec, comp_rel)); - } - } + let local_ty = local_btf.type_by_id(rel.type_id)?; + let local_name = &*local_btf.type_name(local_ty)?.unwrap(); + let access_str = &*local_btf.string_at(rel.access_str_offset)?; + let local_spec = AccessSpec::new(local_btf, rel.type_id, access_str, *rel)?; - matches + let mut matches = match rel.kind { + RelocationKind::TypeIdLocal => Vec::new(), // we don't need to look at target types to relocate this value + _ => { + let candidates = match candidates_cache.get(&rel.type_id) { + Some(cands) => cands, + None => { + candidates_cache.insert( + rel.type_id, + find_candidates(local_ty, local_name, target_btf)?, + ); + candidates_cache.get(&rel.type_id).unwrap() } }; - let comp_rel = if !matches.is_empty() { - let mut matches = matches.drain(..); - let (_, target_spec, target_comp_rel) = matches.next().unwrap(); - - // if there's more than one candidate, make sure that they all resolve to the - // same value, else the relocation is ambiguous and can't be applied - let conflicts = matches - .filter_map(|(cand_name, cand_spec, cand_comp_rel)| { - if cand_spec.bit_offset != target_spec.bit_offset - || cand_comp_rel.target.value != target_comp_rel.target.value - { - Some(cand_name.clone()) - } else { - None - } - }) - .collect::>(); - if !conflicts.is_empty() { - return Err(RelocationError::ConflictingCandidates { - type_name: local_name.to_string(), - candidates: conflicts, - })?; + let mut matches = Vec::new(); + for candidate in candidates { + if let Some(candidate_spec) = match_candidate(&local_spec, candidate)? { + let comp_rel = + ComputedRelocation::new(rel, &local_spec, Some(&candidate_spec))?; + matches.push((candidate.name.clone(), candidate_spec, comp_rel)); } - target_comp_rel - } else { - // there are no candidate matches and therefore no target_spec. This might mean - // that matching failed, or that the relocation can be applied looking at local - // types only - ComputedRelocation::new(rel, &local_spec, None)? - }; + } - comp_rel.apply(program, rel, section_name, local_btf, &target_btf)?; + matches } - } + }; - Ok(()) + let comp_rel = if !matches.is_empty() { + let mut matches = matches.drain(..); + let (_, target_spec, target_comp_rel) = matches.next().unwrap(); + + // if there's more than one candidate, make sure that they all resolve to the + // same value, else the relocation is ambiguous and can't be applied + let conflicts = matches + .filter_map(|(cand_name, cand_spec, cand_comp_rel)| { + if cand_spec.bit_offset != target_spec.bit_offset + || cand_comp_rel.target.value != target_comp_rel.target.value + { + Some(cand_name.clone()) + } else { + None + } + }) + .collect::>(); + if !conflicts.is_empty() { + return Err(RelocationError::ConflictingCandidates { + type_name: local_name.to_string(), + candidates: conflicts, + })?; + } + target_comp_rel + } else { + // there are no candidate matches and therefore no target_spec. This might mean + // that matching failed, or that the relocation can be applied looking at local + // types only + ComputedRelocation::new(rel, &local_spec, None)? + }; + + comp_rel.apply(program, rel, local_btf, &target_btf)?; } + + Ok(()) } fn flavorless_name(name: &str) -> &str { @@ -306,7 +328,7 @@ fn find_candidates<'target>( fn match_candidate<'target>( local_spec: &AccessSpec, candidate: &'target Candidate, -) -> Result>, BpfError> { +) -> Result>, ErrorWrapper> { let mut target_spec = AccessSpec { btf: candidate.btf, root_type_id: candidate.type_id, @@ -434,7 +456,7 @@ fn match_member<'local, 'target>( target_btf: &'target Btf, target_id: u32, target_spec: &mut AccessSpec<'target>, -) -> Result, BpfError> { +) -> Result, ErrorWrapper> { let local_ty = local_btf.type_by_id(local_accessor.type_id)?; let local_member = match local_ty { BtfType::Struct(_, members) | BtfType::Union(_, members) => { @@ -519,7 +541,7 @@ impl<'a> AccessSpec<'a> { root_type_id: u32, spec: &str, relocation: Relocation, - ) -> Result, BpfError> { + ) -> Result, ErrorWrapper> { let parts = spec .split(":") .map(|s| s.parse::()) @@ -724,7 +746,7 @@ impl ComputedRelocation { rel: &Relocation, local_spec: &AccessSpec, target_spec: Option<&AccessSpec>, - ) -> Result { + ) -> Result { use RelocationKind::*; let ret = match rel.kind { FieldByteOffset | FieldByteSize | FieldExists | FieldSigned | FieldLShift64 @@ -749,10 +771,9 @@ impl ComputedRelocation { &self, program: &mut Program, rel: &Relocation, - section_name: &str, local_btf: &Btf, target_btf: &Btf, - ) -> Result<(), BpfError> { + ) -> Result<(), ErrorWrapper> { let instructions = &mut program.instructions; let num_instructions = instructions.len(); let ins_index = rel.ins_offset as usize / std::mem::size_of::(); @@ -762,7 +783,6 @@ impl ComputedRelocation { .ok_or(RelocationError::InvalidInstructionIndex { index: rel.ins_offset as usize, num_instructions, - section_name: section_name.to_string(), relocation_number: rel.number, })?; @@ -840,7 +860,6 @@ impl ComputedRelocation { RelocationError::InvalidInstructionIndex { index: ins_index + 1, num_instructions, - section_name: section_name.to_string(), relocation_number: rel.number, }, )?; @@ -862,7 +881,7 @@ impl ComputedRelocation { fn compute_enum_relocation( rel: &Relocation, spec: Option<&AccessSpec>, - ) -> Result { + ) -> Result { use RelocationKind::*; let value = match rel.kind { EnumVariantExists => spec.is_some() as u32, @@ -888,7 +907,7 @@ impl ComputedRelocation { fn compute_field_relocation( rel: &Relocation, spec: Option<&AccessSpec>, - ) -> Result { + ) -> Result { use RelocationKind::*; if let FieldExists = rel.kind { @@ -1013,7 +1032,7 @@ impl ComputedRelocation { rel: &Relocation, local_spec: &AccessSpec, target_spec: Option<&AccessSpec>, - ) -> Result { + ) -> Result { use RelocationKind::*; let value = match rel.kind { TypeIdLocal => local_spec.root_type_id, @@ -1037,3 +1056,14 @@ impl ComputedRelocation { }) } } + +// this exists only to simplify propagating errors from relocate_btf() and to associate +// RelocationError(s) with their respective program name +#[derive(Error, Debug)] +enum ErrorWrapper { + #[error(transparent)] + BtfError(#[from] BtfError), + + #[error(transparent)] + RelocationError(#[from] RelocationError), +} diff --git a/src/obj/relocation.rs b/src/obj/relocation.rs index fc02a9e0..c881f774 100644 --- a/src/obj/relocation.rs +++ b/src/obj/relocation.rs @@ -1,14 +1,17 @@ -use std::{collections::HashMap, io}; +use std::collections::HashMap; -use object::{RelocationKind, RelocationTarget, SectionIndex}; +use object::{RelocationKind, RelocationTarget, SectionIndex, SymbolIndex}; use thiserror::Error; use crate::{ generated::{bpf_insn, BPF_PSEUDO_MAP_FD, BPF_PSEUDO_MAP_VALUE}, maps::Map, obj::Object, + BpfError, }; +use super::Program; + #[derive(Debug, Error)] pub enum RelocationError { #[error("unknown symbol, index `{index}`")] @@ -52,65 +55,78 @@ pub(crate) struct Symbol { } impl Object { - pub fn relocate_maps(&mut self, maps: &[Map]) -> Result<(), RelocationError> { + pub fn relocate_maps(&mut self, maps: &[Map]) -> Result<(), BpfError> { let maps_by_section = maps .iter() .map(|map| (map.obj.section_index, map)) .collect::>(); - for program in self.programs.values_mut() { + let symbol_table = &self.symbol_table; + for (program_name, program) in self.programs.iter_mut() { if let Some(relocations) = self.relocations.get(&program.section_index) { - for (rel_n, rel) in relocations.iter().enumerate() { - match rel.target { - RelocationTarget::Symbol(index) => { - let sym = self - .symbol_table - .get(&index) - .ok_or(RelocationError::UnknownSymbol { index: index.0 })?; - - let section_index = sym - .section_index - .ok_or(RelocationError::UnknownSymbolSection { index: index.0 })?; - - let map = maps_by_section.get(§ion_index.0).ok_or( - RelocationError::SectionNotFound { - symbol_index: index.0, - symbol_name: sym.name.clone(), - section_index: section_index.0, - }, - )?; - - let map_fd = map.fd.ok_or_else(|| RelocationError::MapNotCreated { - name: map.obj.name.clone(), - section_index: section_index.0, - })?; - - let instructions = &mut program.instructions; - let ins_index = - (rel.offset / std::mem::size_of::() as u64) as usize; - if ins_index >= instructions.len() { - return Err(RelocationError::InvalidInstructionIndex { - index: ins_index, - num_instructions: instructions.len(), - relocation_number: rel_n, - }); - } - if !map.obj.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; - } - RelocationTarget::Section(_index) => {} - RelocationTarget::Absolute => todo!(), - } - } + relocate_program(program, relocations, &maps_by_section, symbol_table).map_err( + |error| BpfError::RelocationError { + program_name: program_name.clone(), + error: Box::new(error), + }, + )?; } } - Ok(()) } } + +fn relocate_program( + program: &mut Program, + relocations: &[Relocation], + maps_by_section: &HashMap, + symbol_table: &HashMap, +) -> Result<(), RelocationError> { + for (rel_n, rel) in relocations.iter().enumerate() { + match rel.target { + RelocationTarget::Symbol(index) => { + let sym = symbol_table + .get(&index) + .ok_or(RelocationError::UnknownSymbol { index: index.0 })?; + + let section_index = sym + .section_index + .ok_or(RelocationError::UnknownSymbolSection { index: index.0 })?; + + let map = maps_by_section.get(§ion_index.0).ok_or( + RelocationError::SectionNotFound { + symbol_index: index.0, + symbol_name: sym.name.clone(), + section_index: section_index.0, + }, + )?; + + let map_fd = map.fd.ok_or_else(|| RelocationError::MapNotCreated { + name: map.obj.name.clone(), + section_index: section_index.0, + })?; + + let instructions = &mut program.instructions; + let ins_index = (rel.offset / std::mem::size_of::() as u64) as usize; + if ins_index >= instructions.len() { + return Err(RelocationError::InvalidInstructionIndex { + index: ins_index, + num_instructions: instructions.len(), + relocation_number: rel_n, + }); + } + if !map.obj.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; + } + RelocationTarget::Section(_index) => {} + RelocationTarget::Absolute => todo!(), + } + } + Ok(()) +}