Use aligned_read for getter_be!

[aureliar8]

Try to fix #69 by using aligned reads (via core::ptr:read) rather than unaligned reads (via core::ptr::unaligned_read)

In addition to that I added some verification in test that getter_be! is only called to do aligned reads

Bellow is my understanding of the problem:

The issue is that the approach doesn't work for the moment:
I assumed the base pointer to all header types would be "properly aligned", and thus that as long as the field offset was a multiple of the field alignment, we would always do aligned reads.

However, with the current code, all header types have an alignment of 1 (easily checked by rust-analyser) . This is because the header types are only made of u8 and [u8; N] which have an alignment of 1.

In a regular rust context, we could use the align 1 macro to raise the alignment of those type and ensure that the rust compiler aligns the base address of those types.

But in an ebpf context, this can't work because the memory layout is controlled by the kernel.
While ctx->data should be at least 4-aligned (and maybe 8-aligned depending on the underlying arch), ensuring alignment for all the header seems difficult. It's probably true that all headers start at an address that is 2-aligned, because all sane protocols have header of a size that is a multiple of two, but we can't assume that all headers will be 4-aligned (For example the ethernet header is of size 14, which isn't a multiple of 4, so the IP header will not be 4-aligned and so the ip_src and ip_dest fields will not be either)

If we want to still benefit a bit from fast aligned reads, I think the only thing we can do is to only do aligned read for u16 fields.
The safety property will be enforced differently depending on the context

• In classic rust, we add #[align(2)] to all header structs to ensure the compiler aligns their base address
• In ebpf, we hope that the full list of header is made of headers that are 2-aligned.
  (Note that in ebpf, the verifier checks that all pointer access are aligned, so we shouldn't have any undefined behavior. At worst a program will be rejected if it does unaligned loads 2)

WDYT ?

---

This change is 

[aureliar8]

I found the following documentation about the struct skb format in the kernel:

When setting up receive packets that an ethernet device will DMA into, we typically call skb_reserve(skb, NET_IP_ALIGN). By default NET_IP_ALIGN is defined to '2'. This makes it so that, after the ethernet header, the protocol header will be aligned on at least a 4-byte boundary. Nearly all of the IPV4 and IPV6 protocol processing assumes that the headers are properly aligned.

This would indicate that, at least for tc programs, we can always assume that all headers are properly aligned.

http://oldvger.kernel.org/~davem/skb_data.html

[vadorovsky]

I will take a deeper look at the test failures today why we end up doing unaligned reads for the ICMP header.

[aureliar8]

I will take a deeper look at the test failures today why we end up doing unaligned reads for the ICMP header.

I think it's because the IcmpHdr struct has an alignment of 1, which means that the rust compiler is free to not align its base address, which then causes the fields to not be aligned either.

Adding an align(x) macro to the struct definition will likely fix the issue, as it'll force the rust compiler to align the base address. However we can't rely on that for ebpf code where the kernel controls the memory allocation and placement

[vadorovsky]

@aureliar8 I think we need to force alignment in Ipv4Hdr as well 🙂

[aureliar8]

Sorry, I didn't have a lot of time to allocate to this PR and didn't have a good idea on how to tackle the problem

I think the correct fix it to simply replace core::ptr::read_unaligned with core::ptr::read:

In an ebpf context the kernel will ensure that the L3 header is 4-bytes aligned (and thus that all headers after are too)
The verifier also rejects unaligned operations so if the epbf program passes verification its safe.

In a pure rust setting, I added a macro to force the compiler to use 4-bytes alignment to all header types of layer 3 and higher.

I think the following PR works, I'm just a bit annoyed because there's not a good test setup to remember to add the align(4) to new header that will be supported in the future. But I don't know how to add this test harness :(

EDIT: Actually rust already test for unaligned access (at least in test mode), so if test passes we know that we are safe in the pure rust context

thread 'mpls::tests::test_mpls_set_s' panicked at src/mpls.rs:97:9:
misaligned pointer dereference: address must be a multiple of 0x4 but is 0x7ef2acdfd452
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.

[aureliar8]

I'm not sure anymore that this is a good idea :(

Even if we can rely on the kernel to 4-align the first IP header, which automatically ensure that the L4 header is aligned, we can't be certain that all headers are 4-aligned.

The problematic scenario are encapsulated packets. For example Geneve encapsulated packets (but it can happen for any encapsulation protocol that encapsulate an Ethernet frame)

The outer ip header is 4-aligned by the kernel, thus the outer UDP header is 4-aligned and the Geneve header is 4-aligned.
After the Geneve header comes the Ethernet header that is also 4-aligned. But as it is 14 bytes, the end of the inner Ethernet header and thus the start of the inner IP header is only 2-byte aligned.

I need to think more on how to tackle this kind of issue