From c05a3b69b7a94036c380bd64c6de51377987077c Mon Sep 17 00:00:00 2001 From: Michal Rostecki Date: Mon, 22 Jan 2024 14:22:53 +0100 Subject: [PATCH] aya-obj: Handle lack of match of enum variants correctly When comparing `local_spec` with `target_spec` for enum relocations, we can encounter a situation when a matchinng variant in a candidate spec doesn't exist. Before this change, such case wasn't handled explicitly, therefore resulted in returning currently constructed `target_spec` at the end. The problem is that such `target_spec` was, due to lack of match, incomplete. It didn't contain any `accessors` nor `parts`. Later usage of such incomplete `target_spec` was leading to panics, since the code operating on enums' `target_spec` expects at least one `accessor` to be available. Fixes #868 --- aya-obj/src/btf/relocation.rs | 67 +++++---- test/integration-test/bpf/reloc.bpf.c | 128 ++++++++++++++++++ .../src/tests/btf_relocations.rs | 8 ++ 3 files changed, 175 insertions(+), 28 deletions(-) diff --git a/aya-obj/src/btf/relocation.rs b/aya-obj/src/btf/relocation.rs index c23bee75..c9412c79 100644 --- a/aya-obj/src/btf/relocation.rs +++ b/aya-obj/src/btf/relocation.rs @@ -450,9 +450,9 @@ fn match_candidate<'target>( candidate.btf, candidate.type_id, )? { - return Ok(Some(target_spec)); + Ok(Some(target_spec)) } else { - return Ok(None); + Ok(None) } } RelocationKind::EnumVariantExists | RelocationKind::EnumVariantValue => { @@ -460,8 +460,15 @@ fn match_candidate<'target>( let target_ty = candidate.btf.type_by_id(target_id)?; // the first accessor is guaranteed to have a name by construction let local_variant_name = local_spec.accessors[0].name.as_ref().unwrap(); - let match_enum = - |name_offset, index, target_spec: &mut AccessSpec| -> Result<_, BtfError> { + + fn match_enum<'a>( + iterator: impl Iterator, + candidate: &Candidate, + local_variant_name: &str, + target_id: u32, + mut target_spec: AccessSpec<'a>, + ) -> Result>, RelocationError> { + for (index, name_offset) in iterator { let target_variant_name = candidate.btf.string_at(name_offset)?; if flavorless_name(local_variant_name) == flavorless_name(&target_variant_name) { @@ -471,29 +478,34 @@ fn match_candidate<'target>( type_id: target_id, name: None, }); - Ok(Some(())) - } else { - Ok(None) - } - }; - match target_ty { - BtfType::Enum(en) => { - for (index, member) in en.variants.iter().enumerate() { - if let Ok(Some(_)) = match_enum(member.name_offset, index, &mut target_spec) - { - return Ok(Some(target_spec)); - } - } - } - BtfType::Enum64(en) => { - for (index, member) in en.variants.iter().enumerate() { - if let Ok(Some(_)) = match_enum(member.name_offset, index, &mut target_spec) - { - return Ok(Some(target_spec)); - } + return Ok(Some(target_spec)); } } - _ => return Ok(None), + Ok(None) + } + + match target_ty { + BtfType::Enum(en) => match_enum( + en.variants + .iter() + .map(|member| member.name_offset) + .enumerate(), + candidate, + local_variant_name, + target_id, + target_spec, + ), + BtfType::Enum64(en) => match_enum( + en.variants + .iter() + .map(|member| member.name_offset) + .enumerate(), + candidate, + local_variant_name, + target_id, + target_spec, + ), + _ => Ok(None), } } RelocationKind::FieldByteOffset @@ -560,10 +572,9 @@ fn match_candidate<'target>( accessor.index * candidate.btf.type_size(target_id)? * 8; } } + Ok(Some(target_spec)) } - }; - - Ok(Some(target_spec)) + } } fn match_member<'target>( diff --git a/test/integration-test/bpf/reloc.bpf.c b/test/integration-test/bpf/reloc.bpf.c index 609e504b..f8104bbb 100644 --- a/test/integration-test/bpf/reloc.bpf.c +++ b/test/integration-test/bpf/reloc.bpf.c @@ -90,6 +90,38 @@ SEC("uprobe") int enum_unsigned_32(void *ctx) { return enum_unsigned_32_global(); } +enum relocated_enum_unsigned_32_checked_variants { +#ifndef TARGET + U32_VAL_A = 0xAAAAAAAA, +#endif + U32_VAL_B = 0xBBBBBBBB, +#ifdef TARGET + U32_VAL_C = 0xCCCCCCCC +#endif +}; + +__noinline int enum_unsigned_32_checked_variants_global() { +#ifndef TARGET + if (bpf_core_enum_value_exists( + enum relocated_enum_unsigned_32_checked_variants, U32_VAL_A)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_unsigned_32_checked_variants, U32_VAL_A)); +#else + if (bpf_core_enum_value_exists( + enum relocated_enum_unsigned_32_checked_variants, U32_VAL_C)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_unsigned_32_checked_variants, U32_VAL_C)); +#endif + } else { + return set_output(bpf_core_enum_value( + enum relocated_enum_unsigned_32_checked_variants, U32_VAL_B)); + } +} + +SEC("uprobe") int enum_unsigned_32_checked_variants(void *ctx) { + return enum_unsigned_32_checked_variants_global(); +} + enum relocated_enum_signed_32 { S32_VAL = #ifndef TARGET @@ -106,6 +138,38 @@ __noinline int enum_signed_32_global() { SEC("uprobe") int enum_signed_32(void *ctx) { return enum_signed_32_global(); } +enum relocated_enum_signed_32_checked_variants { +#ifndef TARGET + S32_VAL_A = -0x7AAAAAAA, +#endif + S32_VAL_B = -0x7BBBBBBB, +#ifdef TARGET + S32_VAL_C = -0x7CCCCCCC +#endif +}; + +__noinline int enum_signed_32_checked_variants_global() { +#ifndef TARGET + if (bpf_core_enum_value_exists(enum relocated_enum_signed_32_checked_variants, + S32_VAL_A)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_signed_32_checked_variants, S32_VAL_A)); +#else + if (bpf_core_enum_value_exists(enum relocated_enum_signed_32_checked_variants, + S32_VAL_C)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_signed_32_checked_variants, S32_VAL_C)); +#endif + } else { + return set_output(bpf_core_enum_value( + enum relocated_enum_signed_32_checked_variants, S32_VAL_B)); + } +} + +SEC("uprobe") int enum_signed_32_checked_variants(void *ctx) { + return enum_signed_32_checked_variants_global(); +} + enum relocated_enum_unsigned_64 { U64_VAL = #ifndef TARGET @@ -124,6 +188,38 @@ SEC("uprobe") int enum_unsigned_64(void *ctx) { return enum_unsigned_64_global(); } +enum relocated_enum_unsigned_64_checked_variants { +#ifndef TARGET + U64_VAL_A = 0xAAAAAAAABBBBBBBB, +#endif + U64_VAL_B = 0xCCCCCCCCDDDDDDDD, +#ifdef TARGET + U64_VAL_C = 0xEEEEEEEEFFFFFFFF +#endif +}; + +__noinline int enum_unsigned_64_checked_variants_global() { +#ifndef TARGET + if (bpf_core_enum_value_exists( + enum relocated_enum_unsigned_64_checked_variants, U64_VAL_A)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_unsigned_64_checked_variants, U64_VAL_A)); +#else + if (bpf_core_enum_value_exists( + enum relocated_enum_unsigned_64_checked_variants, U64_VAL_C)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_unsigned_64_checked_variants, U64_VAL_C)); +#endif + } else { + return set_output(bpf_core_enum_value( + enum relocated_enum_unsigned_64_checked_variants, U64_VAL_B)); + } +} + +SEC("uprobe") int enum_unsigned_64_checked_variants(void *ctx) { + return enum_unsigned_64_checked_variants_global(); +} + enum relocated_enum_signed_64 { S64_VAL = #ifndef TARGET @@ -139,3 +235,35 @@ __noinline int enum_signed_64_global() { } SEC("uprobe") int enum_signed_64(void *ctx) { return enum_signed_64_global(); } + +enum relocated_enum_signed_64_checked_variants { +#ifndef TARGET + S64_VAL_A = -0xAAAAAAABBBBBBB, +#endif + S64_VAL_B = -0xCCCCCCCDDDDDDD, +#ifdef TARGET + S64_VAL_C = -0xEEEEEEEFFFFFFF +#endif +}; + +__noinline int enum_signed_64_checked_variants_global() { +#ifndef TARGET + if (bpf_core_enum_value_exists(enum relocated_enum_signed_64_checked_variants, + S64_VAL_A)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_signed_64_checked_variants, S64_VAL_A)); +#else + if (bpf_core_enum_value_exists(enum relocated_enum_signed_64_checked_variants, + S64_VAL_C)) { + return set_output(bpf_core_enum_value( + enum relocated_enum_signed_64_checked_variants, S64_VAL_C)); +#endif + } else { + return set_output(bpf_core_enum_value( + enum relocated_enum_signed_64_checked_variants, S64_VAL_B)); + } +} + +SEC("uprobe") int enum_signed_64_checked_variants(void *ctx) { + return enum_signed_64_checked_variants_global(); +} diff --git a/test/integration-test/src/tests/btf_relocations.rs b/test/integration-test/src/tests/btf_relocations.rs index c149d43e..2e6c2ee4 100644 --- a/test/integration-test/src/tests/btf_relocations.rs +++ b/test/integration-test/src/tests/btf_relocations.rs @@ -3,12 +3,20 @@ use test_case::test_case; #[test_case("enum_signed_32", false, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0x7AAAAAAAi32 as u64)] #[test_case("enum_signed_32", true, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0x7BBBBBBBi32 as u64)] +#[test_case("enum_signed_32_checked_variants", false, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0x7AAAAAAAi32 as u64)] +#[test_case("enum_signed_32_checked_variants", true, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0x7BBBBBBBi32 as u64)] #[test_case("enum_signed_64", false, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0xAAAAAAABBBBBBBBi64 as u64)] #[test_case("enum_signed_64", true, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0xCCCCCCCDDDDDDDDi64 as u64)] +#[test_case("enum_signed_64_checked_variants", false, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0xAAAAAAABBBBBBBi64 as u64)] +#[test_case("enum_signed_64_checked_variants", true, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), -0xCCCCCCCDDDDDDDi64 as u64)] #[test_case("enum_unsigned_32", false, None, 0xAAAAAAAA)] #[test_case("enum_unsigned_32", true, None, 0xBBBBBBBB)] +#[test_case("enum_unsigned_32_checked_variants", false, None, 0xAAAAAAAA)] +#[test_case("enum_unsigned_32_checked_variants", true, None, 0xBBBBBBBB)] #[test_case("enum_unsigned_64", false, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), 0xAAAAAAAABBBBBBBB)] #[test_case("enum_unsigned_64", true, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), 0xCCCCCCCCDDDDDDDD)] +#[test_case("enum_unsigned_64_checked_variants", false, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), 0xAAAAAAAABBBBBBBB)] +#[test_case("enum_unsigned_64_checked_variants", true, Some((KernelVersion::new(6, 0, 0), "https://github.com/torvalds/linux/commit/6089fb3")), 0xCCCCCCCCDDDDDDDD)] #[test_case("field", false, None, 2)] #[test_case("field", true, None, 1)] #[test_case("pointer", false, None, 42)]