From 37c3a198c4d26db3daf899a6fd9e502b2b1dc5a3 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Sat, 6 Feb 2021 07:14:35 +0000 Subject: [PATCH] Rework the error hierarchy Add BpfError::IO and BpfError::BtfError and remove the nested IO and BtfError variants inside ParseError and RelocationError --- src/bpf.rs | 21 ++++++++---- src/maps/perf_map.rs | 14 +++----- src/obj/btf/relocation.rs | 70 +++++++++++++++++---------------------- src/obj/mod.rs | 38 +++++++-------------- src/obj/relocation.rs | 6 ---- 5 files changed, 62 insertions(+), 87 deletions(-) diff --git a/src/bpf.rs b/src/bpf.rs index 04364540..0573cca1 100644 --- a/src/bpf.rs +++ b/src/bpf.rs @@ -2,13 +2,15 @@ use std::{ cell::{Ref, RefCell, RefMut}, collections::HashMap, convert::TryFrom, + io, }; use thiserror::Error; use crate::{ maps::{Map, MapError}, - obj::{BtfRelocationError, Object, ParseError, RelocationError}, + obj::btf::RelocationError as BtfRelocationError, + obj::{btf::BtfError, Object, ParseError, RelocationError}, programs::{KProbe, Program, ProgramData, ProgramError, SocketFilter, TracePoint, UProbe, Xdp}, syscalls::bpf_map_update_elem_ptr, }; @@ -141,17 +143,24 @@ impl Bpf { #[derive(Debug, Error)] pub enum BpfError { + #[error("IO error: {0}")] + IO(#[from] io::Error), + #[error("error parsing BPF object: {0}")] ParseError(#[from] ParseError), + + #[error("BTF error: {0}")] + BtfError(#[from] BtfError), + #[error("error relocating BPF object: {0}")] RelocationError(#[from] RelocationError), - #[error("BTF error: {error}")] - BtfRelocationError { - #[from] - error: BtfRelocationError, - }, + + #[error(transparent)] + BtfRelocationError(#[from] BtfRelocationError), + #[error("map error: {0}")] MapError(#[from] MapError), + #[error("program error: {0}")] ProgramError(#[from] ProgramError), } diff --git a/src/maps/perf_map.rs b/src/maps/perf_map.rs index 26628bea..48a3f60f 100644 --- a/src/maps/perf_map.rs +++ b/src/maps/perf_map.rs @@ -261,17 +261,11 @@ pub enum PerfMapError { #[error("invalid cpu {cpu_id}")] InvalidCpu { cpu_id: u32 }, - #[error("map error: {map_error}")] - MapError { - #[from] - map_error: MapError, - }, + #[error("map error: {0}")] + MapError(#[from] MapError), - #[error("perf buffer error: {buf_error}")] - PerfBufferError { - #[from] - buf_error: PerfBufferError, - }, + #[error("perf buffer error: {0}")] + PerfBufferError(#[from] PerfBufferError), #[error("bpf_map_update_elem failed: {io_error}")] UpdateElementFailed { diff --git a/src/obj/btf/relocation.rs b/src/obj/btf/relocation.rs index 12e355b6..4ab2fb10 100644 --- a/src/obj/btf/relocation.rs +++ b/src/obj/btf/relocation.rs @@ -19,26 +19,18 @@ use crate::{ }, Btf, BtfError, Object, Program, }, + BpfError, }; #[derive(Error, Debug)] pub enum RelocationError { - #[error("{error}")] - BtfError { - #[from] - error: BtfError, - }, - - #[error("{error}")] - IOError { - #[from] - error: io::Error, - }, + #[error(transparent)] + IOError(#[from] io::Error), #[error("section `{name}` not found")] SectionNotFound { name: String }, - #[error("invalid BTF relocation access string {access_str}")] + #[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}`")] @@ -161,7 +153,7 @@ impl Relocation { } impl Object { - pub fn relocate_btf(&mut self) -> Result<(), RelocationError> { + pub fn relocate_btf(&mut self) -> Result<(), BpfError> { let (local_btf, btf_ext) = match (&self.btf, &self.btf_ext) { (Some(btf), Some(btf_ext)) => (btf, btf_ext), _ => return Ok(()), @@ -203,7 +195,7 @@ impl Object { num_instructions: instructions.len(), section_name: section_name.to_string(), relocation_number: rel.number, - }); + })?; } let local_ty = local_btf.type_by_id(rel.type_id)?; @@ -262,7 +254,7 @@ impl Object { return Err(RelocationError::ConflictingCandidates { type_name: local_name.to_string(), candidates: conflicts, - }); + })?; } target_comp_rel } else { @@ -314,7 +306,7 @@ fn find_candidates<'target>( fn match_candidate<'target>( local_spec: &AccessSpec, candidate: &'target Candidate, -) -> Result>, RelocationError> { +) -> Result>, BpfError> { let mut target_spec = AccessSpec { btf: candidate.btf, root_type_id: candidate.type_id, @@ -416,7 +408,7 @@ fn match_candidate<'target>( if target_spec.parts.len() == MAX_SPEC_LEN { return Err(RelocationError::MaximumNestingLevelReached { type_name: Some(candidate.name.clone()), - }); + })?; } target_spec.parts.push(accessor.index); @@ -442,7 +434,7 @@ fn match_member<'local, 'target>( target_btf: &'target Btf, target_id: u32, target_spec: &mut AccessSpec<'target>, -) -> Result, RelocationError> { +) -> Result, BpfError> { let local_ty = local_btf.type_by_id(local_accessor.type_id)?; let local_member = match local_ty { BtfType::Struct(_, members) | BtfType::Union(_, members) => { @@ -467,7 +459,7 @@ fn match_member<'local, 'target>( let root_ty = target_spec.btf.type_by_id(target_spec.root_type_id)?; return Err(RelocationError::MaximumNestingLevelReached { type_name: target_spec.btf.err_type_name(root_ty), - }); + })?; } let bit_offset = member_bit_offset(target_ty.info().unwrap(), target_member); @@ -527,7 +519,7 @@ impl<'a> AccessSpec<'a> { root_type_id: u32, spec: &str, relocation: Relocation, - ) -> Result, RelocationError> { + ) -> Result, BpfError> { let parts = spec .split(":") .map(|s| s.parse::()) @@ -547,7 +539,7 @@ impl<'a> AccessSpec<'a> { if parts != [0] { return Err(RelocationError::InvalidAccessString { access_str: spec.to_string(), - }); + })?; } AccessSpec { btf, @@ -563,7 +555,7 @@ impl<'a> AccessSpec<'a> { if parts.len() != 1 { return Err(RelocationError::InvalidAccessString { access_str: spec.to_string(), - }); + })?; } let index = parts[0]; if index >= members.len() { @@ -573,7 +565,7 @@ impl<'a> AccessSpec<'a> { index: index, max_index: members.len(), error: "tried to access nonexistant enum variant".to_string(), - }); + })?; } let accessors = vec![Accessor { type_id, @@ -596,7 +588,7 @@ impl<'a> AccessSpec<'a> { relocation_kind: format!("{:?}", relocation.kind), type_kind: format!("{:?}", ty.kind()?.unwrap()), error: "enum relocation on non-enum type".to_string(), - }) + })? } }, @@ -626,7 +618,7 @@ impl<'a> AccessSpec<'a> { index: index, max_index: members.len(), error: "out of bounds struct or union access".to_string(), - }); + })?; } let member = members[index]; @@ -662,7 +654,7 @@ impl<'a> AccessSpec<'a> { index, max_index: array.nelems as usize, error: "array index out of bounds".to_string(), - }); + })?; } accessors.push(Accessor { type_id, @@ -679,7 +671,7 @@ impl<'a> AccessSpec<'a> { type_kind: format!("{:?}", ty.kind()), error: "field relocation on a type that doesn't have fields" .to_string(), - }); + })?; } }; } @@ -732,7 +724,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 @@ -760,7 +752,7 @@ impl ComputedRelocation { section_name: &str, local_btf: &Btf, target_btf: &Btf, - ) -> Result<(), RelocationError> { + ) -> Result<(), BpfError> { let instructions = &mut program.instructions; let num_instructions = instructions.len(); let ins_index = rel.ins_offset as usize / std::mem::size_of::(); @@ -786,7 +778,7 @@ impl ComputedRelocation { relocation_number: rel.number, index: ins_index, error: format!("invalid src_reg={:x} expected {:x}", src_reg, BPF_K), - }); + })?; } ins.imm = target_value as i32; @@ -797,7 +789,7 @@ impl ComputedRelocation { relocation_number: rel.number, index: ins_index, error: format!("value `{}` overflows 16 bits offset field", target_value), - }); + })?; } ins.off = target_value as i16; @@ -822,7 +814,7 @@ impl ComputedRelocation { err_type_name(&target_btf.err_type_name(target_ty)), self.target.size, ), - }) + })? } } @@ -836,7 +828,7 @@ impl ComputedRelocation { relocation_number: rel.number, index: ins_index, error: format!("invalid target size {}", size), - }) + })? } } as u8; ins.code = ins.code & 0xE0 | size | ins.code & 0x07; @@ -860,7 +852,7 @@ impl ComputedRelocation { relocation_number: rel.number, index: ins_index, error: format!("invalid instruction class {:x}", class), - }) + })? } }; @@ -870,7 +862,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, @@ -896,7 +888,7 @@ impl ComputedRelocation { fn compute_field_relocation( rel: &Relocation, spec: Option<&AccessSpec>, - ) -> Result { + ) -> Result { use RelocationKind::*; if let FieldExists = rel.kind { @@ -931,7 +923,7 @@ impl ComputedRelocation { relocation_kind: format!("{:?}", rel_kind), type_kind: format!("{:?}", ty.kind()), error: "invalid relocation kind for array type".to_string(), - }); + })?; } }; } @@ -947,7 +939,7 @@ impl ComputedRelocation { relocation_kind: format!("{:?}", rel.kind), type_kind: format!("{:?}", ty.kind()), error: "field relocation on a type that doesn't have fields".to_string(), - }); + })?; } }; @@ -1021,7 +1013,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, diff --git a/src/obj/mod.rs b/src/obj/mod.rs index 6e2a6e82..34035e9c 100644 --- a/src/obj/mod.rs +++ b/src/obj/mod.rs @@ -1,4 +1,4 @@ -mod btf; +pub(crate) mod btf; mod relocation; use object::{ @@ -14,13 +14,13 @@ use std::{ }; use thiserror::Error; -pub use btf::RelocationError as BtfRelocationError; pub use relocation::*; use crate::{ bpf_map_def, generated::{bpf_insn, bpf_map_type::BPF_MAP_TYPE_ARRAY}, obj::btf::{Btf, BtfError, BtfExt}, + BpfError, }; const KERNEL_VERSION_ANY: u32 = 0xFFFF_FFFE; @@ -85,8 +85,8 @@ impl FromStr for ProgramKind { } impl Object { - pub(crate) fn parse(data: &[u8]) -> Result { - let obj = object::read::File::parse(data).map_err(|source| ParseError::Error { source })?; + pub(crate) fn parse(data: &[u8]) -> Result { + let obj = object::read::File::parse(data).map_err(|e| ParseError::ElfError(e))?; let endianness = obj.endianness(); let section = obj @@ -138,9 +138,7 @@ impl Object { fn parse_program(&self, section: &Section, ty: &str) -> Result { let num_instructions = section.data.len() / mem::size_of::(); if section.data.len() % mem::size_of::() > 0 { - return Err(ParseError::InvalidProgramCode { - name: section.name.to_owned(), - }); + return Err(ParseError::InvalidProgramCode); } let instructions = (0..num_instructions) .map(|i| unsafe { @@ -171,7 +169,7 @@ impl Object { Ok(()) } - fn parse_section(&mut self, section: Section) -> Result<(), ParseError> { + fn parse_section(&mut self, section: Section) -> Result<(), BpfError> { let parts = section.name.split("/").collect::>(); match parts.as_slice() { @@ -209,13 +207,7 @@ impl Object { #[derive(Debug, Clone, Error)] pub enum ParseError { #[error("error parsing ELF data")] - Error { - #[source] - source: object::read::Error, - }, - - #[error("Error parsing BTF: {0}")] - BTF(#[from] BtfError), + ElfError(#[from] object::read::Error), #[error("no license specified")] MissingLicense, @@ -245,8 +237,8 @@ pub enum ParseError { #[error("invalid program kind `{kind}`")] InvalidProgramKind { kind: String }, - #[error("error parsing program `{name}`")] - InvalidProgramCode { name: String }, + #[error("invalid program code")] + InvalidProgramCode, #[error("error parsing map `{name}`")] InvalidMapDefinition { name: String }, @@ -410,7 +402,7 @@ mod tests { fn test_parse_generic_error() { assert!(matches!( Object::parse(&b"foo"[..]), - Err(ParseError::Error { .. }) + Err(BpfError::ParseError(ParseError::ElfError(_))) )) } @@ -570,14 +562,8 @@ mod tests { let obj = fake_obj(); assert_matches!( - obj.parse_program( - &fake_section( - "kprobe/foo", - &42u32.to_ne_bytes(), - ), - "kprobe" - ), - Err(ParseError::InvalidProgramCode { name }) if name == "kprobe/foo" + obj.parse_program(&fake_section("kprobe/foo", &42u32.to_ne_bytes(),), "kprobe"), + Err(ParseError::InvalidProgramCode) ); } diff --git a/src/obj/relocation.rs b/src/obj/relocation.rs index 2b670c87..fc02a9e0 100644 --- a/src/obj/relocation.rs +++ b/src/obj/relocation.rs @@ -34,12 +34,6 @@ pub enum RelocationError { num_instructions: usize, relocation_number: usize, }, - - #[error("IO error: {io_error}")] - IO { - #[from] - io_error: io::Error, - }, } #[derive(Debug, Copy, Clone)]