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

Benno Lossin
2024-09-26T13:23:53Z

On 12.09.24 00:52, Danilo Krummrich wrote:
> `Box` provides the simplest way to allocate memory for a generic type
> with one of the kernel's allocators, e.g. `Kmalloc`, `Vmalloc` or
> `KVmalloc`.
>
> In contrast to Rust's `Box` type, the kernel `Box` type considers the
> kernel's GFP flags for all appropriate functions, always reports
> allocation failures through `Result<_, AllocError>` and remains
> independent from unstable features.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> rust/kernel/alloc.rs | 6 +
> rust/kernel/alloc/kbox.rs | 465 ++++++++++++++++++++++++++++++++++++++
> rust/kernel/prelude.rs | 2 +-
> 3 files changed, 472 insertions(+), 1 deletion(-)
> create mode 100644 rust/kernel/alloc/kbox.rs

I have two nits below. I think the safety documentation can be improved
later, when we have decided on a better approach (I have to to spend a
significant amount of time coming up with a suggestion and even then I
am not satisfied with it). So

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

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

Missing space after "with `A`".

> + /// aligned for and holds a valid `T`. The caller passes ownership of the allocation to the
> + /// `Box`.
> + ///
> + /// For ZSTs, `raw` must be a dangling, well aligned pointer.
> + #[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::)
> + }
> +
> + /// Consumes the `Box<T, A>` and returns a raw pointer.
> + ///
> + /// This will not run the destructor of `T` and for non-ZSTs the allocation will stay alive
> + /// indefinitely. Use [`Box::from_raw`] to recover the [`Box`], drop the value and free the
> + /// allocation, if any.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// let x = KBox::new(24, GFP_KERNEL)?;
> + /// let ptr = KBox::into_raw(x);
> + /// let x = unsafe { KBox::from_raw(ptr) };
> + ///
> + /// assert_eq!(*x, 24);
> + /// # Ok::<(), Error>(())
> + /// ```
> + #[inline]
> + pub fn into_raw(b: Self) -> *mut T {
> + let b = ManuallyDrop::new(b);
> +
> + b.0.as_ptr()

Why not a single line `ManuallyDrop::new().0.as_ptr()`?

> + }
> +
> + /// Consumes and leaks the `Box<T, A>` and returns a mutable reference.
> + ///
> + /// See [Box::into_raw] for more details.
> + #[inline]
> + pub fn leak<'a>(b: Self) -> &'a mut T {
> + // SAFETY: `Box::into_raw` always returns a properly aligned and dereferenceable pointer
> + // which points to an initialized instance of `T`.
> + unsafe { &mut *Box::into_raw(b) }
> + }
> +}