Re: [PATCH v6 09/26] rust: alloc: implement kernel `Box`

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Sep 10, 2024 at 07:49:42PM +0000, Benno Lossin wrote:
> On 10.09.24 19:40, Danilo Krummrich wrote:
> > On Sat, Aug 31, 2024 at 05:39:07AM +0000, Benno Lossin wrote:
> >> On 16.08.24 02:10, Danilo Krummrich wrote:
> >>> +/// # Examples
> >>> +///
> >>> +/// ```
> >>> +/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?;
> >>> +///
> >>> +/// assert_eq!(*b, 24_u64);
> >>> +/// # Ok::<(), Error>(())
> >>> +/// ```
> >>> +///
> >>> +/// ```
> >>> +/// # use kernel::bindings;
> >>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1;
> >>> +/// struct Huge([u8; SIZE]);
> >>> +///
> >>> +/// assert!(KBox::<Huge>::new_uninit(GFP_KERNEL | __GFP_NOWARN).is_err());
> >>> +/// ```
> >>
> >> It would be nice if you could add something like "KBox can't handle big
> >> allocations:" above this example, so that people aren't confused why
> >> this example expects an error.
> > 
> > I don't think that's needed, it's implied by
> > `SIZE == bindings::KMALLOC_MAX_SIZE + 1`.
> > 
> > Surely, we could add it nevertheless, but it's not very precise to just say "big
> > allocations". And I think this isn't the place for lengthy explanations of
> > `Kmalloc` behavior.
> 
> Fair point, nevertheless I find examples a bit more useful, when the
> intention behind them is not only given as code.
> 
> >>> +///
> >>> +/// ```
> >>> +/// # use kernel::bindings;
> >>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1;
> >>> +/// struct Huge([u8; SIZE]);
> >>> +///
> >>> +/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok());
> >>> +/// ```
> >>
> >> Similarly, you could then say above this one "Instead use either `VBox`
> >> or `KVBox`:"
> >>
> >>> +///
> >>> +/// # Invariants
> >>> +///
> >>> +/// The [`Box`]' pointer is always properly aligned and either points to memory allocated with `A`
> >>
> >> Please use `self.0` instead of "[`Box`]'".
> >>
> >>> +/// or, for zero-sized types, is a dangling pointer.
> >>
> >> Probably "dangling, well aligned pointer.".
> > 
> > Does this add any value? For ZSTs everything is "well aligned", isn't it?
> 
> ZSTs can have alignment and then unaligned pointers do exist for them
> (and dereferencing them is UB!):

Where is this documented? The documentation says:

"For operations of size zero, *every* pointer is valid, including the null
pointer. The following points are only concerned with non-zero-sized accesses."
[1]

[1] https://doc.rust-lang.org/std/ptr/index.html

> 
>     #[repr(align(64))]
>     struct Token;
> 
>     fn main() {
>         let t = 64 as *mut Token;
>         let t = unsafe { t.read() }; // this is fine.
>         let t = 4 as *mut Token;
>         let t = unsafe { t.read() }; // this is UB, see below for miri's output
>     }
> 
> Miri complains:
> 
>     error: Undefined Behavior: accessing memory based on pointer with alignment 4, but alignment 64 is required
>      --> src/main.rs:8:22
>       |
>     8 |     let t = unsafe { t.read() }; // this is UB, see below for miri's output
>       |                      ^^^^^^^^ accessing memory based on pointer with alignment 4, but alignment 64 is required
>       |
>       = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
>       = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
>       = note: BACKTRACE:
>       = note: inside `main` at src/main.rs:8:22: 8:30

`read` explicitly asks for non-null and properly aligned even if `T` has size
zero.

> 
> >>> +#[repr(transparent)]
> >>> +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);
> 
> 
> >>> +impl<T, A> Box<T, A>
> >>> +where
> >>> +    T: ?Sized,
> >>> +    A: Allocator,
> >>> +{
> >>> +    /// Creates a new `Box<T, A>` from a raw pointer.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// For non-ZSTs, `raw` must point at an allocation allocated with `A`that is sufficiently
> >>> +    /// aligned for and holds a valid `T`. The caller passes ownership of the allocation to the
> >>> +    /// `Box`.
> >>
> >> You don't say what must happen for ZSTs.
> > 
> > Because we don't require anything for a ZST, do we?
> 
> We require a non-null, well aligned pointer (see above).

We indeed do, gonna add it.

> 
> ---
> Cheers,
> Benno
> 
> >>> +    #[inline]
> >>> +    pub const unsafe fn from_raw(raw: *mut T) -> Self {
> >>> +        // INVARIANT: Validity of `raw` is guaranteed by the safety preconditions of this function.
> >>> +        // SAFETY: By the safety preconditions of this function, `raw` is not a NULL pointer.
> >>> +        Self(unsafe { NonNull::new_unchecked(raw) }, PhantomData::<A>)
> >>> +    }
>