`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 | 480 ++++++++++++++++++++++++++++++++++++++
 rust/kernel/prelude.rs    |   2 +-
 3 files changed, 487 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/alloc/kbox.rs

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index 53a93617a9f6..0c9bb60250af 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -5,6 +5,7 @@
 #[cfg(not(any(test, testlib)))]
 pub mod allocator;
 pub mod box_ext;
+pub mod kbox;
 pub mod vec_ext;
 
 #[cfg(any(test, testlib))]
@@ -13,6 +14,11 @@
 #[cfg(any(test, testlib))]
 pub use self::allocator_test as allocator;
 
+pub use self::kbox::Box;
+pub use self::kbox::KBox;
+pub use self::kbox::KVBox;
+pub use self::kbox::VBox;
+
 /// Indicates an allocation error.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub struct AllocError;
diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
new file mode 100644
index 000000000000..93b1ab9de6e8
--- /dev/null
+++ b/rust/kernel/alloc/kbox.rs
@@ -0,0 +1,480 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Implementation of [`Box`].
+
+#[allow(unused_imports)] // Used in doc comments.
+use super::allocator::{KVmalloc, Kmalloc, Vmalloc};
+use super::{AllocError, Allocator, Flags};
+use core::fmt;
+use core::marker::PhantomData;
+use core::mem::ManuallyDrop;
+use core::mem::MaybeUninit;
+use core::ops::{Deref, DerefMut};
+use core::pin::Pin;
+use core::ptr::NonNull;
+use core::result::Result;
+
+use crate::init::{InPlaceInit, InPlaceWrite, Init, PinInit};
+use crate::types::ForeignOwnable;
+
+/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`.
+///
+/// This is the kernel's version of the Rust stdlib's `Box`. There are several of differences,
+/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not
+/// supported. There are also several API differences, e.g. `Box` always requires an [`Allocator`]
+/// implementation to be passed as generic, page [`Flags`] when allocating memory and all functions
+/// that may allocate memory are failable.
+///
+/// `Box` works with any of the kernel's allocators, e.g. [`Kmalloc`], [`Vmalloc`] or [`KVmalloc`].
+/// There are aliases for `Box` with these allocators ([`KBox`], [`VBox`], [`KVBox`]).
+///
+/// When dropping a [`Box`], the value is also dropped and the heap memory is automatically freed.
+///
+/// # 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());
+/// ```
+///
+/// ```
+/// # 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());
+/// ```
+///
+/// # Invariants
+///
+/// The [`Box`]' pointer is always properly aligned and either points to memory allocated with `A`
+/// or, for zero-sized types, is a dangling pointer.
+#[repr(transparent)]
+pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);
+
+/// Type alias for `Box` with a [`Kmalloc`] allocator.
+///
+/// # Examples
+///
+/// ```
+/// let b = KBox::new(24_u64, GFP_KERNEL)?;
+///
+/// assert_eq!(*b, 24_u64);
+/// # Ok::<(), Error>(())
+/// ```
+pub type KBox<T> = Box<T, super::allocator::Kmalloc>;
+
+/// Type alias for `Box` with a [`Vmalloc`] allocator.
+///
+/// # Examples
+///
+/// ```
+/// let b = VBox::new(24_u64, GFP_KERNEL)?;
+///
+/// assert_eq!(*b, 24_u64);
+/// # Ok::<(), Error>(())
+/// ```
+pub type VBox<T> = Box<T, super::allocator::Vmalloc>;
+
+/// Type alias for `Box` with a [`KVmalloc`] allocator.
+///
+/// # Examples
+///
+/// ```
+/// let b = KVBox::new(24_u64, GFP_KERNEL)?;
+///
+/// assert_eq!(*b, 24_u64);
+/// # Ok::<(), Error>(())
+/// ```
+pub type KVBox<T> = Box<T, super::allocator::KVmalloc>;
+
+// SAFETY: `Box` is `Send` if `T` is `Send` because the `Box` owns a `T`.
+unsafe impl<T, A> Send for Box<T, A>
+where
+    T: Send + ?Sized,
+    A: Allocator,
+{
+}
+
+// SAFETY: `Box` is `Sync` if `T` is `Sync` because the `Box` owns a `T`.
+unsafe impl<T, A> Sync for Box<T, A>
+where
+    T: Sync + ?Sized,
+    A: Allocator,
+{
+}
+
+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`.
+    #[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>)
+    }
+
+    /// 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()
+    }
+
+    /// 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) }
+    }
+}
+
+impl<T, A> Box<MaybeUninit<T>, A>
+where
+    A: Allocator,
+{
+    /// Converts a `Box<MaybeUninit<T>, A>` to a `Box<T, A>`.
+    ///
+    /// It is undefined behavior to call this function while the value inside of `b` is not yet
+    /// fully initialized.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the value inside of `b` is in an initialized state.
+    pub unsafe fn assume_init(b: Self) -> Box<T, A> {
+        let raw = Self::into_raw(b);
+
+        // SAFETY: `raw` comes from a previous call to `Box::into_raw`. By the safety requirements
+        // of this function, the value inside the `Box` is in an initialized state. Hence, it is
+        // safe to reconstruct the `Box` as `Box<T, A>`.
+        unsafe { Box::from_raw(raw as *mut T) }
+    }
+
+    /// Writes the value and converts to `Box<T, A>`.
+    pub fn write(mut b: Self, value: T) -> Box<T, A> {
+        (*b).write(value);
+        // SAFETY: We've just initialized `boxed`'s value.
+        unsafe { Self::assume_init(b) }
+    }
+}
+
+impl<T, A> Box<T, A>
+where
+    A: Allocator,
+{
+    fn is_zst() -> bool {
+        core::mem::size_of::<T>() == 0
+    }
+
+    /// Creates a new `Box<T, A>` and initializes its contents with `x`.
+    ///
+    /// New memory is allocated with `A`. The allocation may fail, in which case an error is
+    /// returned. For ZSTs no memory is allocated.
+    pub fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
+        let b = Self::new_uninit(flags)?;
+        Ok(Box::write(b, x))
+    }
+
+    /// Creates a new `Box<T, A>` with uninitialized contents.
+    ///
+    /// New memory is allocated with `A`. The allocation may fail, in which case an error is
+    /// returned. For ZSTs no memory is allocated.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// let b = KBox::<u64>::new_uninit(GFP_KERNEL)?;
+    /// let b = KBox::write(b, 24);
+    ///
+    /// assert_eq!(*b, 24_u64);
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>, A>, AllocError> {
+        let ptr = if Self::is_zst() {
+            NonNull::dangling()
+        } else {
+            let layout = core::alloc::Layout::new::<MaybeUninit<T>>();
+            let ptr = A::alloc(layout, flags)?;
+
+            ptr.cast()
+        };
+
+        // INVARIANT: `ptr` is either a dangling pointer or points to memory allocated with `A`,
+        // which is sufficient in size and alignment for storing a `T`.
+        Ok(Box(ptr, PhantomData::<A>))
+    }
+
+    /// Constructs a new `Pin<Box<T, A>>`. If `T` does not implement [`Unpin`], then `x` will be
+    /// pinned in memory and can't be moved.
+    #[inline]
+    pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
+    where
+        A: 'static,
+    {
+        Ok(Self::new(x, flags)?.into())
+    }
+
+    /// Forgets the contents (does not run the destructor), but keeps the allocation.
+    fn forget_contents(this: Self) -> Box<MaybeUninit<T>, A> {
+        let ptr = Self::into_raw(this);
+
+        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
+        unsafe { Box::from_raw(ptr.cast()) }
+    }
+
+    /// Drops the contents, but keeps the allocation.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// let value = KBox::new([0; 32], GFP_KERNEL)?;
+    /// assert_eq!(*value, [0; 32]);
+    /// let value = KBox::drop_contents(value);
+    /// // Now we can re-use `value`:
+    /// let value = KBox::write(value, [1; 32]);
+    /// assert_eq!(*value, [1; 32]);
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn drop_contents(this: Self) -> Box<MaybeUninit<T>, A> {
+        let ptr = this.0.as_ptr();
+
+        // SAFETY: `ptr` is valid, because it came from `this`. After this call we never access the
+        // value stored in `this` again.
+        unsafe { core::ptr::drop_in_place(ptr) };
+
+        Self::forget_contents(this)
+    }
+
+    /// Moves the `Box`' value out of the `Box` and consumes the `Box`.
+    pub fn into_inner(b: Self) -> T {
+        // SAFETY: By the type invariant `&*b` is valid for `read`.
+        let value = unsafe { core::ptr::read(&*b) };
+        let _ = Self::forget_contents(b);
+        value
+    }
+}
+
+impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
+where
+    T: ?Sized,
+    A: Allocator,
+{
+    /// Converts a `Box<T, A>` into a `Pin<Box<T, A>>`. If `T` does not implement [`Unpin`], then
+    /// `*b` will be pinned in memory and can't be moved.
+    ///
+    /// This moves `b` into `Pin` without moving `*b` or allocating and copying any memory.
+    fn from(b: Box<T, A>) -> Self {
+        // SAFETY: The value wrapped inside a `Pin<Box<T, A>>` cannot be moved or replaced as long
+        // as `T` does not implement `Unpin`.
+        unsafe { Pin::new_unchecked(b) }
+    }
+}
+
+impl<T, A> InPlaceWrite<T> for Box<MaybeUninit<T>, A>
+where
+    A: Allocator + 'static,
+{
+    type Initialized = Box<T, A>;
+
+    fn write_init<E>(mut self, init: impl Init<T, E>) -> Result<Self::Initialized, E> {
+        let slot = self.as_mut_ptr();
+        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
+        // slot is valid.
+        unsafe { init.__init(slot)? };
+        // SAFETY: All fields have been initialized.
+        Ok(unsafe { Box::assume_init(self) })
+    }
+
+    fn write_pin_init<E>(mut self, init: impl PinInit<T, E>) -> Result<Pin<Self::Initialized>, E> {
+        let slot = self.as_mut_ptr();
+        // SAFETY: When init errors/panics, slot will get deallocated but not dropped,
+        // slot is valid and will not be moved, because we pin it later.
+        unsafe { init.__pinned_init(slot)? };
+        // SAFETY: All fields have been initialized.
+        Ok(unsafe { Box::assume_init(self) }.into())
+    }
+}
+
+impl<T, A> InPlaceInit<T> for Box<T, A>
+where
+    A: Allocator + 'static,
+{
+    #[inline]
+    fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
+    where
+        E: From<AllocError>,
+    {
+        Box::<_, A>::new_uninit(flags)?.write_pin_init(init)
+    }
+
+    #[inline]
+    fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
+    where
+        E: From<AllocError>,
+    {
+        Box::<_, A>::new_uninit(flags)?.write_init(init)
+    }
+}
+
+impl<T: 'static, A> ForeignOwnable for Box<T, A>
+where
+    A: Allocator,
+{
+    type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a mut T;
+
+    fn into_foreign(self) -> *const core::ffi::c_void {
+        Box::into_raw(self) as _
+    }
+
+    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
+        // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
+        // call to `Self::into_foreign`.
+        unsafe { Box::from_raw(ptr as _) }
+    }
+
+    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a T {
+        // SAFETY: The safety requirements of this method ensure that the object remains alive and
+        // immutable for the duration of 'a.
+        unsafe { &*ptr.cast() }
+    }
+
+    unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> &'a mut T {
+        // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
+        // nothing else will access the value for the duration of 'a.
+        unsafe { &mut *ptr.cast_mut().cast() }
+    }
+}
+
+impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
+where
+    A: Allocator,
+{
+    type Borrowed<'a> = Pin<&'a T>;
+    type BorrowedMut<'a> = Pin<&'a mut T>;
+
+    fn into_foreign(self) -> *const core::ffi::c_void {
+        // SAFETY: We are still treating the box as pinned.
+        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) as _
+    }
+
+    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
+        // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
+        // call to `Self::into_foreign`.
+        unsafe { Pin::new_unchecked(Box::from_raw(ptr as _)) }
+    }
+
+    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Pin<&'a T> {
+        // SAFETY: The safety requirements for this function ensure that the object is still alive,
+        // so it is safe to dereference the raw pointer.
+        // The safety requirements of `from_foreign` also ensure that the object remains alive for
+        // the lifetime of the returned value.
+        let r = unsafe { &*ptr.cast() };
+
+        // SAFETY: This pointer originates from a `Pin<Box<T>>`.
+        unsafe { Pin::new_unchecked(r) }
+    }
+
+    unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> Pin<&'a mut T> {
+        // SAFETY: The safety requirements for this function ensure that the object is still alive,
+        // so it is safe to dereference the raw pointer.
+        // The safety requirements of `from_foreign` also ensure that the object remains alive for
+        // the lifetime of the returned value.
+        let r = unsafe { &mut *ptr.cast_mut().cast() };
+
+        // SAFETY: This pointer originates from a `Pin<Box<T>>`.
+        unsafe { Pin::new_unchecked(r) }
+    }
+}
+
+impl<T, A> Deref for Box<T, A>
+where
+    T: ?Sized,
+    A: Allocator,
+{
+    type Target = T;
+
+    fn deref(&self) -> &T {
+        // SAFETY: `self.0` is always properly aligned, dereferenceable and points to an initialized
+        // instance of `T`.
+        unsafe { self.0.as_ref() }
+    }
+}
+
+impl<T, A> DerefMut for Box<T, A>
+where
+    T: ?Sized,
+    A: Allocator,
+{
+    fn deref_mut(&mut self) -> &mut T {
+        // SAFETY: `self.0` is always properly aligned, dereferenceable and points to an initialized
+        // instance of `T`.
+        unsafe { self.0.as_mut() }
+    }
+}
+
+impl<T, A> fmt::Debug for Box<T, A>
+where
+    T: ?Sized + fmt::Debug,
+    A: Allocator,
+{
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt::Debug::fmt(&**self, f)
+    }
+}
+
+impl<T, A> Drop for Box<T, A>
+where
+    T: ?Sized,
+    A: Allocator,
+{
+    fn drop(&mut self) {
+        let size = core::mem::size_of_val::<T>(self);
+
+        // SAFETY: We need to drop `self.0` in place, before we free the backing memory.
+        unsafe { core::ptr::drop_in_place(self.0.as_ptr()) };
+
+        if size != 0 {
+            // SAFETY: `ptr` was previously allocated with `A`.
+            unsafe { A::free(self.0.cast()) };
+        }
+    }
+}
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index 4571daec0961..a9210634a8c3 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -14,7 +14,7 @@
 #[doc(no_inline)]
 pub use core::pin::Pin;
 
-pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt};
+pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt, KBox, KVBox, VBox};
 
 #[doc(no_inline)]
 pub use alloc::{boxed::Box, vec::Vec};
-- 
2.46.0

On 16.08.24 02:10, Danilo Krummrich wrote:
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> new file mode 100644
> index 000000000000..93b1ab9de6e8
> --- /dev/null
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -0,0 +1,480 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Implementation of [`Box`].
> +
> +#[allow(unused_imports)] // Used in doc comments.
> +use super::allocator::{KVmalloc, Kmalloc, Vmalloc};
> +use super::{AllocError, Allocator, Flags};
> +use core::fmt;
> +use core::marker::PhantomData;
> +use core::mem::ManuallyDrop;
> +use core::mem::MaybeUninit;
> +use core::ops::{Deref, DerefMut};
> +use core::pin::Pin;
> +use core::ptr::NonNull;
> +use core::result::Result;
> +
> +use crate::init::{InPlaceInit, InPlaceWrite, Init, PinInit};
> +use crate::types::ForeignOwnable;
> +
> +/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`.

A single `-` doesn't really render nicely in markdown, instead use a
double or triple dash (`--` or `---`).

> +///
> +/// This is the kernel's version of the Rust stdlib's `Box`. There are several of differences,
> +/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not
> +/// supported. There are also several API differences, e.g. `Box` always requires an [`Allocator`]
> +/// implementation to be passed as generic, page [`Flags`] when allocating memory and all functions
> +/// that may allocate memory are failable.

Do you mean fallible?

> +///
> +/// `Box` works with any of the kernel's allocators, e.g. [`Kmalloc`], [`Vmalloc`] or [`KVmalloc`].
> +/// There are aliases for `Box` with these allocators ([`KBox`], [`VBox`], [`KVBox`]).
> +///
> +/// When dropping a [`Box`], the value is also dropped and the heap memory is automatically freed.
> +///
> +/// # 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.

> +///
> +/// ```
> +/// # 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.".

> +#[repr(transparent)]
> +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);
> +
> +/// Type alias for `Box` with a [`Kmalloc`] allocator.

Can you make these `Box` references links?

> +///
> +/// # Examples
> +///
> +/// ```
> +/// let b = KBox::new(24_u64, GFP_KERNEL)?;
> +///
> +/// assert_eq!(*b, 24_u64);
> +/// # Ok::<(), Error>(())
> +/// ```
> +pub type KBox<T> = Box<T, super::allocator::Kmalloc>;
> +
> +/// Type alias for `Box` with a [`Vmalloc`] allocator.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// let b = VBox::new(24_u64, GFP_KERNEL)?;
> +///
> +/// assert_eq!(*b, 24_u64);
> +/// # Ok::<(), Error>(())
> +/// ```
> +pub type VBox<T> = Box<T, super::allocator::Vmalloc>;
> +
> +/// Type alias for `Box` with a [`KVmalloc`] allocator.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// let b = KVBox::new(24_u64, GFP_KERNEL)?;
> +///
> +/// assert_eq!(*b, 24_u64);
> +/// # Ok::<(), Error>(())
> +/// ```
> +pub type KVBox<T> = Box<T, super::allocator::KVmalloc>;
> +
> +// SAFETY: `Box` is `Send` if `T` is `Send` because the `Box` owns a `T`.
> +unsafe impl<T, A> Send for Box<T, A>
> +where
> +    T: Send + ?Sized,
> +    A: Allocator,
> +{
> +}
> +
> +// SAFETY: `Box` is `Sync` if `T` is `Sync` because the `Box` owns a `T`.
> +unsafe impl<T, A> Sync for Box<T, A>
> +where
> +    T: Sync + ?Sized,
> +    A: Allocator,
> +{
> +}
> +
> +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.

> +    #[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>)
> +    }
> +
> +    /// 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()
> +    }
> +
> +    /// 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) }
> +    }
> +}
> +
> +impl<T, A> Box<MaybeUninit<T>, A>
> +where
> +    A: Allocator,
> +{
> +    /// Converts a `Box<MaybeUninit<T>, A>` to a `Box<T, A>`.
> +    ///
> +    /// It is undefined behavior to call this function while the value inside of `b` is not yet
> +    /// fully initialized.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the value inside of `b` is in an initialized state.
> +    pub unsafe fn assume_init(b: Self) -> Box<T, A> {
> +        let raw = Self::into_raw(b);
> +
> +        // SAFETY: `raw` comes from a previous call to `Box::into_raw`. By the safety requirements
> +        // of this function, the value inside the `Box` is in an initialized state. Hence, it is
> +        // safe to reconstruct the `Box` as `Box<T, A>`.
> +        unsafe { Box::from_raw(raw as *mut T) }

You should be able to use `.cast()` instead.

> +    }
> +
> +    /// Writes the value and converts to `Box<T, A>`.
> +    pub fn write(mut b: Self, value: T) -> Box<T, A> {
> +        (*b).write(value);
> +        // SAFETY: We've just initialized `boxed`'s value.

The variable is called `b`.

> +        unsafe { Self::assume_init(b) }
> +    }
> +}

[...]

> +impl<T, A> Drop for Box<T, A>
> +where
> +    T: ?Sized,
> +    A: Allocator,
> +{
> +    fn drop(&mut self) {
> +        let size = core::mem::size_of_val::<T>(self);
> +
> +        // SAFETY: We need to drop `self.0` in place, before we free the backing memory.

This is the reason you are calling this function, not the justification
why it is OK to do so. (the pointer is valid)

> +        unsafe { core::ptr::drop_in_place(self.0.as_ptr()) };

Instead of using the raw pointer directly, you can also just use
`deref_mut`.

> +
> +        if size != 0 {
> +            // SAFETY: `ptr` was previously allocated with `A`.

There is no variable `ptr`, this is guaranteed by the type invariant of
`Self`.

---
Cheers,
Benno

> +            unsafe { A::free(self.0.cast()) };
> +        }
> +    }
> +}
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index 4571daec0961..a9210634a8c3 100644
> --- a/rust/kernel/prelude.rs
> +++ b/rust/kernel/prelude.rs
> @@ -14,7 +14,7 @@
>  #[doc(no_inline)]
>  pub use core::pin::Pin;
> 
> -pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt};
> +pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt, KBox, KVBox, VBox};
> 
>  #[doc(no_inline)]
>  pub use alloc::{boxed::Box, vec::Vec};
> --
> 2.46.0
> 

On Sat, Aug 31, 2024 at 05:39:07AM +0000, Benno Lossin wrote:
> On 16.08.24 02:10, Danilo Krummrich wrote:
> > diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> > new file mode 100644
> > index 000000000000..93b1ab9de6e8
> > --- /dev/null
> > +++ b/rust/kernel/alloc/kbox.rs
> > @@ -0,0 +1,480 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Implementation of [`Box`].
> > +
> > +#[allow(unused_imports)] // Used in doc comments.
> > +use super::allocator::{KVmalloc, Kmalloc, Vmalloc};
> > +use super::{AllocError, Allocator, Flags};
> > +use core::fmt;
> > +use core::marker::PhantomData;
> > +use core::mem::ManuallyDrop;
> > +use core::mem::MaybeUninit;
> > +use core::ops::{Deref, DerefMut};
> > +use core::pin::Pin;
> > +use core::ptr::NonNull;
> > +use core::result::Result;
> > +
> > +use crate::init::{InPlaceInit, InPlaceWrite, Init, PinInit};
> > +use crate::types::ForeignOwnable;
> > +
> > +/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`.
> 
> A single `-` doesn't really render nicely in markdown, instead use a
> double or triple dash (`--` or `---`).
> 
> > +///
> > +/// This is the kernel's version of the Rust stdlib's `Box`. There are several of differences,
> > +/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not
> > +/// supported. There are also several API differences, e.g. `Box` always requires an [`Allocator`]
> > +/// implementation to be passed as generic, page [`Flags`] when allocating memory and all functions
> > +/// that may allocate memory are failable.
> 
> Do you mean fallible?
> 
> > +///
> > +/// `Box` works with any of the kernel's allocators, e.g. [`Kmalloc`], [`Vmalloc`] or [`KVmalloc`].
> > +/// There are aliases for `Box` with these allocators ([`KBox`], [`VBox`], [`KVBox`]).
> > +///
> > +/// When dropping a [`Box`], the value is also dropped and the heap memory is automatically freed.
> > +///
> > +/// # 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.

> 
> > +///
> > +/// ```
> > +/// # 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?

> 
> > +#[repr(transparent)]
> > +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);
> > +
> > +/// Type alias for `Box` with a [`Kmalloc`] allocator.
> 
> Can you make these `Box` references links?
> 
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let b = KBox::new(24_u64, GFP_KERNEL)?;
> > +///
> > +/// assert_eq!(*b, 24_u64);
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +pub type KBox<T> = Box<T, super::allocator::Kmalloc>;
> > +
> > +/// Type alias for `Box` with a [`Vmalloc`] allocator.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let b = VBox::new(24_u64, GFP_KERNEL)?;
> > +///
> > +/// assert_eq!(*b, 24_u64);
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +pub type VBox<T> = Box<T, super::allocator::Vmalloc>;
> > +
> > +/// Type alias for `Box` with a [`KVmalloc`] allocator.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let b = KVBox::new(24_u64, GFP_KERNEL)?;
> > +///
> > +/// assert_eq!(*b, 24_u64);
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +pub type KVBox<T> = Box<T, super::allocator::KVmalloc>;
> > +
> > +// SAFETY: `Box` is `Send` if `T` is `Send` because the `Box` owns a `T`.
> > +unsafe impl<T, A> Send for Box<T, A>
> > +where
> > +    T: Send + ?Sized,
> > +    A: Allocator,
> > +{
> > +}
> > +
> > +// SAFETY: `Box` is `Sync` if `T` is `Sync` because the `Box` owns a `T`.
> > +unsafe impl<T, A> Sync for Box<T, A>
> > +where
> > +    T: Sync + ?Sized,
> > +    A: Allocator,
> > +{
> > +}
> > +
> > +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?

> 
> > +    #[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>)
> > +    }
> > +
> > +    /// 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()
> > +    }
> > +
> > +    /// 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) }
> > +    }
> > +}
> > +
> > +impl<T, A> Box<MaybeUninit<T>, A>
> > +where
> > +    A: Allocator,
> > +{
> > +    /// Converts a `Box<MaybeUninit<T>, A>` to a `Box<T, A>`.
> > +    ///
> > +    /// It is undefined behavior to call this function while the value inside of `b` is not yet
> > +    /// fully initialized.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that the value inside of `b` is in an initialized state.
> > +    pub unsafe fn assume_init(b: Self) -> Box<T, A> {
> > +        let raw = Self::into_raw(b);
> > +
> > +        // SAFETY: `raw` comes from a previous call to `Box::into_raw`. By the safety requirements
> > +        // of this function, the value inside the `Box` is in an initialized state. Hence, it is
> > +        // safe to reconstruct the `Box` as `Box<T, A>`.
> > +        unsafe { Box::from_raw(raw as *mut T) }
> 
> You should be able to use `.cast()` instead.
> 
> > +    }
> > +
> > +    /// Writes the value and converts to `Box<T, A>`.
> > +    pub fn write(mut b: Self, value: T) -> Box<T, A> {
> > +        (*b).write(value);
> > +        // SAFETY: We've just initialized `boxed`'s value.
> 
> The variable is called `b`.
> 
> > +        unsafe { Self::assume_init(b) }
> > +    }
> > +}
> 
> [...]
> 
> > +impl<T, A> Drop for Box<T, A>
> > +where
> > +    T: ?Sized,
> > +    A: Allocator,
> > +{
> > +    fn drop(&mut self) {
> > +        let size = core::mem::size_of_val::<T>(self);
> > +
> > +        // SAFETY: We need to drop `self.0` in place, before we free the backing memory.
> 
> This is the reason you are calling this function, not the justification
> why it is OK to do so. (the pointer is valid)
> 
> > +        unsafe { core::ptr::drop_in_place(self.0.as_ptr()) };
> 
> Instead of using the raw pointer directly, you can also just use
> `deref_mut`.
> 
> > +
> > +        if size != 0 {
> > +            // SAFETY: `ptr` was previously allocated with `A`.
> 
> There is no variable `ptr`, this is guaranteed by the type invariant of
> `Self`.
> 
> ---
> Cheers,
> Benno
> 
> > +            unsafe { A::free(self.0.cast()) };
> > +        }
> > +    }
> > +}
> > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> > index 4571daec0961..a9210634a8c3 100644
> > --- a/rust/kernel/prelude.rs
> > +++ b/rust/kernel/prelude.rs
> > @@ -14,7 +14,7 @@
> >  #[doc(no_inline)]
> >  pub use core::pin::Pin;
> > 
> > -pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt};
> > +pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt, KBox, KVBox, VBox};
> > 
> >  #[doc(no_inline)]
> >  pub use alloc::{boxed::Box, vec::Vec};
> > --
> > 2.46.0
> > 
>

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

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

On 11.09.24 01:25, Danilo Krummrich wrote:
> 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]

That's a good point, the documentation looks a bit outdated. I found
this page in the nomicon: https://doc.rust-lang.org/nomicon/vec/vec-zsts.html
The first iterator implementation has an alignment issue. (Nevertheless,
that chapter of the nomicon is probably useful to you, since it goes
over implementing `Vec`, but maybe you already saw it)

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

Might be a good idea to improve/complain about this at the rust project.

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

Dereferencing (ie `*t`) also requires that (I just didn't do it, since
then the `Token` must implement `Copy`).

---
Cheers,
Benno

On Wed, Sep 11, 2024 at 08:36:38AM +0000, Benno Lossin wrote:
> On 11.09.24 01:25, Danilo Krummrich wrote:
> > 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]
> 
> That's a good point, the documentation looks a bit outdated. I found
> this page in the nomicon: https://doc.rust-lang.org/nomicon/vec/vec-zsts.html
> The first iterator implementation has an alignment issue. (Nevertheless,
> that chapter of the nomicon is probably useful to you, since it goes
> over implementing `Vec`, but maybe you already saw it)
> 
> > [1] https://doc.rust-lang.org/std/ptr/index.html
> 
> Might be a good idea to improve/complain about this at the rust project.

Well, my point is how do we know? There's no language specification and the
documentation is (at least) ambiguous.

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

I mentioned this because for `read` it's explicitly documented.

However, the nomicon also says "This is possibly needless pedantry because
ptr::read is a noop for a ZST, [...]".

> 
> Dereferencing (ie `*t`) also requires that (I just didn't do it, since
> then the `Token` must implement `Copy`).

Again, how do you know? The documentation isn't clear about it.

> 
> ---
> Cheers,
> Benno
>

On 11.09.24 13:02, Danilo Krummrich wrote:
> On Wed, Sep 11, 2024 at 08:36:38AM +0000, Benno Lossin wrote:
>> On 11.09.24 01:25, Danilo Krummrich wrote:
>>> 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:
>>>>>>> +///
>>>>>>> +/// ```
>>>>>>> +/// # 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]
>>
>> That's a good point, the documentation looks a bit outdated. I found
>> this page in the nomicon: https://doc.rust-lang.org/nomicon/vec/vec-zsts.html
>> The first iterator implementation has an alignment issue. (Nevertheless,
>> that chapter of the nomicon is probably useful to you, since it goes
>> over implementing `Vec`, but maybe you already saw it)
>>
>>> [1] https://doc.rust-lang.org/std/ptr/index.html
>>
>> Might be a good idea to improve/complain about this at the rust project.
> 
> Well, my point is how do we know? There's no language specification and the
> documentation is (at least) ambiguous.

So I went through the unsafe-coding-guidelines issues list and only
found this one: https://github.com/rust-lang/unsafe-code-guidelines/issues/93
Maybe I missed something. You could also try to ask at the rust zulip in
the t-opsem channel for further clarification.

I think we should just be on the safe side and assume that ZSTs require
alignment. But if you get a convincing answer and if they say that they
will document it, then I don't mind removing the alignment requirement.

---
Cheers,
Benno

On Wed, Sep 11, 2024 at 3:26 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 11.09.24 13:02, Danilo Krummrich wrote:
> > On Wed, Sep 11, 2024 at 08:36:38AM +0000, Benno Lossin wrote:
> >> On 11.09.24 01:25, Danilo Krummrich wrote:
> >>> 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:
> >>>>>>> +///
> >>>>>>> +/// ```
> >>>>>>> +/// # 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]
> >>
> >> That's a good point, the documentation looks a bit outdated. I found
> >> this page in the nomicon: https://doc.rust-lang.org/nomicon/vec/vec-zsts.html
> >> The first iterator implementation has an alignment issue. (Nevertheless,
> >> that chapter of the nomicon is probably useful to you, since it goes
> >> over implementing `Vec`, but maybe you already saw it)
> >>
> >>> [1] https://doc.rust-lang.org/std/ptr/index.html
> >>
> >> Might be a good idea to improve/complain about this at the rust project.
> >
> > Well, my point is how do we know? There's no language specification and the
> > documentation is (at least) ambiguous.
>
> So I went through the unsafe-coding-guidelines issues list and only
> found this one: https://github.com/rust-lang/unsafe-code-guidelines/issues/93
> Maybe I missed something. You could also try to ask at the rust zulip in
> the t-opsem channel for further clarification.
>
> I think we should just be on the safe side and assume that ZSTs require
> alignment. But if you get a convincing answer and if they say that they
> will document it, then I don't mind removing the alignment requirement.

Please see the section on alignment in the same page. Just because a
pointer is valid does not mean that it is properly aligned.

From the page:

Valid raw pointers as defined above are not necessarily properly
aligned (where “proper” alignment is defined by the pointee type,
i.e., *const T must be aligned to mem::align_of::<T>()). However, most
functions require their arguments to be properly aligned, and will
explicitly state this requirement in their documentation. Notable
exceptions to this are read_unaligned and write_unaligned.

When a function requires proper alignment, it does so even if the
access has size 0, i.e., even if memory is not actually touched.
Consider using NonNull::dangling in such cases.

Alice

On Wed, Sep 11, 2024 at 03:27:57PM +0200, Alice Ryhl wrote:
> On Wed, Sep 11, 2024 at 3:26 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > On 11.09.24 13:02, Danilo Krummrich wrote:
> > > On Wed, Sep 11, 2024 at 08:36:38AM +0000, Benno Lossin wrote:
> > >> On 11.09.24 01:25, Danilo Krummrich wrote:
> > >>> 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:
> > >>>>>>> +///
> > >>>>>>> +/// ```
> > >>>>>>> +/// # 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]
> > >>
> > >> That's a good point, the documentation looks a bit outdated. I found
> > >> this page in the nomicon: https://doc.rust-lang.org/nomicon/vec/vec-zsts.html
> > >> The first iterator implementation has an alignment issue. (Nevertheless,
> > >> that chapter of the nomicon is probably useful to you, since it goes
> > >> over implementing `Vec`, but maybe you already saw it)
> > >>
> > >>> [1] https://doc.rust-lang.org/std/ptr/index.html
> > >>
> > >> Might be a good idea to improve/complain about this at the rust project.
> > >
> > > Well, my point is how do we know? There's no language specification and the
> > > documentation is (at least) ambiguous.
> >
> > So I went through the unsafe-coding-guidelines issues list and only
> > found this one: https://github.com/rust-lang/unsafe-code-guidelines/issues/93
> > Maybe I missed something. You could also try to ask at the rust zulip in
> > the t-opsem channel for further clarification.
> >
> > I think we should just be on the safe side and assume that ZSTs require
> > alignment. But if you get a convincing answer and if they say that they
> > will document it, then I don't mind removing the alignment requirement.

I agree -- I also wrote this in a previous mail.

I was just wondering why you are so sure about it, since the documentation
doesn't seem to be clear about it.

> 
> Please see the section on alignment in the same page. Just because a
> pointer is valid does not mean that it is properly aligned.
> 
> From the page:
> 
> Valid raw pointers as defined above are not necessarily properly
> aligned (where “proper” alignment is defined by the pointee type,
> i.e., *const T must be aligned to mem::align_of::<T>()). However, most
> functions require their arguments to be properly aligned, and will
> explicitly state this requirement in their documentation. Notable
> exceptions to this are read_unaligned and write_unaligned.
> 
> When a function requires proper alignment, it does so even if the
> access has size 0, i.e., even if memory is not actually touched.
> Consider using NonNull::dangling in such cases.

Good point.

It still sounds like it's only required for functions that explicitly state so.

And as cited from nomicon "This is possibly needless pedantry because ptr::read
is a noop for a ZST, [...]". But, no question, of course we have to honor it
anyways.

> 
> Alice
> 

On 11.09.24 16:50, Danilo Krummrich wrote:
> On Wed, Sep 11, 2024 at 03:27:57PM +0200, Alice Ryhl wrote:
>> On Wed, Sep 11, 2024 at 3:26 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>> On 11.09.24 13:02, Danilo Krummrich wrote:
>>>> On Wed, Sep 11, 2024 at 08:36:38AM +0000, Benno Lossin wrote:
>>>>> On 11.09.24 01:25, Danilo Krummrich wrote:
>>>>>> 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:
>>>>>>>>>> +///
>>>>>>>>>> +/// ```
>>>>>>>>>> +/// # 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]
>>>>>
>>>>> That's a good point, the documentation looks a bit outdated. I found
>>>>> this page in the nomicon: https://doc.rust-lang.org/nomicon/vec/vec-zsts.html
>>>>> The first iterator implementation has an alignment issue. (Nevertheless,
>>>>> that chapter of the nomicon is probably useful to you, since it goes
>>>>> over implementing `Vec`, but maybe you already saw it)
>>>>>
>>>>>> [1] https://doc.rust-lang.org/std/ptr/index.html
>>>>>
>>>>> Might be a good idea to improve/complain about this at the rust project.
>>>>
>>>> Well, my point is how do we know? There's no language specification and the
>>>> documentation is (at least) ambiguous.
>>>
>>> So I went through the unsafe-coding-guidelines issues list and only
>>> found this one: https://github.com/rust-lang/unsafe-code-guidelines/issues/93
>>> Maybe I missed something. You could also try to ask at the rust zulip in
>>> the t-opsem channel for further clarification.
>>>
>>> I think we should just be on the safe side and assume that ZSTs require
>>> alignment. But if you get a convincing answer and if they say that they
>>> will document it, then I don't mind removing the alignment requirement.
> 
> I agree -- I also wrote this in a previous mail.
> 
> I was just wondering why you are so sure about it, since the documentation
> doesn't seem to be clear about it.

As Alice found below, the documentation is actually clear about this. (I
think I read it at some point, but forgot exactly where it was)

Maybe it could be better documented that dereferencing has the same
requirements as `read` (or whatever they are).

>> Please see the section on alignment in the same page. Just because a
>> pointer is valid does not mean that it is properly aligned.
>>
>> From the page:
>>
>> Valid raw pointers as defined above are not necessarily properly
>> aligned (where “proper” alignment is defined by the pointee type,
>> i.e., *const T must be aligned to mem::align_of::<T>()). However, most
>> functions require their arguments to be properly aligned, and will
>> explicitly state this requirement in their documentation. Notable
>> exceptions to this are read_unaligned and write_unaligned.
>>
>> When a function requires proper alignment, it does so even if the
>> access has size 0, i.e., even if memory is not actually touched.
>> Consider using NonNull::dangling in such cases.
> 
> Good point.
> 
> It still sounds like it's only required for functions that explicitly state so.
> 
> And as cited from nomicon "This is possibly needless pedantry because ptr::read
> is a noop for a ZST, [...]". But, no question, of course we have to honor it
> anyways.

This sounds to me like an implementation detail note, not something that
a caller should consider. But that's my interpretation.

---
Cheers,
Benno

On Fri, Aug 16, 2024 at 2:13 AM Danilo Krummrich <dakr@kernel.org> 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>

Overall looks good to me, but I have a question:

> +impl<T: 'static, A> ForeignOwnable for Box<T, A>
> +where
> +    A: Allocator,
> +{
> +    type Borrowed<'a> = &'a T;
> +    type BorrowedMut<'a> = &'a mut T;
> [..]
> +    unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> &'a mut T {
> +        // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
> +        // nothing else will access the value for the duration of 'a.
> +        unsafe { &mut *ptr.cast_mut().cast() }
> +    }

Where does this come from? It looks like you've based the series on
top of [1], but I dropped that patch a long time ago, and I don't see
it in rust-dev anymore.

Alice

[1]: https://lore.kernel.org/all/20230710074642.683831-1-aliceryhl@google.com/

On Tue, Aug 20, 2024 at 11:47:45AM +0200, Alice Ryhl wrote:
> On Fri, Aug 16, 2024 at 2:13 AM Danilo Krummrich <dakr@kernel.org> 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>
> 
> Overall looks good to me, but I have a question:
> 
> > +impl<T: 'static, A> ForeignOwnable for Box<T, A>
> > +where
> > +    A: Allocator,
> > +{
> > +    type Borrowed<'a> = &'a T;
> > +    type BorrowedMut<'a> = &'a mut T;
> > [..]
> > +    unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> &'a mut T {
> > +        // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
> > +        // nothing else will access the value for the duration of 'a.
> > +        unsafe { &mut *ptr.cast_mut().cast() }
> > +    }
> 
> Where does this come from? It looks like you've based the series on
> top of [1], but I dropped that patch a long time ago, and I don't see
> it in rust-dev anymore.

I comes from me rebasing onto rust-dev. When Boqun asked me to resolve the merge
conflicts a few days ago, this patch was in rust-dev. I think it disappeared two
days ago or so.

@Bonqun: Need to me to rebase again?

- Danilo

> 
> Alice
> 
> [1]: https://lore.kernel.org/all/20230710074642.683831-1-aliceryhl@google.com/
> 

On Tue, Aug 20, 2024 at 05:26:22PM +0200, Danilo Krummrich wrote:
> On Tue, Aug 20, 2024 at 11:47:45AM +0200, Alice Ryhl wrote:
> > On Fri, Aug 16, 2024 at 2:13 AM Danilo Krummrich <dakr@kernel.org> 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>
> > 
> > Overall looks good to me, but I have a question:
> > 
> > > +impl<T: 'static, A> ForeignOwnable for Box<T, A>
> > > +where
> > > +    A: Allocator,
> > > +{
> > > +    type Borrowed<'a> = &'a T;
> > > +    type BorrowedMut<'a> = &'a mut T;
> > > [..]
> > > +    unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> &'a mut T {
> > > +        // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
> > > +        // nothing else will access the value for the duration of 'a.
> > > +        unsafe { &mut *ptr.cast_mut().cast() }
> > > +    }
> > 
> > Where does this come from? It looks like you've based the series on
> > top of [1], but I dropped that patch a long time ago, and I don't see
> > it in rust-dev anymore.
> 
> I comes from me rebasing onto rust-dev. When Boqun asked me to resolve the merge
> conflicts a few days ago, this patch was in rust-dev. I think it disappeared two
> days ago or so.
> 
> @Bonqun: Need to me to rebase again?
> 

No need to rebase for this. I've already applied your series on
rust-dev. The background of carrying `borrow_mut` is because it was
simple to do, and so I carried it as the best effort, now with more
`ForeignOwnable` ipmls, I think it's better if we just drop it, and
re-introduce if needed.

Sorry for a bit out-of-sync on this one.

Regards,
Boqun

> - Danilo
> 
> > 
> > Alice
> > 
> > [1]: https://lore.kernel.org/all/20230710074642.683831-1-aliceryhl@google.com/
> >