uefi: alloc_pages() and alloc_pool() now return NonNull<[u8]>

[phip1611]

uefi: alloc_pages() and alloc_pool() now return NonNull<[u8]>. This aligns the signature with the Rust allocator API and also makes more sense.

Checklist

• Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
• Update the changelog (if necessary)

[phip1611]

Suggested change

	if let Some(ptr) = NonNull::new(ptr) {
	NonNull::new(ptr).map(|ptr| NonNull::slice_from_raw_parts(ptr, size)).ok_or(Status::OUT_OF_RESOURCES.into())

[phip1611]

Just making the new API compatible with existing code; shortcut

[nicholasbishop]

I think this is unsound: as_mut creates a reference, but that's not valid because the allocated memory is uninitialized. I think instead this can be:

            boot::allocate_pool(memory_type, size)
                .map(|ptr: NonNull<[u8]>| ptr.as_ptr().cast::<u8>())
                .unwrap_or(ptr::null_mut())

(Potentially in the future it could be further simplified to ptr.as_mut_ptr(), but it's not yet stable.)

[phip1611]

I think this is unsound: as_mut creates a reference, but that's not valid because the allocated memory is uninitialized.

Discussion continues here: #1606 (comment)

[nicholasbishop]

Same issue here, this is unsound with uninitialized memory. I don't think we have to use write_volatile like the code here did originally, but we do need to use some pointer function like ptr::write or ptr::write_bytes.

Since so many of the NonNull<[T]>/*mut [T] functions are unstable, I think it's most convenient to cast it back to *mut u8. Then you can do unsafe { ptr.write_bytes(...) }.

Ditto for the docstring examples on boot::allocate_pages/boot::allocate_pool.

[phip1611]

I think the concern you raised is not correct: #1606 (comment)

[phip1611]

I think we are safe if we write the following into the documentation of each function:

I think this is valid as Miri verifies my assumptions in this code:

#![feature(allocator_api)]

use std::alloc::{Allocator, GlobalAlloc, Layout, System};

fn main() {
    let layout = Layout::from_size_align(4096, 4096).unwrap();
    let mut buffer_ptr = System.allocate(layout).unwrap();
    let buffer = unsafe { buffer_ptr.as_mut() };
    buffer[1] = 42;
    let _x = buffer[1];
    let _y = buffer[2];

    unsafe {
        System.dealloc(buffer_ptr.as_ptr().cast(), layout);
    }
}

What do you think, are we on the same page?

[nicholasbishop]

Miri is a runtime checker, so it can't fully detect all cases of UB. I'm pretty confident that creating a reference to uninitialized memory is always UB, regardless of whether the reference is read from, unless you use MaybeUninit or a similar construct. The MaybeUninit doc has some examples of instant-UB that don't require a read.

So there's no documentation-level exemption we can give ourselves -- the allocation must be initialized via the pointer API (or &[MaybeUninit<u8>]).