Browse Source

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
Michal Rostecki 1 year ago
parent
commit
c05a3b69b7

+ 39 - 28
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<Item = (usize, u32)>,
+                candidate: &Candidate,
+                local_variant_name: &str,
+                target_id: u32,
+                mut target_spec: AccessSpec<'a>,
+            ) -> Result<Option<AccessSpec<'a>>, 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>(

+ 128 - 0
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();
+}

+ 8 - 0
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)]