Merge pull request #501 from alessandrod/fix-enum32-relocs

aya: btf: fix relocations for signed enums (32 bits)

aya-obj/src/btf/btf.rs

@@ -529,7 +529,7 @@ impl Btf {
                         .iter()
                         .map(|p| BtfEnum {
                             name_offset: p.name_offset,
-                            value: p.btf_type as i32,
+                            value: p.btf_type,
                         })
                         .collect();
                     let enum_type = BtfType::Enum(Enum::new(ty.name_offset, members));
@@ -1246,9 +1246,9 @@ mod tests {
             assert!(fixed.name_offset == 0);
             assert!(fixed.variants.len() == 2);
             assert!(btf.string_at(fixed.variants[0].name_offset).unwrap() == "a");
-            assert!(fixed.variants[0].value == int_type_id as i32);
+            assert!(fixed.variants[0].value == int_type_id);
             assert!(btf.string_at(fixed.variants[1].name_offset).unwrap() == "b");
-            assert!(fixed.variants[1].value == int_type_id as i32);
+            assert!(fixed.variants[1].value == int_type_id);
         } else {
             panic!("not an emum")
         }

aya-obj/src/btf/relocation.rs

@@ -786,7 +786,7 @@ struct ComputedRelocation {
 
 #[derive(Debug)]
 struct ComputedRelocationValue {
-    value: u32,
+    value: u64,
     size: u32,
     type_id: Option<u32>,
 }
@@ -854,7 +854,7 @@ impl ComputedRelocation {
                 ins.imm = target_value as i32;
             }
             BPF_LDX | BPF_ST | BPF_STX => {
-                if target_value > i16::MAX as u32 {
+                if target_value > i16::MAX as u64 {
                     return Err(RelocationError::InvalidInstruction {
                         relocation_number: rel.number,
                         index: ins_index,
@@ -914,7 +914,7 @@ impl ComputedRelocation {
                     },
                 )?;
 
-                next_ins.imm = 0;
+                next_ins.imm = (target_value >> 32) as i32;
             }
             class => {
                 return Err(RelocationError::InvalidInstruction {
@@ -934,11 +934,18 @@ impl ComputedRelocation {
     ) -> Result<ComputedRelocationValue, RelocationError> {
         use RelocationKind::*;
         let value = match (rel.kind, spec) {
-            (EnumVariantExists, spec) => spec.is_some() as u32,
+            (EnumVariantExists, spec) => spec.is_some() as u64,
             (EnumVariantValue, Some(spec)) => {
                 let accessor = &spec.accessors[0];
                 match spec.btf.type_by_id(accessor.type_id)? {
-                    BtfType::Enum(en) => en.variants[accessor.index].value as u32,
+                    BtfType::Enum(en) => {
+                        let value = en.variants[accessor.index].value;
+                        if en.is_signed() {
+                            value as i32 as u64
+                        } else {
+                            value as u64
+                        }
+                    }
                     // candidate selection ensures that rel_kind == local_kind == target_kind
                     _ => unreachable!(),
                 }
@@ -969,7 +976,7 @@ impl ComputedRelocation {
             // this is the bpf_preserve_field_info(member_access, FIELD_EXISTENCE) case. If we
             // managed to build a spec, it means the field exists.
             return Ok(ComputedRelocationValue {
-                value: spec.is_some() as u32,
+                value: spec.is_some() as u64,
                 size: 0,
                 type_id: None,
             });
@@ -991,12 +998,12 @@ impl ComputedRelocation {
             // the last accessor is unnamed, meaning that this is an array access
             return match rel.kind {
                 FieldByteOffset => Ok(ComputedRelocationValue {
-                    value: (spec.bit_offset / 8) as u32,
+                    value: (spec.bit_offset / 8) as u64,
                     size: spec.btf.type_size(accessor.type_id)? as u32,
                     type_id: Some(accessor.type_id),
                 }),
                 FieldByteSize => Ok(ComputedRelocationValue {
-                    value: spec.btf.type_size(accessor.type_id)? as u32,
+                    value: spec.btf.type_size(accessor.type_id)? as u64,
                     size: 0,
                     type_id: Some(accessor.type_id),
                 }),
@@ -1061,30 +1068,30 @@ impl ComputedRelocation {
         #[allow(clippy::wildcard_in_or_patterns)]
         match rel.kind {
             FieldByteOffset => {
-                value.value = byte_off;
+                value.value = byte_off as u64;
                 if !is_bitfield {
                     value.size = byte_size;
                     value.type_id = Some(member_type_id);
                 }
             }
             FieldByteSize => {
-                value.value = byte_size;
+                value.value = byte_size as u64;
             }
             FieldSigned => match member_ty {
-                BtfType::Enum(_) => value.value = 1,
-                BtfType::Int(i) => value.value = i.encoding() as u32 & IntEncoding::Signed as u32,
+                BtfType::Enum(en) => value.value = en.is_signed() as u64,
+                BtfType::Int(i) => value.value = i.encoding() as u64 & IntEncoding::Signed as u64,
                 _ => (),
             },
             #[cfg(target_endian = "little")]
             FieldLShift64 => {
-                value.value = 64 - (bit_off + bit_size - byte_off * 8);
+                value.value = 64 - (bit_off + bit_size - byte_off * 8) as u64;
             }
             #[cfg(target_endian = "big")]
             FieldLShift64 => {
                 value.value = (8 - byte_size) * 8 + (bit_off - byte_off * 8);
             }
             FieldRShift64 => {
-                value.value = 64 - bit_size;
+                value.value = 64 - bit_size as u64;
             }
             FieldExists // this is handled at the start of the function
             | _ => panic!("bug! this should not be reached"),
@@ -1101,11 +1108,11 @@ impl ComputedRelocation {
         use RelocationKind::*;
 
         let value = match (rel.kind, target_spec) {
-            (TypeIdLocal, _) => local_spec.root_type_id,
-            (TypeIdTarget, Some(target_spec)) => target_spec.root_type_id,
-            (TypeExists, target_spec) => target_spec.is_some() as u32,
+            (TypeIdLocal, _) => local_spec.root_type_id as u64,
+            (TypeIdTarget, Some(target_spec)) => target_spec.root_type_id as u64,
+            (TypeExists, target_spec) => target_spec.is_some() as u64,
             (TypeSize, Some(target_spec)) => {
-                target_spec.btf.type_size(target_spec.root_type_id)? as u32
+                target_spec.btf.type_size(target_spec.root_type_id)? as u64
             }
             _ => {
                 return Err(RelocationError::MissingTargetDefinition {

aya-obj/src/btf/types.rs

@@ -389,7 +389,7 @@ impl Int {
 #[derive(Debug, Clone)]
 pub(crate) struct BtfEnum {
     pub(crate) name_offset: u32,
-    pub(crate) value: i32,
+    pub(crate) value: u32,
 }
 
 #[repr(C)]
@@ -409,7 +409,7 @@ impl Enum {
         buf.extend(bytes_of::<u32>(&self.size));
         for v in &self.variants {
             buf.extend(bytes_of::<u32>(&v.name_offset));
-            buf.extend(bytes_of::<i32>(&v.value));
+            buf.extend(bytes_of::<u32>(&v.value));
         }
         buf
     }
@@ -432,6 +432,10 @@ impl Enum {
             variants,
         }
     }
+
+    pub(crate) fn is_signed(&self) -> bool {
+        self.info >> 31 == 1
+    }
 }
 
 #[repr(C)]

test/integration-test/src/tests/relocations.rs

@@ -6,6 +6,9 @@ use aya::{maps::Array, programs::TracePoint, BpfLoader, Btf, Endianness};
 
 use super::{integration_test, IntegrationTest};
 
+// In the tests below we often use values like 0xAAAAAAAA or -0x7AAAAAAA. Those values have no
+// special meaning, they just have "nice" bit patterns that can be helpful while debugging.
+
 #[integration_test]
 fn relocate_field() {
     let test = RelocationTest {
@@ -28,9 +31,7 @@ fn relocate_field() {
         relocation_code: r#"
             __u8 memory[] = {1, 2, 3, 4};
             struct foo *ptr = (struct foo *) &memory;
-            bpf_probe_read_kernel(&value,
-                                  sizeof(__u8),
-                                  __builtin_preserve_access_index(&ptr->c));
+            value = __builtin_preserve_access_index(ptr->c);
         "#,
     }
     .build()
@@ -43,10 +44,10 @@ fn relocate_field() {
 fn relocate_enum() {
     let test = RelocationTest {
         local_definition: r#"
-            enum foo { D = 1 };
+            enum foo { D = 0xAAAAAAAA };
         "#,
         target_btf: r#"
-            enum foo { D = 4 } e1;
+            enum foo { D = 0xBBBBBBBB } e1;
         "#,
         relocation_code: r#"
             #define BPF_ENUMVAL_VALUE 1
@@ -55,8 +56,28 @@ fn relocate_enum() {
     }
     .build()
     .unwrap();
-    assert_eq!(test.run().unwrap(), 4);
-    assert_eq!(test.run_no_btf().unwrap(), 1);
+    assert_eq!(test.run().unwrap(), 0xBBBBBBBB);
+    assert_eq!(test.run_no_btf().unwrap(), 0xAAAAAAAA);
+}
+
+#[integration_test]
+fn relocate_enum_signed() {
+    let test = RelocationTest {
+        local_definition: r#"
+            enum foo { D = -0x7AAAAAAA };
+        "#,
+        target_btf: r#"
+            enum foo { D = -0x7BBBBBBB } e1;
+        "#,
+        relocation_code: r#"
+            #define BPF_ENUMVAL_VALUE 1
+            value = __builtin_preserve_enum_value(*(typeof(enum foo) *)D, BPF_ENUMVAL_VALUE);
+        "#,
+    }
+    .build()
+    .unwrap();
+    assert_eq!(test.run().unwrap() as i64, -0x7BBBBBBBi64);
+    assert_eq!(test.run_no_btf().unwrap() as i64, -0x7AAAAAAAi64);
 }
 
 #[integration_test]
@@ -73,9 +94,7 @@ fn relocate_pointer() {
         relocation_code: r#"
             __u8 memory[] = {42, 0, 0, 0, 0, 0, 0, 0};
             struct bar* ptr = (struct bar *) &memory;
-            bpf_probe_read_kernel(&value,
-                                  sizeof(void *),
-                                  __builtin_preserve_access_index(&ptr->f));
+            value = (__u64) __builtin_preserve_access_index(ptr->f);
         "#,
     }
     .build()
@@ -121,14 +140,13 @@ impl RelocationTest {
                 #include <linux/bpf.h>
 
                 static long (*bpf_map_update_elem)(void *map, const void *key, const void *value, __u64 flags) = (void *) 2;
-                static long (*bpf_probe_read_kernel)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) 113;
 
                 {local_definition}
 
                 struct {{
                   int (*type)[BPF_MAP_TYPE_ARRAY];
                   __u32 *key;
-                  __u32 *value;
+                  __u64 *value;
                   int (*max_entries)[1];
                 }} output_map
                 __attribute__((section(".maps"), used));
@@ -136,7 +154,7 @@ impl RelocationTest {
                 __attribute__((section("tracepoint/bpf_prog"), used))
                 int bpf_prog(void *ctx) {{
                   __u32 key = 0;
-                  __u32 value = 0;
+                  __u64 value = 0;
                   {relocation_code}
                   bpf_map_update_elem(&output_map, &key, &value, BPF_ANY);
                   return 0;
@@ -165,11 +183,9 @@ impl RelocationTest {
             r#"
                 #include <linux/bpf.h>
 
-                static long (*bpf_probe_read_kernel)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) 113;
-
                 {target_btf}
                 int main() {{
-                    __u32 value = 0;
+                    __u64 value = 0;
                     // This is needed to make sure to emit BTF for the defined types,
                     // it could be dead code eliminated if we don't.
                     {relocation_code};
@@ -218,17 +234,17 @@ struct RelocationTestRunner {
 
 impl RelocationTestRunner {
     /// Run test and return the output value
-    fn run(&self) -> Result<u32> {
+    fn run(&self) -> Result<u64> {
         self.run_internal(true).context("Error running with BTF")
     }
 
     /// Run without loading btf
-    fn run_no_btf(&self) -> Result<u32> {
+    fn run_no_btf(&self) -> Result<u64> {
         self.run_internal(false)
             .context("Error running without BTF")
     }
 
-    fn run_internal(&self, with_relocations: bool) -> Result<u32> {
+    fn run_internal(&self, with_relocations: bool) -> Result<u64> {
         let mut loader = BpfLoader::new();
         if with_relocations {
             loader.btf(Some(&self.btf));
@@ -250,7 +266,7 @@ impl RelocationTestRunner {
         // To inspect the loaded eBPF bytecode, increse the timeout and run:
         // $ sudo bpftool prog dump xlated name bpf_prog
 
-        let output_map: Array<_, u32> = bpf.take_map("output_map").unwrap().try_into().unwrap();
+        let output_map: Array<_, u64> = bpf.take_map("output_map").unwrap().try_into().unwrap();
         let key = 0;
         output_map.get(&key, 0).context("Getting key 0 failed")
     }