Raise compile error when the alignment of value > 8 bytes

[rhdxmr]

Raise compile error when the alignment of value of BPF map is greater than 8 bytes.

cargo-bpf::llvm is modified to implement this feature. This decision incurs tightening coupling between cargo-bpf and redbpf-macros. I also considered raising runtime error when loading maps in redbpf but I thought the compile error is more helpful to users than runtime errors.

Modifying cargo-bpf::llvm was the last resort. Before trying modifying it, I had tried 2 other methods.

1. use compile_error! macro.

I wanted to use it because it is very intuitive and understandable error message can be provided to users. But I couldn't figure out including compile_error! conditionally by comparing the alignment... I found that including compile_error! conditionally can only be achieved by macro_rules! or #[cfg(...)]. However I don't know how to make use of them to include compile_error! by comparing the alignment.

2. use const assert technique

I tried the code below

modified   redbpf-macros/src/lib.rs
@@ -266,6 +266,19 @@ pub fn map(attrs: TokenStream, item: TokenStream) -> TokenStream {
                 use super::*;
                 use core::mem;

+                macro_rules! check_value_align_le_8_bytes {
+                    ($x:expr $(,)?) => {
+                        const ASSERT: bool = $x;
+                        const LEN: usize = if ASSERT {
+                            0
+                        } else {
+                            1
+                        };
+                        const _: [(); 0] = [(); LEN];
+                    };
+                }
+                check_value_align_le_8_bytes!(mem::align_of::<#vtype>() <= 8);
+
                 #[repr(C)]
                 struct MapBtf {
                     key_type: #ktype,

It worked. It raised compile error as expected but the error message is diffcult to understand.. The only hint users can understand is the macro name... but I think it is insufficient.

error[E0308]: mismatched types
  --> src/devqueue/main.rs:12:1
   |
12 | #[map]
   | ^^^^^^ expected an array with a fixed size of 0 elements, found one with 1 element
   |
   = note: this error originates in the macro `check_value_align_le_8_bytes` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0308`.

---

This is the basis why the item whose alignment exceeds 8 bytes are banned.

1. In official Rust documents, it is stated that misaligned pointers or references lead to undefined behavior.
2. In BPF programs, the pointer returned by bpf_map_lookup_elem is misaligned because the value the pointer points to is merely 8 bytes aligned data.
3. Hence, creating reference of the value, or dereferencing the pointer incurs undefined behavior.

Signed-off-by: Junyeong Jeong rhdxmr@gmail.com

[rhdxmr]

@rsdy Yes, the alignment check is done at compile time for all maps that are decorated by the #[map] attribute.

[rsdy]

This seems like a big deal, would you mind prepping a release? I think it makes sense, as it will help newcomers get meaningful errors instead of fighting with the verifier?

[rhdxmr]

@rsdy Actually I have not seen that the verifier raised errors on the alignment of values of BPF maps. I tried repr(align(16)) on the value type but the BPF verifier never complained about that.

This PR solely depends on the principle of pointers/references alignment in Rust.

[rhdxmr]

@rsdy I found a problem of this modification. Sometimes using value of which alignment is greater than 8bytes is okay.. For example, if BPF program only sets the value into map and never gets the reference of the value then it is no problem. So it is too strict to inhibit using value that exceeds 8 bytes alignment.

It would be better to raise compile error only when the BPF program gets reference of value if the value's alignment is greater than 8 bytes..

[rsdy]

What would happen if you set a value in a Map from a BPF program, and read it from a Rust userland?

I think if there's a possibility for introducing inconsistency and surprising behaviour between the 2 spaces, we should err on the side of caution.

[rhdxmr]

@rsdy It is okay to read big alignment data from the userspace. Because, the output pointer is always aligned properly thanks to MaybeUninit.

    let mut value = MaybeUninit::zeroed();
    if unsafe {
        bpf_sys::bpf_map_lookup_elem(
            fd,
            &mut key as *mut _ as *mut _,
            &mut value as *mut _ as *mut _,
        )
    } < 0
    {
        return None;
    }
    Some(unsafe { value.assume_init() })

But reading from BPF program is considered as undefined behavior because the data stored at kernel space is not aligned properly and get methods unconditionally create references from the misaligned data.

            pub fn get(&mut self, key: &K) -> Option<&V> {
                unsafe {
                    let value = bpf_map_lookup_elem(
                        &mut self.def as *mut _ as *mut c_void,
                        key as *const _ as *const c_void,
                    );
                    if value.is_null() {
                        None
                    } else {
                        Some(&*(value as *const V))
                    }
                }
            }

[rsdy]

@rhdxmr oh, that makes sense. I'm a bit out of my depth here, honestly, so I can't comment on the ideal way to address this, but if you think we should revert this patchset, and follow up with another, happy to roll with that!