Alignment issue in `GlobalAlloc::{alloc, dealloc}`

[fbq]

This was brought up at: #815 (comment)

• kmalloc() only guarantees “aligned to size” for size is a power of 2
• otherwise the alignment is SLAB_MINALIGN (usually is size_of::<usize>() )
• right now, our allocator is just
  • bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL)

And the result of this is that in theory, if a struct has alignment > SLAB_MINALIGN, and its size is not a power of two, we may get a mis-aligned memory object from Rust allocation APIs (Box, Arc, etc.).

For fixes, based on previous and recent discussions:

• For long term, we want slab APIs (mostly krealloc()) to provide a way for users to tell the allocator the desired alignment, which benefits both C and Rust.
• For short term, a quick fix would be do an internal "extend the size to the closest power of 2" trick in side GlobalAlloc::alloc, for example:

unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
    let layout = layout.pad_to_align(); // Handles customized layout.
    let mut size = layout.size();
    
    if layout.align() > SLAB_MINALIGN {
        // `align()` exceeds the kernel allocator alignment guarantee, round up to next 2^n if the size is not a power of two.
        size = size.next_power_of_two(); 
    }
    bindings::krealloc(ptr::null(), size, bindings::GFP_KERNEL)
}

[Darksonn]

Note here that Rust's next_power_of_two method will return the input unchanged when given a power of two, despite what its name suggests.

[fbq]

Note here that Rust's next_power_of_two method will return the input unchanged when given a power of two, despite what its name suggests.

Thanks, I'm actually counting on that behavior to avoid unnecessary enlarging the size. I will fix the comment.

[metaspace]

@fbq I think the solution you posted is not enough. How about this:

    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
        // Alignment is guaranteed to be at least ARCH_KMALLOC_MINALIGN. For
        // sizes that are a power of two, alignment is guaranteed to that size.
        let size = if layout.align() <= bindings::BINDINGS_ARCH_KMALLOC_MINALIGN {
            layout.size()
        } else if layout.size() < layout.align() {
            layout.align()
        } else {
            layout.size().next_power_of_two()
        };

        // `krealloc()` is used instead of `kmalloc()` because the latter is
        // an inline function and cannot be bound to as a result.
        unsafe { bindings::krealloc(ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 }
    }

Adding kmalloc_aligned() on the C side is another option, but not something that I can help with. It was discussed in 2019 when the power of two guarantees were added to kmalloc, but it was dropped because it would be wasteful. Much in the same way that what we are proposing here is wasteful, I would assume.

[fbq]

Hmm.. I did a layout.pad_to_align() to avoid case where size < align, so maybe my approach is the same as yours? But anyway, huge thanks for finding out the previous discussion. I will take some time digging into it.

[metaspace]

Hmm.. I did a layout.pad_to_align() to avoid case where size < align, so maybe my approach is the same as yours? But anyway, huge thanks for finding out the previous discussion. I will take some time digging into it.

Ah, I missed the pad_to_align()! Yes, then it practically the same. I was thinking if alignment is less than MIN_ALIGN and size is less than that. Your code would pad size to align. But it does not matter, unless the allocator is able to take advantage of the potential free bytes in that corner case. Those bytes can never be allocated because they are not aligned to MIN_ALIGN, so it would have to be for internal use 🤷

[metaspace]

@fbq I talked to Vlastimil Babka (SLAB maintainer) and he confirmed that padding to the next power of two is the best we can do. There will be no further alignment options in the kernel allocators than the current guarantee on power of 2. Also, if they did add an API they would implement it the same way we do here.

Do you want to go ahead and submit your solution?