`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 | 330 ++++++++++++++++++++++++++++++++++++++
rust/kernel/init.rs | 35 +++-
rust/kernel/prelude.rs | 2 +-
rust/kernel/types.rs | 56 +++++++
5 files changed, 427 insertions(+), 2 deletions(-)
create mode 100644 rust/kernel/alloc/kbox.rs
diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index 942e2755f217..d7beaf0372af 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..4a4379980745
--- /dev/null
+++ b/rust/kernel/alloc/kbox.rs
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Implementation of [`Box`].
+
+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::result::Result;
+
+use crate::types::Unique;
+
+/// The kernel's [`Box`] type.
+///
+/// `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`.
+///
+/// For non-zero-sized values, a [`Box`] will use the given allocator `A` for its allocation. For
+/// the most common allocators the type aliases `KBox`, `VBox` and `KVBox` exist.
+///
+/// It is valid to convert both ways between a [`Box`] and a raw pointer allocated with any
+/// `Allocator`, given that the `Layout` used with the allocator is correct for the type.
+///
+/// For zero-sized values the [`Box`]' pointer must be `dangling_mut::<T>`; no memory is allocated.
+///
+/// So long as `T: Sized`, a `Box<T>` is guaranteed to be represented as a single pointer and is
+/// also ABI-compatible with C pointers (i.e. the C type `T*`).
+///
+/// # Invariants
+///
+/// The [`Box`]' pointer always properly aligned and either points to memory allocated with `A` or,
+/// for zero-sized types, is a dangling pointer.
+///
+/// # Examples
+///
+/// ```
+/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?;
+///
+/// assert_eq!(*b, 24_u64);
+///
+/// # Ok::<(), Error>(())
+/// ```
+///
+/// ```
+/// struct Huge([u8; 1 << 24]);
+///
+/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok());
+/// ```
+pub struct Box<T: ?Sized, A: Allocator>(Unique<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>;
+
+impl<T, A> Box<T, A>
+where
+ T: ?Sized,
+ A: Allocator,
+{
+ /// Constructs a `Box<T, A>` from a raw pointer.
+ ///
+ /// # Safety
+ ///
+ /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
+ /// type `T`.
+ #[inline]
+ pub const unsafe fn from_raw(raw: *mut T) -> Self {
+ // SAFETY: Validity of `raw` is guaranteed by the safety preconditions of this function.
+ Self(unsafe { Unique::new_unchecked(raw) }, PhantomData::<A>)
+ }
+
+ /// Consumes the `Box<T>`, returning a wrapped raw pointer.
+ ///
+ /// # 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>`, returning a mutable reference, &'a mut T.
+ #[inline]
+ pub fn leak<'a>(b: Self) -> &'a mut T
+ where
+ T: 'a,
+ {
+ // 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) }
+ }
+
+ /// Converts a `Box<T, A>` into a `Pin<Box<T, A>>`.
+ #[inline]
+ pub fn into_pin(b: Self) -> Pin<Self>
+ where
+ A: 'static,
+ {
+ // SAFETY: It's not possible to move or replace the insides of a `Pin<Box<T, A>>` when
+ // `T: !Unpin`, so it's safe to pin it directly without any additional requirements.
+ unsafe { Pin::new_unchecked(b) }
+ }
+}
+
+impl<T, A> Box<MaybeUninit<T>, A>
+where
+ A: Allocator,
+{
+ /// Converts to `Box<T, A>`.
+ ///
+ /// # Safety
+ ///
+ /// As with MaybeUninit::assume_init, it is up to the caller to guarantee that the value really
+ /// is in an initialized state. Calling this when the content is not yet fully initialized
+ /// causes immediate undefined behavior.
+ pub unsafe fn assume_init(b: Self) -> Box<T, A> {
+ let raw = Self::into_raw(b);
+ // SAFETY: Reconstruct the `Box<MaybeUninit<T>, A>` as Box<T, A> now that has been
+ // initialized. `raw` and `alloc` are safe by the invariants of `Box`.
+ 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
+ }
+
+ /// Allocates memory with the allocator `A` and then places `x` into it.
+ ///
+ /// This doesn’t actually allocate if T is zero-sized.
+ pub fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
+ let b = Self::new_uninit(flags)?;
+ Ok(Box::write(b, x))
+ }
+
+ /// Constructs a new `Box<T, A>` with uninitialized contents.
+ ///
+ /// # 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() {
+ Unique::dangling()
+ } else {
+ let layout = core::alloc::Layout::new::<MaybeUninit<T>>();
+ let ptr = A::alloc(layout, flags)?;
+
+ ptr.cast().into()
+ };
+
+ 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 unable to 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())
+ }
+
+ /// 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 = Box::into_raw(this);
+ // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
+ unsafe { core::ptr::drop_in_place(ptr) };
+ // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
+ unsafe { Box::from_raw(ptr.cast()) }
+ }
+}
+
+impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
+where
+ T: ?Sized,
+ A: Allocator,
+ A: 'static,
+{
+ /// Converts a `Box<T>` into a `Pin<Box<T>>`. If `T` does not implement [`Unpin`], then
+ /// `*boxed` will be pinned in memory and unable to be moved.
+ ///
+ /// This conversion does not allocate on the heap and happens in place.
+ ///
+ /// This is also available via [`Box::into_pin`].
+ ///
+ /// Constructing and pinning a `Box` with <code><Pin<Box\<T>>>::from([Box::new]\(x))</code>
+ /// can also be written more concisely using <code>[Box::pin]\(x)</code>.
+ /// This `From` implementation is useful if you already have a `Box<T>`, or you are
+ /// constructing a (pinned) `Box` in a different way than with [`Box::new`].
+ fn from(b: Box<T, A>) -> Self {
+ Box::into_pin(b)
+ }
+}
+
+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 ptr = self.0.as_ptr();
+
+ // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
+ // instance of `T`.
+ let size = unsafe { core::mem::size_of_val(&*ptr) };
+
+ // SAFETY: We need to drop `self.0` in place, before we free the backing memory.
+ unsafe { core::ptr::drop_in_place(ptr) };
+
+ if size != 0 {
+ // SAFETY: `ptr` was previously allocated with `A`.
+ unsafe { A::free(self.0.as_non_null().cast()) };
+ }
+ }
+}
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 495c09ebe3a3..5fd7a0ffabd2 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -211,7 +211,7 @@
//! [`pin_init!`]: crate::pin_init!
use crate::{
- alloc::{box_ext::BoxExt, AllocError, Flags},
+ alloc::{box_ext::BoxExt, AllocError, Allocator, Flags},
error::{self, Error},
sync::UniqueArc,
types::{Opaque, ScopeGuard},
@@ -1178,6 +1178,39 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
}
}
+impl<T, A> InPlaceInit<T> for crate::alloc::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>,
+ {
+ let mut this = crate::alloc::Box::<_, A>::new_uninit(flags)?;
+ let slot = this.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 { crate::alloc::Box::assume_init(this) }.into())
+ }
+
+ #[inline]
+ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
+ where
+ E: From<AllocError>,
+ {
+ let mut this = crate::alloc::Box::<_, A>::new_uninit(flags)?;
+ let slot = this.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 { crate::alloc::Box::assume_init(this) })
+ }
+}
+
impl<T> InPlaceInit<T> for UniqueArc<T> {
#[inline]
fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index b37a0b3180fb..39f9331a48e2 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};
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 7cf89067b5fc..9fe87528d129 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -2,6 +2,7 @@
//! Kernel types.
+use crate::alloc::Allocator;
use crate::init::{self, PinInit};
use alloc::boxed::Box;
use core::{
@@ -9,6 +10,7 @@
marker::{PhantomData, PhantomPinned},
mem::MaybeUninit,
ops::{Deref, DerefMut},
+ pin::Pin,
ptr::NonNull,
};
@@ -89,6 +91,60 @@ unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
}
}
+impl<T: 'static, A> ForeignOwnable for crate::alloc::Box<T, A>
+where
+ A: Allocator,
+{
+ type Borrowed<'a> = &'a T;
+
+ fn into_foreign(self) -> *const core::ffi::c_void {
+ crate::alloc::Box::into_raw(self) as _
+ }
+
+ unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'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.
+ unsafe { &*ptr.cast() }
+ }
+
+ 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 { crate::alloc::Box::from_raw(ptr as _) }
+ }
+}
+
+impl<T: 'static, A> ForeignOwnable for Pin<crate::alloc::Box<T, A>>
+where
+ A: Allocator,
+{
+ type Borrowed<'a> = Pin<&'a T>;
+
+ fn into_foreign(self) -> *const core::ffi::c_void {
+ // SAFETY: We are still treating the box as pinned.
+ crate::alloc::Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) 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 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(crate::alloc::Box::from_raw(ptr as _)) }
+ }
+}
+
impl ForeignOwnable for () {
type Borrowed<'a> = ();
--
2.45.2
On 05.08.24 17:19, 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 | 330 ++++++++++++++++++++++++++++++++++++++
> rust/kernel/init.rs | 35 +++-
> rust/kernel/prelude.rs | 2 +-
> rust/kernel/types.rs | 56 +++++++
> 5 files changed, 427 insertions(+), 2 deletions(-)
> create mode 100644 rust/kernel/alloc/kbox.rs
>
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index 942e2755f217..d7beaf0372af 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..4a4379980745
> --- /dev/null
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -0,0 +1,330 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Implementation of [`Box`].
> +
> +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::result::Result;
> +
> +use crate::types::Unique;
> +
> +/// The kernel's [`Box`] type.
> +///
> +/// `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`.
> +///
> +/// For non-zero-sized values, a [`Box`] will use the given allocator `A` for its allocation. For
> +/// the most common allocators the type aliases `KBox`, `VBox` and `KVBox` exist.
> +///
> +/// It is valid to convert both ways between a [`Box`] and a raw pointer allocated with any
> +/// `Allocator`, given that the `Layout` used with the allocator is correct for the type.
> +///
> +/// For zero-sized values the [`Box`]' pointer must be `dangling_mut::<T>`; no memory is allocated.
Why do we need this to be in the docs?
> +///
> +/// So long as `T: Sized`, a `Box<T>` is guaranteed to be represented as a single pointer and is
> +/// also ABI-compatible with C pointers (i.e. the C type `T*`).
You did not make `Box` `repr(transparent)`, so this is not true.
Additionally, `Box<T>` (from stdlib) is not FFI-safe [1], this might be
surprising, given that it is ABI-compatible (and the documentation seems
to suggest that one *can* just use it across FFI). I think we should
generally avoid using `Box` in glue code between Rust and C. I would
remove this paragraph.
[1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/334
I think there are also other improvements, how about the following (feel
free to adapt it):
/// A heap allocation for a single value of type `T`.
///
/// When dropping a [`Box`], the value is also dropped and the heap memory is automatically freed.
///
/// This is the kernel's version of the Rust stdlib's `Box`. There are a couple, for example no
/// `noalias` attribute is emitted and partially moving out of a `Box` is not supported.
///
/// `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`]).
> +///
> +/// # Invariants
Please move this section below the examples section.
> +///
> +/// The [`Box`]' pointer always properly aligned and either points to memory allocated with `A` or,
"The [`Box`]' pointer" -> "`self.0` is"
> +/// for zero-sized types, is a dangling pointer.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?;
> +///
> +/// assert_eq!(*b, 24_u64);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
Do you think it would be a good idea to have an example that fails (ie
allocate with Kmalloc more than PAGE_SIZE)?
> +///
> +/// ```
> +/// struct Huge([u8; 1 << 24]);
I know that this is ~16MB, but are there test-vms with less memory (I
have no idea how much you normally run these at, I usually give my vms
plenty of ram, but when testing, people might not [my intuition is
telling me that 16MB should be fine, but I am not sure]).
> +///
> +/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok());
> +/// ```
> +pub struct Box<T: ?Sized, A: Allocator>(Unique<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>;
> +
> +impl<T, A> Box<T, A>
> +where
> + T: ?Sized,
> + A: Allocator,
> +{
> + /// Constructs a `Box<T, A>` from a raw pointer.
> + ///
> + /// # Safety
> + ///
> + /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
> + /// type `T`.
With this requirement and the invariant on `Box`, I am lead to believe
that you can't use this for ZSTs, since they are not allocated with `A`.
One solution would be to adjust this requirement. But I would rather use
a different solution: we move the dangling pointer stuff into the
allocator and also call it when `T` is a ZST (ie don't special case them
in `Box` but in the impls of `Allocator`). That way this can stay as-is
and the part about ZSTs in the invariant can be removed.
> + #[inline]
> + pub const unsafe fn from_raw(raw: *mut T) -> Self {
> + // SAFETY: Validity of `raw` is guaranteed by the safety preconditions of this function.
This is not a safety requirement of `Unique::new_unchecked`. Instead it
is a type invariant of `Box`, so it should be an INVARIANT comment.
You still need to justify `new_unchecked` though (which is requires that
`raw` is not NULL.
> + Self(unsafe { Unique::new_unchecked(raw) }, PhantomData::<A>)
You don't need the `::<A>`.
> + }
> +
> + /// Consumes the `Box<T>`, returning a wrapped raw pointer.
> + ///
Please add a new paragraph: "This will not run the destructor of `T` and
the allocation will stay alive indefinitely. Use [`Box::from_raw`] to
recover the [`Box`], drop the value and free the allocation.".
> + /// # 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>`, returning a mutable reference, &'a mut T.
The last part seems a bit weird, it should definitely be enclosed in
'`', but it also seems unnecessary. Instead I would stress that this
will never drop the value and also never free the allocation.
> + #[inline]
> + pub fn leak<'a>(b: Self) -> &'a mut T
> + where
> + T: 'a,
> + {
> + // 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) }
> + }
> +
> + /// Converts a `Box<T, A>` into a `Pin<Box<T, A>>`.
> + #[inline]
> + pub fn into_pin(b: Self) -> Pin<Self>
> + where
> + A: 'static,
Why do we require this? Our `Box` doesn't store an allocator.
> + {
> + // SAFETY: It's not possible to move or replace the insides of a `Pin<Box<T, A>>` when
> + // `T: !Unpin`, so it's safe to pin it directly without any additional requirements.
> + unsafe { Pin::new_unchecked(b) }
> + }
> +}
> +
> +impl<T, A> Box<MaybeUninit<T>, A>
> +where
> + A: Allocator,
> +{
> + /// Converts to `Box<T, A>`.
> + ///
> + /// # Safety
> + ///
> + /// As with MaybeUninit::assume_init, it is up to the caller to guarantee that the value really
> + /// is in an initialized state. Calling this when the content is not yet fully initialized
> + /// causes immediate undefined behavior.
This also looks like it was copied from the Rust stdlib, please see
Miguel's response as to what to do about that.
Additionally, this Safety section is not up to par with the rest of the
kernel, for me this sounds better:
/// The pointee must be a valid value of type `T`.
> + pub unsafe fn assume_init(b: Self) -> Box<T, A> {
> + let raw = Self::into_raw(b);
> + // SAFETY: Reconstruct the `Box<MaybeUninit<T>, A>` as Box<T, A> now that has been
> + // initialized. `raw` and `alloc` are safe by the invariants of `Box`.
> + 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
> + }
> +
> + /// Allocates memory with the allocator `A` and then places `x` into it.
> + ///
> + /// This doesn’t actually allocate if T is zero-sized.
> + pub fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
> + let b = Self::new_uninit(flags)?;
> + Ok(Box::write(b, x))
> + }
> +
> + /// Constructs a new `Box<T, A>` with uninitialized contents.
> + ///
> + /// # 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() {
> + Unique::dangling()
> + } else {
> + let layout = core::alloc::Layout::new::<MaybeUninit<T>>();
> + let ptr = A::alloc(layout, flags)?;
> +
> + ptr.cast().into()
> + };
> +
> + 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 unable to be moved.
> + #[inline]
> + pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
> + where
> + A: 'static,
Again, we don't need this.
> + {
> + Ok(Self::new(x, flags)?.into())
> + }
> +
> + /// 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 = Box::into_raw(this);
> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> + unsafe { core::ptr::drop_in_place(ptr) };
> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> + unsafe { Box::from_raw(ptr.cast()) }
> + }
I don't particularly care in this instance, but you just took my patch
and folded it into your own without asking me or specifying it in the
commit message. In general I would have assumed that you just put the
entire patch into the series (with correct From:... etc).
> +}
> +
> +impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
> +where
> + T: ?Sized,
> + A: Allocator,
> + A: 'static,
> +{
> + /// Converts a `Box<T>` into a `Pin<Box<T>>`. If `T` does not implement [`Unpin`], then
> + /// `*boxed` will be pinned in memory and unable to be moved.
> + ///
> + /// This conversion does not allocate on the heap and happens in place.
> + ///
> + /// This is also available via [`Box::into_pin`].
> + ///
> + /// Constructing and pinning a `Box` with <code><Pin<Box\<T>>>::from([Box::new]\(x))</code>
> + /// can also be written more concisely using <code>[Box::pin]\(x)</code>.
> + /// This `From` implementation is useful if you already have a `Box<T>`, or you are
> + /// constructing a (pinned) `Box` in a different way than with [`Box::new`].
This also looks very much like something from the stdlib...
> + fn from(b: Box<T, A>) -> Self {
> + Box::into_pin(b)
> + }
> +}
> +
> +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`.
If `T` is a ZST, then it is not dereferenceable.
> + 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 ptr = self.0.as_ptr();
> +
> + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
> + // instance of `T`.
> + let size = unsafe { core::mem::size_of_val(&*ptr) };
1. `size_of_val` is not `unsafe`.
2. why not use `&*self` instead of using the raw pointer? (then move the
let binding below this line)
> +
> + // SAFETY: We need to drop `self.0` in place, before we free the backing memory.
> + unsafe { core::ptr::drop_in_place(ptr) };
> +
> + if size != 0 {
Making zero-sized allocations possible with Allocators would also
simplify this.
> + // SAFETY: `ptr` was previously allocated with `A`.
> + unsafe { A::free(self.0.as_non_null().cast()) };
> + }
> + }
> +}
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index 495c09ebe3a3..5fd7a0ffabd2 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -211,7 +211,7 @@
> //! [`pin_init!`]: crate::pin_init!
>
> use crate::{
> - alloc::{box_ext::BoxExt, AllocError, Flags},
> + alloc::{box_ext::BoxExt, AllocError, Allocator, Flags},
> error::{self, Error},
> sync::UniqueArc,
> types::{Opaque, ScopeGuard},
> @@ -1178,6 +1178,39 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
> }
> }
>
> +impl<T, A> InPlaceInit<T> for crate::alloc::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>,
> + {
> + let mut this = crate::alloc::Box::<_, A>::new_uninit(flags)?;
> + let slot = this.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 { crate::alloc::Box::assume_init(this) }.into())
> + }
> +
> + #[inline]
> + fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
> + where
> + E: From<AllocError>,
> + {
> + let mut this = crate::alloc::Box::<_, A>::new_uninit(flags)?;
> + let slot = this.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 { crate::alloc::Box::assume_init(this) })
> + }
> +}
Please move this impl into kbox.rs, for the stdlib `Box`, this was here,
since we did not own that `Box`.
> +
> impl<T> InPlaceInit<T> for UniqueArc<T> {
> #[inline]
> fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index b37a0b3180fb..39f9331a48e2 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};
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 7cf89067b5fc..9fe87528d129 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -2,6 +2,7 @@
>
> //! Kernel types.
>
> +use crate::alloc::Allocator;
> use crate::init::{self, PinInit};
> use alloc::boxed::Box;
> use core::{
> @@ -9,6 +10,7 @@
> marker::{PhantomData, PhantomPinned},
> mem::MaybeUninit,
> ops::{Deref, DerefMut},
> + pin::Pin,
> ptr::NonNull,
> };
>
> @@ -89,6 +91,60 @@ unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> }
> }
>
> +impl<T: 'static, A> ForeignOwnable for crate::alloc::Box<T, A>
> +where
> + A: Allocator,
> +{
> + type Borrowed<'a> = &'a T;
> +
> + fn into_foreign(self) -> *const core::ffi::c_void {
> + crate::alloc::Box::into_raw(self) as _
> + }
> +
> + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'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.
> + unsafe { &*ptr.cast() }
> + }
> +
> + 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 { crate::alloc::Box::from_raw(ptr as _) }
> + }
> +}
> +
> +impl<T: 'static, A> ForeignOwnable for Pin<crate::alloc::Box<T, A>>
> +where
> + A: Allocator,
> +{
> + type Borrowed<'a> = Pin<&'a T>;
> +
> + fn into_foreign(self) -> *const core::ffi::c_void {
> + // SAFETY: We are still treating the box as pinned.
> + crate::alloc::Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) 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 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(crate::alloc::Box::from_raw(ptr as _)) }
> + }
> +}
Ditto for these two.
---
Cheers,
Benno
> +
> impl ForeignOwnable for () {
> type Borrowed<'a> = ();
>
> --
> 2.45.2
>
On Tue, Aug 06, 2024 at 07:47:17PM +0000, Benno Lossin wrote:
> On 05.08.24 17:19, 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 | 330 ++++++++++++++++++++++++++++++++++++++
> > rust/kernel/init.rs | 35 +++-
> > rust/kernel/prelude.rs | 2 +-
> > rust/kernel/types.rs | 56 +++++++
> > 5 files changed, 427 insertions(+), 2 deletions(-)
> > create mode 100644 rust/kernel/alloc/kbox.rs
> >
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index 942e2755f217..d7beaf0372af 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..4a4379980745
> > --- /dev/null
> > +++ b/rust/kernel/alloc/kbox.rs
> > @@ -0,0 +1,330 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Implementation of [`Box`].
> > +
> > +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::result::Result;
> > +
> > +use crate::types::Unique;
> > +
> > +/// The kernel's [`Box`] type.
> > +///
> > +/// `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`.
> > +///
> > +/// For non-zero-sized values, a [`Box`] will use the given allocator `A` for its allocation. For
> > +/// the most common allocators the type aliases `KBox`, `VBox` and `KVBox` exist.
> > +///
> > +/// It is valid to convert both ways between a [`Box`] and a raw pointer allocated with any
> > +/// `Allocator`, given that the `Layout` used with the allocator is correct for the type.
> > +///
> > +/// For zero-sized values the [`Box`]' pointer must be `dangling_mut::<T>`; no memory is allocated.
>
> Why do we need this to be in the docs?
Probably not - do you suggest to remove it entirely? Otherwise, where do you
think we should move it?
>
> > +///
> > +/// So long as `T: Sized`, a `Box<T>` is guaranteed to be represented as a single pointer and is
> > +/// also ABI-compatible with C pointers (i.e. the C type `T*`).
>
> You did not make `Box` `repr(transparent)`, so this is not true.
> Additionally, `Box<T>` (from stdlib) is not FFI-safe [1], this might be
> surprising, given that it is ABI-compatible (and the documentation seems
> to suggest that one *can* just use it across FFI). I think we should
> generally avoid using `Box` in glue code between Rust and C. I would
> remove this paragraph.
Agreed.
>
> [1]: https://github.com/rust-lang/unsafe-code-guidelines/issues/334
>
>
> I think there are also other improvements, how about the following (feel
> free to adapt it):
>
> /// A heap allocation for a single value of type `T`.
> ///
> /// When dropping a [`Box`], the value is also dropped and the heap memory is automatically freed.
> ///
> /// This is the kernel's version of the Rust stdlib's `Box`. There are a couple, for example no
> /// `noalias` attribute is emitted and partially moving out of a `Box` is not supported.
> ///
> /// `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`]).
That sounds good, I will take this with a few minor adjustments, thanks.
>
> > +///
> > +/// # Invariants
>
> Please move this section below the examples section.
>
> > +///
> > +/// The [`Box`]' pointer always properly aligned and either points to memory allocated with `A` or,
>
> "The [`Box`]' pointer" -> "`self.0` is"
>
> > +/// for zero-sized types, is a dangling pointer.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?;
> > +///
> > +/// assert_eq!(*b, 24_u64);
> > +///
> > +/// # Ok::<(), Error>(())
> > +/// ```
>
> Do you think it would be a good idea to have an example that fails (ie
> allocate with Kmalloc more than PAGE_SIZE)?
I think that's a good idea, I'll add one.
Please note that kmalloc() can allocate larger buffers than PAGE_SIZE. We can
request something larger than KMALLOC_MAX_SIZE though.
>
> > +///
> > +/// ```
> > +/// struct Huge([u8; 1 << 24]);
>
> I know that this is ~16MB, but are there test-vms with less memory (I
> have no idea how much you normally run these at, I usually give my vms
> plenty of ram, but when testing, people might not [my intuition is
> telling me that 16MB should be fine, but I am not sure]).
I think it's pretty reasonable to ask for 16MiB for a test case to succeed.
>
> > +///
> > +/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok());
> > +/// ```
> > +pub struct Box<T: ?Sized, A: Allocator>(Unique<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>;
> > +
> > +impl<T, A> Box<T, A>
> > +where
> > + T: ?Sized,
> > + A: Allocator,
> > +{
> > + /// Constructs a `Box<T, A>` from a raw pointer.
> > + ///
> > + /// # Safety
> > + ///
> > + /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
> > + /// type `T`.
>
> With this requirement and the invariant on `Box`, I am lead to believe
> that you can't use this for ZSTs, since they are not allocated with `A`.
> One solution would be to adjust this requirement. But I would rather use
> a different solution: we move the dangling pointer stuff into the
> allocator and also call it when `T` is a ZST (ie don't special case them
> in `Box` but in the impls of `Allocator`). That way this can stay as-is
> and the part about ZSTs in the invariant can be removed.
Actually, we already got that. Every zero sized allocation will return a
dangling pointer. However, we can't call `Allocator::free` with (any) dangling
pointer though.
>
> > + #[inline]
> > + pub const unsafe fn from_raw(raw: *mut T) -> Self {
> > + // SAFETY: Validity of `raw` is guaranteed by the safety preconditions of this function.
>
> This is not a safety requirement of `Unique::new_unchecked`. Instead it
> is a type invariant of `Box`, so it should be an INVARIANT comment.
> You still need to justify `new_unchecked` though (which is requires that
> `raw` is not NULL.
Agreed.
>
> > + Self(unsafe { Unique::new_unchecked(raw) }, PhantomData::<A>)
>
> You don't need the `::<A>`.
>
> > + }
> > +
> > + /// Consumes the `Box<T>`, returning a wrapped raw pointer.
> > + ///
>
> Please add a new paragraph: "This will not run the destructor of `T` and
> the allocation will stay alive indefinitely. Use [`Box::from_raw`] to
> recover the [`Box`], drop the value and free the allocation.".
>
> > + /// # 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>`, returning a mutable reference, &'a mut T.
>
> The last part seems a bit weird, it should definitely be enclosed in
> '`', but it also seems unnecessary. Instead I would stress that this
> will never drop the value and also never free the allocation.
Agreed, for this and the above.
>
> > + #[inline]
> > + pub fn leak<'a>(b: Self) -> &'a mut T
> > + where
> > + T: 'a,
> > + {
> > + // 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) }
> > + }
> > +
> > + /// Converts a `Box<T, A>` into a `Pin<Box<T, A>>`.
> > + #[inline]
> > + pub fn into_pin(b: Self) -> Pin<Self>
> > + where
> > + A: 'static,
>
> Why do we require this? Our `Box` doesn't store an allocator.
I just forgot to remove it.
>
> > + {
> > + // SAFETY: It's not possible to move or replace the insides of a `Pin<Box<T, A>>` when
> > + // `T: !Unpin`, so it's safe to pin it directly without any additional requirements.
> > + unsafe { Pin::new_unchecked(b) }
> > + }
> > +}
> > +
> > +impl<T, A> Box<MaybeUninit<T>, A>
> > +where
> > + A: Allocator,
> > +{
> > + /// Converts to `Box<T, A>`.
> > + ///
> > + /// # Safety
> > + ///
> > + /// As with MaybeUninit::assume_init, it is up to the caller to guarantee that the value really
> > + /// is in an initialized state. Calling this when the content is not yet fully initialized
> > + /// causes immediate undefined behavior.
>
> This also looks like it was copied from the Rust stdlib, please see
> Miguel's response as to what to do about that.
Yeah, I think there are a few places more places where I forgot about that, will
fix all of them.
>
> Additionally, this Safety section is not up to par with the rest of the
> kernel, for me this sounds better:
>
> /// The pointee must be a valid value of type `T`.
>
> > + pub unsafe fn assume_init(b: Self) -> Box<T, A> {
> > + let raw = Self::into_raw(b);
> > + // SAFETY: Reconstruct the `Box<MaybeUninit<T>, A>` as Box<T, A> now that has been
> > + // initialized. `raw` and `alloc` are safe by the invariants of `Box`.
> > + 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
> > + }
> > +
> > + /// Allocates memory with the allocator `A` and then places `x` into it.
> > + ///
> > + /// This doesn’t actually allocate if T is zero-sized.
> > + pub fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
> > + let b = Self::new_uninit(flags)?;
> > + Ok(Box::write(b, x))
> > + }
> > +
> > + /// Constructs a new `Box<T, A>` with uninitialized contents.
> > + ///
> > + /// # 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() {
> > + Unique::dangling()
> > + } else {
> > + let layout = core::alloc::Layout::new::<MaybeUninit<T>>();
> > + let ptr = A::alloc(layout, flags)?;
> > +
> > + ptr.cast().into()
> > + };
> > +
> > + 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 unable to be moved.
> > + #[inline]
> > + pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
> > + where
> > + A: 'static,
>
> Again, we don't need this.
>
> > + {
> > + Ok(Self::new(x, flags)?.into())
> > + }
> > +
> > + /// 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 = Box::into_raw(this);
> > + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> > + unsafe { core::ptr::drop_in_place(ptr) };
> > + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> > + unsafe { Box::from_raw(ptr.cast()) }
> > + }
>
> I don't particularly care in this instance, but you just took my patch
> and folded it into your own without asking me or specifying it in the
> commit message. In general I would have assumed that you just put the
> entire patch into the series (with correct From:... etc).
When I asked about this in [1] my understanding was that we expect [1] to land
prior to this series. So, I'm just anticipating a future rebase where I move
this code from box_ext.rs to kbox.rs, just like Alice suggested for her
"ForeignOwnable for Pin<crate::alloc::Box<T, A>>" implementation.
I also understand your later reply, where you said: "[...] then you can just
include it when you remove the `BoxExit` trait." as confirmation.
Probably that's a misunderstanding though. Sorry if that's the case.
[1] https://lore.kernel.org/lkml/24a8d381-dd13-4d19-a736-689b8880dbe1@proton.me/
>
> > +}
> > +
> > +impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
> > +where
> > + T: ?Sized,
> > + A: Allocator,
> > + A: 'static,
> > +{
> > + /// Converts a `Box<T>` into a `Pin<Box<T>>`. If `T` does not implement [`Unpin`], then
> > + /// `*boxed` will be pinned in memory and unable to be moved.
> > + ///
> > + /// This conversion does not allocate on the heap and happens in place.
> > + ///
> > + /// This is also available via [`Box::into_pin`].
> > + ///
> > + /// Constructing and pinning a `Box` with <code><Pin<Box\<T>>>::from([Box::new]\(x))</code>
> > + /// can also be written more concisely using <code>[Box::pin]\(x)</code>.
> > + /// This `From` implementation is useful if you already have a `Box<T>`, or you are
> > + /// constructing a (pinned) `Box` in a different way than with [`Box::new`].
>
> This also looks very much like something from the stdlib...
Yeah, I'll replace that.
>
> > + fn from(b: Box<T, A>) -> Self {
> > + Box::into_pin(b)
> > + }
> > +}
> > +
> > +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`.
>
> If `T` is a ZST, then it is not dereferenceable.
Why not? If `T` is a ZST `self.0` is `Unique::<T>::dangling()`. So, in the end
this is the same as `NonNull::<T>::dangling().as_ref()`.
>
> > + 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 ptr = self.0.as_ptr();
> > +
> > + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
> > + // instance of `T`.
> > + let size = unsafe { core::mem::size_of_val(&*ptr) };
>
> 1. `size_of_val` is not `unsafe`.
Right, but dereferencing the `ptr` is unsafe.
> 2. why not use `&*self` instead of using the raw pointer? (then move the
> let binding below this line)
If we ever support non-ZST `Allocator`s using `self` would not always evaluate
to the correct size. I think evaluating the size of `T` rather than `Box<T>` is
the correct thing to do.
>
> > +
> > + // SAFETY: We need to drop `self.0` in place, before we free the backing memory.
> > + unsafe { core::ptr::drop_in_place(ptr) };
> > +
> > + if size != 0 {
>
> Making zero-sized allocations possible with Allocators would also
> simplify this.
As mentioned, it's possible already. But we still can't pass any dangling
pointer to `free`.
>
> > + // SAFETY: `ptr` was previously allocated with `A`.
> > + unsafe { A::free(self.0.as_non_null().cast()) };
> > + }
> > + }
> > +}
> > diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> > index 495c09ebe3a3..5fd7a0ffabd2 100644
> > --- a/rust/kernel/init.rs
> > +++ b/rust/kernel/init.rs
> > @@ -211,7 +211,7 @@
> > //! [`pin_init!`]: crate::pin_init!
> >
> > use crate::{
> > - alloc::{box_ext::BoxExt, AllocError, Flags},
> > + alloc::{box_ext::BoxExt, AllocError, Allocator, Flags},
> > error::{self, Error},
> > sync::UniqueArc,
> > types::{Opaque, ScopeGuard},
> > @@ -1178,6 +1178,39 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
> > }
> > }
> >
> > +impl<T, A> InPlaceInit<T> for crate::alloc::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>,
> > + {
> > + let mut this = crate::alloc::Box::<_, A>::new_uninit(flags)?;
> > + let slot = this.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 { crate::alloc::Box::assume_init(this) }.into())
> > + }
> > +
> > + #[inline]
> > + fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
> > + where
> > + E: From<AllocError>,
> > + {
> > + let mut this = crate::alloc::Box::<_, A>::new_uninit(flags)?;
> > + let slot = this.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 { crate::alloc::Box::assume_init(this) })
> > + }
> > +}
>
> Please move this impl into kbox.rs, for the stdlib `Box`, this was here,
> since we did not own that `Box`.
>
> > +
> > impl<T> InPlaceInit<T> for UniqueArc<T> {
> > #[inline]
> > fn try_pin_init<E>(init: impl PinInit<T, E>, flags: Flags) -> Result<Pin<Self>, E>
> > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> > index b37a0b3180fb..39f9331a48e2 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};
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index 7cf89067b5fc..9fe87528d129 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -2,6 +2,7 @@
> >
> > //! Kernel types.
> >
> > +use crate::alloc::Allocator;
> > use crate::init::{self, PinInit};
> > use alloc::boxed::Box;
> > use core::{
> > @@ -9,6 +10,7 @@
> > marker::{PhantomData, PhantomPinned},
> > mem::MaybeUninit,
> > ops::{Deref, DerefMut},
> > + pin::Pin,
> > ptr::NonNull,
> > };
> >
> > @@ -89,6 +91,60 @@ unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> > }
> > }
> >
> > +impl<T: 'static, A> ForeignOwnable for crate::alloc::Box<T, A>
> > +where
> > + A: Allocator,
> > +{
> > + type Borrowed<'a> = &'a T;
> > +
> > + fn into_foreign(self) -> *const core::ffi::c_void {
> > + crate::alloc::Box::into_raw(self) as _
> > + }
> > +
> > + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'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.
> > + unsafe { &*ptr.cast() }
> > + }
> > +
> > + 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 { crate::alloc::Box::from_raw(ptr as _) }
> > + }
> > +}
> > +
> > +impl<T: 'static, A> ForeignOwnable for Pin<crate::alloc::Box<T, A>>
> > +where
> > + A: Allocator,
> > +{
> > + type Borrowed<'a> = Pin<&'a T>;
> > +
> > + fn into_foreign(self) -> *const core::ffi::c_void {
> > + // SAFETY: We are still treating the box as pinned.
> > + crate::alloc::Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }) 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 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(crate::alloc::Box::from_raw(ptr as _)) }
> > + }
> > +}
>
> Ditto for these two.
Agreed, will do.
>
> ---
> Cheers,
> Benno
>
> > +
> > impl ForeignOwnable for () {
> > type Borrowed<'a> = ();
> >
> > --
> > 2.45.2
> >
>
On 07.08.24 01:01, Danilo Krummrich wrote:
> On Tue, Aug 06, 2024 at 07:47:17PM +0000, Benno Lossin wrote:
>> On 05.08.24 17:19, 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 | 330 ++++++++++++++++++++++++++++++++++++++
>>> rust/kernel/init.rs | 35 +++-
>>> rust/kernel/prelude.rs | 2 +-
>>> rust/kernel/types.rs | 56 +++++++
>>> 5 files changed, 427 insertions(+), 2 deletions(-)
>>> create mode 100644 rust/kernel/alloc/kbox.rs
>>>
>>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
>>> index 942e2755f217..d7beaf0372af 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..4a4379980745
>>> --- /dev/null
>>> +++ b/rust/kernel/alloc/kbox.rs
>>> @@ -0,0 +1,330 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Implementation of [`Box`].
>>> +
>>> +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::result::Result;
>>> +
>>> +use crate::types::Unique;
>>> +
>>> +/// The kernel's [`Box`] type.
>>> +///
>>> +/// `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`.
>>> +///
>>> +/// For non-zero-sized values, a [`Box`] will use the given allocator `A` for its allocation. For
>>> +/// the most common allocators the type aliases `KBox`, `VBox` and `KVBox` exist.
>>> +///
>>> +/// It is valid to convert both ways between a [`Box`] and a raw pointer allocated with any
>>> +/// `Allocator`, given that the `Layout` used with the allocator is correct for the type.
>>> +///
>>> +/// For zero-sized values the [`Box`]' pointer must be `dangling_mut::<T>`; no memory is allocated.
>>
>> Why do we need this to be in the docs?
>
> Probably not - do you suggest to remove it entirely? Otherwise, where do you
> think we should move it?
I would remove it, since it's just implementation detail and
allocator-dependent.
>>> +impl<T, A> Box<T, A>
>>> +where
>>> + T: ?Sized,
>>> + A: Allocator,
>>> +{
>>> + /// Constructs a `Box<T, A>` from a raw pointer.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
>>> + /// type `T`.
>>
>> With this requirement and the invariant on `Box`, I am lead to believe
>> that you can't use this for ZSTs, since they are not allocated with `A`.
>> One solution would be to adjust this requirement. But I would rather use
>> a different solution: we move the dangling pointer stuff into the
>> allocator and also call it when `T` is a ZST (ie don't special case them
>> in `Box` but in the impls of `Allocator`). That way this can stay as-is
>> and the part about ZSTs in the invariant can be removed.
>
> Actually, we already got that. Every zero sized allocation will return a
> dangling pointer. However, we can't call `Allocator::free` with (any) dangling
> pointer though.
The last part is rather problematic in my opinion, since the safety
requirements of the functions in `Allocator` don't ensure that you're
not allowed to do it. We should make it possible to free dangling
pointers that were previously "allocated" by the allocator (ie returned
by `realloc`).
Maybe we do need an `old_layout` parameter for that (that way we can
also `debug_assert_eq!(old_layout.align(), new_layout.align())`).
>>> + {
>>> + Ok(Self::new(x, flags)?.into())
>>> + }
>>> +
>>> + /// 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 = Box::into_raw(this);
>>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>> + unsafe { core::ptr::drop_in_place(ptr) };
>>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>> + unsafe { Box::from_raw(ptr.cast()) }
>>> + }
>>
>> I don't particularly care in this instance, but you just took my patch
>> and folded it into your own without asking me or specifying it in the
>> commit message. In general I would have assumed that you just put the
>> entire patch into the series (with correct From:... etc).
>
> When I asked about this in [1] my understanding was that we expect [1] to land
> prior to this series. So, I'm just anticipating a future rebase where I move
> this code from box_ext.rs to kbox.rs, just like Alice suggested for her
> "ForeignOwnable for Pin<crate::alloc::Box<T, A>>" implementation.
>
> I also understand your later reply, where you said: "[...] then you can just
> include it when you remove the `BoxExit` trait." as confirmation.
>
> Probably that's a misunderstanding though. Sorry if that's the case.
Yeah what I meant by that was you base it on top and then move it from
the `BoxExt` trait over to `Box` in a correctly attributed patch. As I
said above, I don't really mind in this case, since it's trivial, so no
worries. Just a heads-up for occasions where it is non-trivial.
> [1] https://lore.kernel.org/lkml/24a8d381-dd13-4d19-a736-689b8880dbe1@proton.me/
>
>>
>>> +}
>>> +
>>> +impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
>>> +where
>>> + T: ?Sized,
>>> + A: Allocator,
>>> + A: 'static,
>>> +{
>>> + /// Converts a `Box<T>` into a `Pin<Box<T>>`. If `T` does not implement [`Unpin`], then
>>> + /// `*boxed` will be pinned in memory and unable to be moved.
>>> + ///
>>> + /// This conversion does not allocate on the heap and happens in place.
>>> + ///
>>> + /// This is also available via [`Box::into_pin`].
>>> + ///
>>> + /// Constructing and pinning a `Box` with <code><Pin<Box\<T>>>::from([Box::new]\(x))</code>
>>> + /// can also be written more concisely using <code>[Box::pin]\(x)</code>.
>>> + /// This `From` implementation is useful if you already have a `Box<T>`, or you are
>>> + /// constructing a (pinned) `Box` in a different way than with [`Box::new`].
>>
>> This also looks very much like something from the stdlib...
>
> Yeah, I'll replace that.
>
>>
>>> + fn from(b: Box<T, A>) -> Self {
>>> + Box::into_pin(b)
>>> + }
>>> +}
>>> +
>>> +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`.
>>
>> If `T` is a ZST, then it is not dereferenceable.
>
> Why not? If `T` is a ZST `self.0` is `Unique::<T>::dangling()`. So, in the end
> this is the same as `NonNull::<T>::dangling().as_ref()`.
You are right, I just looked at [1] again and they define
dereferenceable as "the memory range of the given size starting at the
pointer must all be within the bounds of a single allocated object", for
a zero-sized allocation, this holds vacuously.
[1]: https://doc.rust-lang.org/core/ptr/index.html#safety
>>> + 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 ptr = self.0.as_ptr();
>>> +
>>> + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
>>> + // instance of `T`.
>>> + let size = unsafe { core::mem::size_of_val(&*ptr) };
>>
>> 1. `size_of_val` is not `unsafe`.
>
> Right, but dereferencing the `ptr` is unsafe.
>
>> 2. why not use `&*self` instead of using the raw pointer? (then move the
>> let binding below this line)
>
> If we ever support non-ZST `Allocator`s using `self` would not always evaluate
> to the correct size. I think evaluating the size of `T` rather than `Box<T>` is
> the correct thing to do.
I mean use `Box::deref` (that's what `&*self` should do), you don't need
to repeat the same SAFETY comment when it already is wrapped by a safe
function.
---
Cheers,
Benno
On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote:
> >>> +impl<T, A> Drop for Box<T, A>
> >>> +where
> >>> + T: ?Sized,
> >>> + A: Allocator,
> >>> +{
> >>> + fn drop(&mut self) {
> >>> + let ptr = self.0.as_ptr();
> >>> +
> >>> + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
> >>> + // instance of `T`.
> >>> + let size = unsafe { core::mem::size_of_val(&*ptr) };
> >>
> >> 1. `size_of_val` is not `unsafe`.
> >
> > Right, but dereferencing the `ptr` is unsafe.
> >
> >> 2. why not use `&*self` instead of using the raw pointer? (then move the
> >> let binding below this line)
> >
> > If we ever support non-ZST `Allocator`s using `self` would not always evaluate
> > to the correct size. I think evaluating the size of `T` rather than `Box<T>` is
> > the correct thing to do.
>
> I mean use `Box::deref` (that's what `&*self` should do), you don't need
Actually, this must either be `size_of_val(&**self)` or `size_of_val::<T>(self).
`size_of_val(&*self)` should indeed resolve to `&Box<T, A>`, right?
> to repeat the same SAFETY comment when it already is wrapped by a safe
> function.
>
> ---
> Cheers,
> Benno
>
On 08.08.24 19:44, Danilo Krummrich wrote:
> On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote:
>>>>> +impl<T, A> Drop for Box<T, A>
>>>>> +where
>>>>> + T: ?Sized,
>>>>> + A: Allocator,
>>>>> +{
>>>>> + fn drop(&mut self) {
>>>>> + let ptr = self.0.as_ptr();
>>>>> +
>>>>> + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
>>>>> + // instance of `T`.
>>>>> + let size = unsafe { core::mem::size_of_val(&*ptr) };
>>>>
>>>> 1. `size_of_val` is not `unsafe`.
>>>
>>> Right, but dereferencing the `ptr` is unsafe.
>>>
>>>> 2. why not use `&*self` instead of using the raw pointer? (then move the
>>>> let binding below this line)
>>>
>>> If we ever support non-ZST `Allocator`s using `self` would not always evaluate
>>> to the correct size. I think evaluating the size of `T` rather than `Box<T>` is
>>> the correct thing to do.
>>
>> I mean use `Box::deref` (that's what `&*self` should do), you don't need
>
> Actually, this must either be `size_of_val(&**self)` or `size_of_val::<T>(self).
>
> `size_of_val(&*self)` should indeed resolve to `&Box<T, A>`, right?
Oh yeah that is true, good catch! Here is a playground example [1].
[1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c439494a0582bb287232c7a05e21bd23
---
Cheers,
Benno
>
>> to repeat the same SAFETY comment when it already is wrapped by a safe
>> function.
>>
>> ---
>> Cheers,
>> Benno
>>
On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote:
> >> With this requirement and the invariant on `Box`, I am lead to believe
> >> that you can't use this for ZSTs, since they are not allocated with `A`.
> >> One solution would be to adjust this requirement. But I would rather use
> >> a different solution: we move the dangling pointer stuff into the
> >> allocator and also call it when `T` is a ZST (ie don't special case them
> >> in `Box` but in the impls of `Allocator`). That way this can stay as-is
> >> and the part about ZSTs in the invariant can be removed.
> >
> > Actually, we already got that. Every zero sized allocation will return a
> > dangling pointer. However, we can't call `Allocator::free` with (any) dangling
> > pointer though.
>
> The last part is rather problematic in my opinion, since the safety
> requirements of the functions in `Allocator` don't ensure that you're
> not allowed to do it.
Yes, I think it needs to be added.
> We should make it possible to free dangling
> pointers that were previously "allocated" by the allocator (ie returned
> by `realloc`).
> Maybe we do need an `old_layout` parameter for that (that way we can
> also `debug_assert_eq!(old_layout.align(), new_layout.align())`).
Please see my reply in [1] - let's continue the discussion there.
[1] https://lore.kernel.org/rust-for-linux/ZrNIaAcGkGU0d8I3@pollux/
>
> >>> + {
> >>> + Ok(Self::new(x, flags)?.into())
> >>> + }
> >>> +
> >>> + /// 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 = Box::into_raw(this);
> >>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> >>> + unsafe { core::ptr::drop_in_place(ptr) };
> >>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
> >>> + unsafe { Box::from_raw(ptr.cast()) }
> >>> + }
> >>
> >> I don't particularly care in this instance, but you just took my patch
> >> and folded it into your own without asking me or specifying it in the
> >> commit message. In general I would have assumed that you just put the
> >> entire patch into the series (with correct From:... etc).
> >
> > When I asked about this in [1] my understanding was that we expect [1] to land
> > prior to this series. So, I'm just anticipating a future rebase where I move
> > this code from box_ext.rs to kbox.rs, just like Alice suggested for her
> > "ForeignOwnable for Pin<crate::alloc::Box<T, A>>" implementation.
> >
> > I also understand your later reply, where you said: "[...] then you can just
> > include it when you remove the `BoxExit` trait." as confirmation.
> >
> > Probably that's a misunderstanding though. Sorry if that's the case.
>
> Yeah what I meant by that was you base it on top and then move it from
> the `BoxExt` trait over to `Box` in a correctly attributed patch.
I don't understand this. What do you mean with "correctly attributed patch" in
this case?
There are various existing implementations around `Box` and `BoxExt`, are you
saying that I should create separate patches for moving / adapting all of them,
e.g. this patch adapts parts from `BoxExt`, the implementation of
`ForeignOwnable` for `Box<T>`, the implementation of `InPlaceInit<T>` for
`Box<T>`.
I don't think this is necessary.
I probably shouldn't anticipate a future rebase though, it just leads to
confusion. I'll drop it for now and re-add it once your patch lands in rust-next.
> As I
> said above, I don't really mind in this case, since it's trivial, so no
> worries. Just a heads-up for occasions where it is non-trivial.
>
> > [1] https://lore.kernel.org/lkml/24a8d381-dd13-4d19-a736-689b8880dbe1@proton.me/
> >
> >>
> >>> +}
> >>> +
> >>> +impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
> >>> +where
> >>> + T: ?Sized,
> >>> + A: Allocator,
> >>> + A: 'static,
> >>> +{
> >>> + /// Converts a `Box<T>` into a `Pin<Box<T>>`. If `T` does not implement [`Unpin`], then
> >>> + /// `*boxed` will be pinned in memory and unable to be moved.
> >>> + ///
> >>> + /// This conversion does not allocate on the heap and happens in place.
> >>> + ///
> >>> + /// This is also available via [`Box::into_pin`].
> >>> + ///
> >>> + /// Constructing and pinning a `Box` with <code><Pin<Box\<T>>>::from([Box::new]\(x))</code>
> >>> + /// can also be written more concisely using <code>[Box::pin]\(x)</code>.
> >>> + /// This `From` implementation is useful if you already have a `Box<T>`, or you are
> >>> + /// constructing a (pinned) `Box` in a different way than with [`Box::new`].
> >>
> >> This also looks very much like something from the stdlib...
> >
> > Yeah, I'll replace that.
> >
> >>
> >>> + fn from(b: Box<T, A>) -> Self {
> >>> + Box::into_pin(b)
> >>> + }
> >>> +}
> >>> +
> >>> +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`.
> >>
> >> If `T` is a ZST, then it is not dereferenceable.
> >
> > Why not? If `T` is a ZST `self.0` is `Unique::<T>::dangling()`. So, in the end
> > this is the same as `NonNull::<T>::dangling().as_ref()`.
>
> You are right, I just looked at [1] again and they define
> dereferenceable as "the memory range of the given size starting at the
> pointer must all be within the bounds of a single allocated object", for
> a zero-sized allocation, this holds vacuously.
>
> [1]: https://doc.rust-lang.org/core/ptr/index.html#safety
>
> >>> + 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 ptr = self.0.as_ptr();
> >>> +
> >>> + // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
> >>> + // instance of `T`.
> >>> + let size = unsafe { core::mem::size_of_val(&*ptr) };
> >>
> >> 1. `size_of_val` is not `unsafe`.
> >
> > Right, but dereferencing the `ptr` is unsafe.
> >
> >> 2. why not use `&*self` instead of using the raw pointer? (then move the
> >> let binding below this line)
> >
> > If we ever support non-ZST `Allocator`s using `self` would not always evaluate
> > to the correct size. I think evaluating the size of `T` rather than `Box<T>` is
> > the correct thing to do.
>
> I mean use `Box::deref` (that's what `&*self` should do), you don't need
> to repeat the same SAFETY comment when it already is wrapped by a safe
> function.
Oh, yes, that's indeed a good suggestion.
>
> ---
> Cheers,
> Benno
>
On 07.08.24 12:38, Danilo Krummrich wrote:
> On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote:
>>>>> + {
>>>>> + Ok(Self::new(x, flags)?.into())
>>>>> + }
>>>>> +
>>>>> + /// 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 = Box::into_raw(this);
>>>>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>>>> + unsafe { core::ptr::drop_in_place(ptr) };
>>>>> + // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>>>> + unsafe { Box::from_raw(ptr.cast()) }
>>>>> + }
>>>>
>>>> I don't particularly care in this instance, but you just took my patch
>>>> and folded it into your own without asking me or specifying it in the
>>>> commit message. In general I would have assumed that you just put the
>>>> entire patch into the series (with correct From:... etc).
>>>
>>> When I asked about this in [1] my understanding was that we expect [1] to land
>>> prior to this series. So, I'm just anticipating a future rebase where I move
>>> this code from box_ext.rs to kbox.rs, just like Alice suggested for her
>>> "ForeignOwnable for Pin<crate::alloc::Box<T, A>>" implementation.
>>>
>>> I also understand your later reply, where you said: "[...] then you can just
>>> include it when you remove the `BoxExit` trait." as confirmation.
>>>
>>> Probably that's a misunderstanding though. Sorry if that's the case.
>>
>> Yeah what I meant by that was you base it on top and then move it from
>> the `BoxExt` trait over to `Box` in a correctly attributed patch.
>
> I don't understand this. What do you mean with "correctly attributed patch" in
> this case?
So, seems like I mixed the two approaches I thought of. Here are they
separated:
1. base the series on top of my patches and then copy the implementation
from the in-tree file this patch.
2. create a new patch that adds `drop_contents` to your Box (ie after
this patch) that has `Signed-off-by: you` and `Co-Developed-by: me`.
With this approach I would also ask privately (including the patch)
if that would be ok.
---
Cheers,
Benno
> There are various existing implementations around `Box` and `BoxExt`, are you
> saying that I should create separate patches for moving / adapting all of them,
> e.g. this patch adapts parts from `BoxExt`, the implementation of
> `ForeignOwnable` for `Box<T>`, the implementation of `InPlaceInit<T>` for
> `Box<T>`.
>
> I don't think this is necessary.
>
> I probably shouldn't anticipate a future rebase though, it just leads to
> confusion. I'll drop it for now and re-add it once your patch lands in rust-next.
>
>> As I
>> said above, I don't really mind in this case, since it's trivial, so no
>> worries. Just a heads-up for occasions where it is non-trivial.
>>
>>> [1] https://lore.kernel.org/lkml/24a8d381-dd13-4d19-a736-689b8880dbe1@proton.me/
On Wed, Aug 7, 2024 at 9:49 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 07.08.24 01:01, Danilo Krummrich wrote:
> > On Tue, Aug 06, 2024 at 07:47:17PM +0000, Benno Lossin wrote:
> >> On 05.08.24 17:19, 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 | 330 ++++++++++++++++++++++++++++++++++++++
> >>> rust/kernel/init.rs | 35 +++-
> >>> rust/kernel/prelude.rs | 2 +-
> >>> rust/kernel/types.rs | 56 +++++++
> >>> 5 files changed, 427 insertions(+), 2 deletions(-)
> >>> create mode 100644 rust/kernel/alloc/kbox.rs
> >>>
> >>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> >>> index 942e2755f217..d7beaf0372af 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..4a4379980745
> >>> --- /dev/null
> >>> +++ b/rust/kernel/alloc/kbox.rs
> >>> @@ -0,0 +1,330 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +//! Implementation of [`Box`].
> >>> +
> >>> +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::result::Result;
> >>> +
> >>> +use crate::types::Unique;
> >>> +
> >>> +/// The kernel's [`Box`] type.
> >>> +///
> >>> +/// `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`.
> >>> +///
> >>> +/// For non-zero-sized values, a [`Box`] will use the given allocator `A` for its allocation. For
> >>> +/// the most common allocators the type aliases `KBox`, `VBox` and `KVBox` exist.
> >>> +///
> >>> +/// It is valid to convert both ways between a [`Box`] and a raw pointer allocated with any
> >>> +/// `Allocator`, given that the `Layout` used with the allocator is correct for the type.
> >>> +///
> >>> +/// For zero-sized values the [`Box`]' pointer must be `dangling_mut::<T>`; no memory is allocated.
> >>
> >> Why do we need this to be in the docs?
> >
> > Probably not - do you suggest to remove it entirely? Otherwise, where do you
> > think we should move it?
>
> I would remove it, since it's just implementation detail and
> allocator-dependent.
>
> >>> +impl<T, A> Box<T, A>
> >>> +where
> >>> + T: ?Sized,
> >>> + A: Allocator,
> >>> +{
> >>> + /// Constructs a `Box<T, A>` from a raw pointer.
> >>> + ///
> >>> + /// # Safety
> >>> + ///
> >>> + /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
> >>> + /// type `T`.
> >>
> >> With this requirement and the invariant on `Box`, I am lead to believe
> >> that you can't use this for ZSTs, since they are not allocated with `A`.
> >> One solution would be to adjust this requirement. But I would rather use
> >> a different solution: we move the dangling pointer stuff into the
> >> allocator and also call it when `T` is a ZST (ie don't special case them
> >> in `Box` but in the impls of `Allocator`). That way this can stay as-is
> >> and the part about ZSTs in the invariant can be removed.
> >
> > Actually, we already got that. Every zero sized allocation will return a
> > dangling pointer. However, we can't call `Allocator::free` with (any) dangling
> > pointer though.
>
> The last part is rather problematic in my opinion, since the safety
> requirements of the functions in `Allocator` don't ensure that you're
> not allowed to do it. We should make it possible to free dangling
> pointers that were previously "allocated" by the allocator (ie returned
> by `realloc`).
> Maybe we do need an `old_layout` parameter for that (that way we can
> also `debug_assert_eq!(old_layout.align(), new_layout.align())`).
The std allocators generally prohibit zero sized allocations, so it
seems sensible for us to do the same?
Alice
On 07.08.24 09:51, Alice Ryhl wrote:
> On Wed, Aug 7, 2024 at 9:49 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> On 07.08.24 01:01, Danilo Krummrich wrote:
>>> On Tue, Aug 06, 2024 at 07:47:17PM +0000, Benno Lossin wrote:
>>>> On 05.08.24 17:19, Danilo Krummrich wrote:
>>>>> +impl<T, A> Box<T, A>
>>>>> +where
>>>>> + T: ?Sized,
>>>>> + A: Allocator,
>>>>> +{
>>>>> + /// Constructs a `Box<T, A>` from a raw pointer.
>>>>> + ///
>>>>> + /// # Safety
>>>>> + ///
>>>>> + /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
>>>>> + /// type `T`.
>>>>
>>>> With this requirement and the invariant on `Box`, I am lead to believe
>>>> that you can't use this for ZSTs, since they are not allocated with `A`.
>>>> One solution would be to adjust this requirement. But I would rather use
>>>> a different solution: we move the dangling pointer stuff into the
>>>> allocator and also call it when `T` is a ZST (ie don't special case them
>>>> in `Box` but in the impls of `Allocator`). That way this can stay as-is
>>>> and the part about ZSTs in the invariant can be removed.
>>>
>>> Actually, we already got that. Every zero sized allocation will return a
>>> dangling pointer. However, we can't call `Allocator::free` with (any) dangling
>>> pointer though.
>>
>> The last part is rather problematic in my opinion, since the safety
>> requirements of the functions in `Allocator` don't ensure that you're
>> not allowed to do it. We should make it possible to free dangling
>> pointers that were previously "allocated" by the allocator (ie returned
>> by `realloc`).
>> Maybe we do need an `old_layout` parameter for that (that way we can
>> also `debug_assert_eq!(old_layout.align(), new_layout.align())`).
>
> The std allocators generally prohibit zero sized allocations, so it
> seems sensible for us to do the same?
I never understood why they do that, the stdlib `Allocator` trait has
all the information it needs to detect zero-sized allocations, so it
could just return dangling pointers. I don't see the point of
duplicating the zero-sized logic in `Box` and `Vec`...
---
Cheers,
Benno
On Wed, Aug 07, 2024 at 08:01:00AM +0000, Benno Lossin wrote:
> On 07.08.24 09:51, Alice Ryhl wrote:
> > On Wed, Aug 7, 2024 at 9:49 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >> On 07.08.24 01:01, Danilo Krummrich wrote:
> >>> On Tue, Aug 06, 2024 at 07:47:17PM +0000, Benno Lossin wrote:
> >>>> On 05.08.24 17:19, Danilo Krummrich wrote:
> >>>>> +impl<T, A> Box<T, A>
> >>>>> +where
> >>>>> + T: ?Sized,
> >>>>> + A: Allocator,
> >>>>> +{
> >>>>> + /// Constructs a `Box<T, A>` from a raw pointer.
> >>>>> + ///
> >>>>> + /// # Safety
> >>>>> + ///
> >>>>> + /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
> >>>>> + /// type `T`.
> >>>>
> >>>> With this requirement and the invariant on `Box`, I am lead to believe
> >>>> that you can't use this for ZSTs, since they are not allocated with `A`.
> >>>> One solution would be to adjust this requirement. But I would rather use
> >>>> a different solution: we move the dangling pointer stuff into the
> >>>> allocator and also call it when `T` is a ZST (ie don't special case them
> >>>> in `Box` but in the impls of `Allocator`). That way this can stay as-is
> >>>> and the part about ZSTs in the invariant can be removed.
> >>>
> >>> Actually, we already got that. Every zero sized allocation will return a
> >>> dangling pointer. However, we can't call `Allocator::free` with (any) dangling
> >>> pointer though.
> >>
> >> The last part is rather problematic in my opinion, since the safety
> >> requirements of the functions in `Allocator` don't ensure that you're
> >> not allowed to do it. We should make it possible to free dangling
> >> pointers that were previously "allocated" by the allocator (ie returned
> >> by `realloc`).
> >> Maybe we do need an `old_layout` parameter for that (that way we can
> >> also `debug_assert_eq!(old_layout.align(), new_layout.align())`).
> >
> > The std allocators generally prohibit zero sized allocations, so it
> > seems sensible for us to do the same?
>
> I never understood why they do that, the stdlib `Allocator` trait has
> all the information it needs to detect zero-sized allocations, so it
> could just return dangling pointers. I don't see the point of
> duplicating the zero-sized logic in `Box` and `Vec`...
I think it's simpler and less error prone.
Besides that, I think the stdlib `Box` allows to call `from_raw` with any random
pointer for ZST, which the allocator API can't catch.
>
> ---
> Cheers,
> Benno
>
© 2016 - 2025 Red Hat, Inc.