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

[Benno Lossin]

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!):

    #[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

>>> +#[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).

---
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>)
>>> +    }