Generic `Allocator` support for Rust

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Quoted 7 lines · Danilo Krummrich

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Quoted 2 lines

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

We discussed this in our weekly meeting (I think ~one week ago?). If you give me a draft version of the comment that you plan to add regarding the old_layout parameter, I can see if I am happy with it. If I am, I would give you my RB.

---
Cheers,
Benno

Quoted 11 lines · Benno Lossin

Quoted 9 lines · Danilo Krummrich

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Quoted 4 lines

We discussed this in our weekly meeting (I think ~one week ago?). If you give me a draft version of the comment that you plan to add regarding the old_layout parameter, I can see if I am happy with it. If I am, I would give you my RB.

May I propose you let me know what you would like to see covered, rather than me trying to guess it. :-)

Quoted 12 lines · Danilo Krummrich

Quoted 11 lines · Benno Lossin

Quoted 9 lines · Danilo Krummrich

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Quoted 7 lines

Quoted 4 lines

We discussed this in our weekly meeting (I think ~one week ago?). If you give me a draft version of the comment that you plan to add regarding the old_layout parameter, I can see if I am happy with it. If I am, I would give you my RB.

May I propose you let me know what you would like to see covered, rather than me trying to guess it. :-)

I was hoping that we put that in our meeting notes, but I failed to find them... I would put this in a normal comment, so it doesn't show up in the documentation. Preface it like implementation decision/detail:
- Why do Allocator::{realloc,free} not have an old_layout parameter like in the stdlib? (the reasons you had for that decision, like we don't need it etc.)

- Then something along the lines of "Note that no technical reason is listed above, so if you need/want to implement an allocator taking advantage of that, you can change it"

I don't think we need a lot here. Additionally it would be very useful to also put this in an issue to not lose track of it.

---
Cheers,
Benno

Quoted 21 lines · Benno Lossin

Quoted 12 lines · Danilo Krummrich

Quoted 11 lines · Benno Lossin

Quoted 9 lines · Danilo Krummrich

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Quoted 7 lines

Quoted 4 lines

We discussed this in our weekly meeting (I think ~one week ago?). If you give me a draft version of the comment that you plan to add regarding the old_layout parameter, I can see if I am happy with it. If I am, I would give you my RB.

May I propose you let me know what you would like to see covered, rather than me trying to guess it. :-)

Quoted 6 lines

I was hoping that we put that in our meeting notes, but I failed to find them... I would put this in a normal comment, so it doesn't show up in the documentation. Preface it like implementation decision/detail:
- Why do Allocator::{realloc,free} not have an old_layout parameter like in the stdlib? (the reasons you had for that decision, like we don't need it etc.)

Ok.

Quoted 3 lines

- Then something along the lines of "Note that no technical reason is listed above, so if you need/want to implement an allocator taking advantage of that, you can change it"

I don't really want to set the conditions for this to change in the documentation. It really depends on whether it's actually needed or the advantage of having it is huge enough to leave the core kernel allocators with unused arguments.

This can really only be properly evaluated case by case in a discussion.

Quoted 40 lines · Danilo Krummrich

Quoted 21 lines · Benno Lossin

Quoted 12 lines · Danilo Krummrich

Quoted 11 lines · Benno Lossin

Quoted 9 lines · Danilo Krummrich

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Quoted 7 lines

Quoted 4 lines

We discussed this in our weekly meeting (I think ~one week ago?). If you give me a draft version of the comment that you plan to add regarding the old_layout parameter, I can see if I am happy with it. If I am, I would give you my RB.

May I propose you let me know what you would like to see covered, rather than me trying to guess it. :-)

Quoted 6 lines

I was hoping that we put that in our meeting notes, but I failed to find them... I would put this in a normal comment, so it doesn't show up in the documentation. Preface it like implementation decision/detail:
- Why do Allocator::{realloc,free} not have an old_layout parameter like in the stdlib? (the reasons you had for that decision, like we don't need it etc.)

Ok.

Quoted 3 lines

- Then something along the lines of "Note that no technical reason is listed above, so if you need/want to implement an allocator taking advantage of that, you can change it"

I don't really want to set the conditions for this to change in the documentation. It really depends on whether it's actually needed or the advantage of having it is huge enough to leave the core kernel allocators with unused arguments.

Quoted 1 line

This can really only be properly evaluated case by case in a discussion.

Agreed, but I don't want people to think that we have a reason against doing it in the future. Do you have an idea how to convey this?

---
Cheers,
Benno

Quoted 43 lines · Benno Lossin

Quoted 41 lines · Danilo Krummrich

Quoted 21 lines · Benno Lossin

Quoted 12 lines · Danilo Krummrich

Quoted 11 lines · Benno Lossin

Quoted 9 lines · Danilo Krummrich

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

Quoted 7 lines

Quoted 4 lines

We discussed this in our weekly meeting (I think ~one week ago?). If you give me a draft version of the comment that you plan to add regarding the old_layout parameter, I can see if I am happy with it. If I am, I would give you my RB.

May I propose you let me know what you would like to see covered, rather than me trying to guess it. :-)

Quoted 6 lines

I was hoping that we put that in our meeting notes, but I failed to find them... I would put this in a normal comment, so it doesn't show up in the documentation. Preface it like implementation decision/detail:
- Why do Allocator::{realloc,free} not have an old_layout parameter like in the stdlib? (the reasons you had for that decision, like we don't need it etc.)

Ok.

Quoted 3 lines

- Then something along the lines of "Note that no technical reason is listed above, so if you need/want to implement an allocator taking advantage of that, you can change it"

I don't really want to set the conditions for this to change in the documentation. It really depends on whether it's actually needed or the advantage of having it is huge enough to leave the core kernel allocators with unused arguments.

This can really only be properly evaluated case by case in a discussion.

Quoted 2 lines

Agreed, but I don't want people to think that we have a reason against doing it in the future. Do you have an idea how to convey this?

I understand (and agree with) your intention. But I don't think it's necessary to document, because, ideally, this is already true for the whole kernel.

Generally, I think it's valid to assume that people are willing to change the code if the advantages outweigh the disadvantages.

So, we could write something like "This may be changed if the advantages outweigh the disadvantages.", but it'd be a bit random, since we could probably sprinkle this everywhere.

rust/kernel/alloc.rs +11 -0

+/// The kernel's [`Allocator`] trait.
+///
+/// An implementation of [`Allocator`] can allocate, re-allocate and free memory buffer described
+/// via [`Layout`].
+///
+/// [`Allocator`] is designed to be implemented as a ZST; [`Allocator`] functions do not operate on
+/// an object instance.
+///
+/// In order to be able to support `#[derive(SmartPointer)]` later on, we need to avoid a design
+/// that requires an `Allocator` to be instantiated, hence its functions must not contain any kind
+/// of `self` parameter.

Add a kernel specific Allocator trait, that in contrast to the one in Rust's core library doesn't require unstable features and supports GFP flags.

Subsequent patches add the following trait implementors: Kmalloc, Vmalloc and KVmalloc.

Reviewed-by Alice Ryhl <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc.rs | 102 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

rust/kernel/alloc.rs +70 -0

index 1966bd407017..9932f21b0539 100644
@@ -11,6 +11,7 @@
 /// Indicates an allocation error.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub struct AllocError;
+use core::{alloc::Layout, ptr::NonNull};
 
 /// Flags to be used when allocating memory.
 ///

@@ -86,3 +87,104 @@ pub mod flags {
     /// small allocations.
     pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
 }
+
+/// The kernel's [`Allocator`] trait.
+///
+/// An implementation of [`Allocator`] can allocate, re-allocate and free memory buffer described
+/// via [`Layout`].
+///
+/// [`Allocator`] is designed to be implemented as a ZST; [`Allocator`] functions do not operate on
+/// an object instance.
+///
+/// In order to be able to support `#[derive(SmartPointer)]` later on, we need to avoid a design
+/// that requires an `Allocator` to be instantiated, hence its functions must not contain any kind
+/// of `self` parameter.
+///
+/// # Safety
+///
+/// A memory allocation returned from an allocator must remain valid until it is explicitly freed.
+///
+/// Any pointer to a valid memory allocation must be valid to be passed to any other [`Allocator`]
+/// function of the same type.
+///
+/// Implementers must ensure that all trait functions abide by the guarantees documented in the
+/// `# Guarantees` sections.
+pub unsafe trait Allocator {
+    /// Allocate memory based on `layout` and `flags`.
+    ///
+    /// On success, returns a buffer represented as `NonNull<[u8]>` that satisfies the layout
+    /// constraints (i.e. minimum size and alignment as specified by `layout`).
+    ///
+    /// This function is equivalent to `realloc` when called with `None`.
+    ///
+    /// # Guarantees
+    ///
+    /// When the return value is `Ok(ptr)`, then `ptr` is
+    /// - valid for reads and writes for `layout.size()` bytes, until it is passed to
+    ///   [`Allocator::free`] or [`Allocator::realloc`],
+    /// - aligned to `layout.align()`,
+    ///
+    /// Additionally, `Flags` are honored as documented in
+    /// <https://docs.kernel.org/core-api/mm-api.html#mm-api-gfp-flags>.
+    fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: Passing `None` to `realloc` is valid by it's safety requirements and asks for a
+        // new memory allocation.
+        unsafe { Self::realloc(None, layout, flags) }
+    }
+
+    /// Re-allocate an existing memory allocation to satisfy the requested `layout`.
+    ///
+    /// If the requested size is zero, `realloc` behaves equivalent to `free`.
+    ///
+    /// If the requested size is larger than the size of the existing allocation, a successful call
+    /// to `realloc` guarantees that the new or grown buffer has at least `Layout::size` bytes, but
+    /// may also be larger.
+    ///
+    /// If the requested size is smaller than the size of the existing allocation, `realloc` may or
+    /// may not shrink the buffer; this is implementation specific to the allocator.
+    ///
+    /// On allocation failure, the existing buffer, if any, remains valid.
+    ///
+    /// The buffer is represented as `NonNull<[u8]>`.
+    ///
+    /// # Safety
+    ///
+    /// If `ptr == Some(p)`, then `p` must point to an existing and valid memory allocation created
+    /// by this allocator. The alignment encoded in `layout` must be smaller than or equal to the
+    /// alignment requested in the previous `alloc` or `realloc` call of the same allocation.
+    ///
+    /// Additionally, `ptr` is allowed to be `None`; in this case a new memory allocation is
+    /// created.
+    ///

rust/kernel/alloc.rs +22 -0

+    /// - when the return value is `Err(AllocError)`, then `p` is still valid.
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError>;
+
+    /// Free an existing memory allocation.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to an existing and valid memory allocation created by this `Allocator` and
+    /// must not be a dangling pointer.
+    ///
+    /// The memory allocation at `ptr` must never again be read from or written to.
+    unsafe fn free(ptr: NonNull<u8>) {
+        // SAFETY: The caller guarantees that `ptr` points at a valid allocation created by this
+        // allocator. We are passing a `Layout` with the smallest possible alignment, so it is
+        // smaller than or equal to the alignment previously used with this allocation.
+        let _ = unsafe { Self::realloc(Some(ptr), Layout::new::<()>(), Flags(0)) };
+    }
+}

Separate aligned_size from krealloc_aligned.

Subsequent patches implement Allocator derivates, such as Kmalloc, that require aligned_size and replace the original krealloc_aligned.

Reviewed-by Alice Ryhl <[email protected]>

Reviewed-by Benno Lossin <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc/allocator.rs | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

rust/kernel/alloc/allocator.rs +12 -7

index e6ea601f38c6..c83b6dff896d 100644
@@ -8,6 +8,17 @@
 
 struct KernelAllocator;
 
+/// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
+fn aligned_size(new_layout: Layout) -> usize {
+    // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
+    let layout = new_layout.pad_to_align();
+
+    // Note that `layout.size()` (after padding) is guaranteed to be a multiple of `layout.align()`
+    // which together with the slab guarantees means the `krealloc` will return a properly aligned
+    // object (see comments in `kmalloc()` for more information).
+    layout.size()
+}
+
 /// Calls `krealloc` with a proper size to alloc a new object aligned to `new_layout`'s alignment.
 ///
 /// # Safety

@@ -15,13 +26,7 @@
 /// - `ptr` can be either null or a pointer which has been allocated by this allocator.
 /// - `new_layout` must have a non-zero size.
 pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: Flags) -> *mut u8 {
-    // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
-    let layout = new_layout.pad_to_align();
-
-    // Note that `layout.size()` (after padding) is guaranteed to be a multiple of `layout.align()`
-    // which together with the slab guarantees means the `krealloc` will return a properly aligned
-    // object (see comments in `kmalloc()` for more information).
-    let size = layout.size();
+    let size = aligned_size(new_layout);
 
     // SAFETY:
     // - `ptr` is either null or a pointer returned from a previous `k{re}alloc()` by the

Implement Allocator for Kmalloc, the kernel's default allocator, typically used for objects smaller than page size.

All memory allocations made with Kmalloc end up in krealloc().

It serves as allocator for the subsequently introduced types KBox and KVec.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc.rs           |  2 +-
 rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

rust/kernel/alloc.rs +1 -1

index 9932f21b0539..477dbe3c5a2f 100644
@@ -4,7 +4,7 @@
 
 #[cfg(not(test))]
 #[cfg(not(testlib))]
-mod allocator;
+pub mod allocator;
 pub mod box_ext;
 pub mod vec_ext;
 

rust/kernel/alloc/allocator.rs +0 -0

index e32182f91167..78e7d5488843 100644

rust/kernel/alloc/allocator.rs +0 -0

 use super::{flags::*, Flags};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;

rust/kernel/alloc/allocator.rs +20 -0

+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -36,6 +52,60 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,

rust/kernel/alloc/allocator.rs +6 -0

+        let raw_ptr = unsafe {
+            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
+            self.0(ptr.cast(), size, flags.0).cast()
+        };
+
+        let ptr = if size == 0 {

rust/kernel/alloc/allocator.rs +11 -1

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}
+
 unsafe impl GlobalAlloc for Kmalloc {
     unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
         // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
--

2.46.0

Quoted 32 lines · Danilo Krummrich

Implement Allocator for Kmalloc, the kernel's default allocator, typically used for objects smaller than page size.

All memory allocations made with Kmalloc end up in krealloc().

It serves as allocator for the subsequently introduced types KBox and KVec.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc.rs           |  2 +-
 rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

rust/kernel/alloc.rs +1 -1

index 9932f21b0539..477dbe3c5a2f 100644
@@ -4,7 +4,7 @@
 
 #[cfg(not(test))]
 #[cfg(not(testlib))]
-mod allocator;
+pub mod allocator;
 pub mod box_ext;
 pub mod vec_ext;
 

rust/kernel/alloc/allocator.rs +0 -0

index e32182f91167..78e7d5488843 100644

rust/kernel/alloc/allocator.rs line 9

Quoted 9 lines

rust/kernel/alloc/allocator.rs +6 -0

@@ -1,12 +1,28 @@
 // SPDX-License-Identifier: GPL-2.0
 
 //! Allocator support.
+//!
+//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
+//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
+//! typical application of the different kernel allocators.
+//!
+//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>

Thanks, this nice.

Quoted 4 lines

rust/kernel/alloc/allocator.rs +0 -0

 use super::{flags::*, Flags};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;

rust/kernel/alloc/allocator.rs line 22

Quoted 10 lines

rust/kernel/alloc/allocator.rs +8 -1

+use core::ptr::NonNull;
 
-struct Kmalloc;
+use crate::alloc::{AllocError, Allocator};
+use crate::bindings;
+
+/// The contiguous kernel allocator.
+///
+/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
+/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.

Does putting a link here work? (I guess we don't yet export the bindings documentation, so it will probably fail... When we decide to enable it, we should create an issue to add missing links)

Quoted 32 lines

rust/kernel/alloc/allocator.rs +25 -0

+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -36,6 +52,60 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let size = aligned_size(layout);
+        let ptr = match ptr {

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 81

Quoted 5 lines

rust/kernel/alloc/allocator.rs +5 -0

+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.

You need some justification as to why calling the three allowed functions here.

Quoted 11 lines

rust/kernel/alloc/allocator.rs +10 -0

+        let raw_ptr = unsafe {
+            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
+            self.0(ptr.cast(), size, flags.0).cast()
+        };
+
+        let ptr = if size == 0 {
+            NonNull::dangling()
+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 97

Quoted 6 lines

rust/kernel/alloc/allocator.rs +6 -0

+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+
+unsafe impl Allocator for Kmalloc {

Missing SAFETY comment.

---
Cheers,
Benno

Quoted 17 lines

rust/kernel/alloc/allocator.rs +11 -1

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}
+
 unsafe impl GlobalAlloc for Kmalloc {
     unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
         // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
--

2.46.0

Quoted 96 lines · Benno Lossin

Quoted 32 lines · Danilo Krummrich

Implement Allocator for Kmalloc, the kernel's default allocator, typically used for objects smaller than page size.

All memory allocations made with Kmalloc end up in krealloc().

It serves as allocator for the subsequently introduced types KBox and KVec.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc.rs           |  2 +-
 rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

rust/kernel/alloc.rs +1 -1

index 9932f21b0539..477dbe3c5a2f 100644
@@ -4,7 +4,7 @@
 
 #[cfg(not(test))]
 #[cfg(not(testlib))]
-mod allocator;
+pub mod allocator;
 pub mod box_ext;
 pub mod vec_ext;
 

rust/kernel/alloc/allocator.rs +0 -0

index e32182f91167..78e7d5488843 100644

rust/kernel/alloc/allocator.rs line 9

Quoted 9 lines

rust/kernel/alloc/allocator.rs +6 -0

@@ -1,12 +1,28 @@
 // SPDX-License-Identifier: GPL-2.0
 
 //! Allocator support.
+//!
+//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
+//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
+//! typical application of the different kernel allocators.
+//!
+//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>

Thanks, this nice.

Quoted 4 lines

rust/kernel/alloc/allocator.rs +0 -0

 use super::{flags::*, Flags};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;

rust/kernel/alloc/allocator.rs line 22

Quoted 10 lines

rust/kernel/alloc/allocator.rs +8 -1

+use core::ptr::NonNull;
 
-struct Kmalloc;
+use crate::alloc::{AllocError, Allocator};
+use crate::bindings;
+
+/// The contiguous kernel allocator.
+///
+/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
+/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.

Does putting a link here work? (I guess we don't yet export the bindings documentation, so it will probably fail... When we decide to enable it, we should create an issue to add missing links)

Quoted 32 lines

rust/kernel/alloc/allocator.rs +25 -0

+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -36,6 +52,60 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let size = aligned_size(layout);
+        let ptr = match ptr {

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 81

Quoted 5 lines

rust/kernel/alloc/allocator.rs +5 -0

+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.

You need some justification as to why calling the three allowed functions here.

What kind of justification do I need? Can you please share some more details on what you think is missing here?

Quoted 10 lines

Quoted 10 lines

rust/kernel/alloc/allocator.rs +10 -0

+        let raw_ptr = unsafe {
+            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
+            self.0(ptr.cast(), size, flags.0).cast()
+        };
+
+        let ptr = if size == 0 {
+            NonNull::dangling()
+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 97

Quoted 6 lines

rust/kernel/alloc/allocator.rs +6 -0

+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+
+unsafe impl Allocator for Kmalloc {

Missing SAFETY comment.

Yeah, I think we came across this in an earlier version of the series. I asked you about the content and usefulness of a comment here, since I'd just end up re-iterating what the Allocator trait documentation says.

IIRC, you replied that you want to think of something that'd make sense to add here.

What do you think should be written here?

Quoted 10 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 107

Quoted 10 lines

rust/kernel/alloc/allocator.rs +10 -0

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}

Quoted 8 lines

Quoted 7 lines

rust/kernel/alloc/allocator.rs +1 -1

+
 unsafe impl GlobalAlloc for Kmalloc {
     unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
         // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
--

2.46.0

Quoted 106 lines · Danilo Krummrich

Quoted 96 lines · Benno Lossin

Quoted 32 lines · Danilo Krummrich

Implement Allocator for Kmalloc, the kernel's default allocator, typically used for objects smaller than page size.

All memory allocations made with Kmalloc end up in krealloc().

It serves as allocator for the subsequently introduced types KBox and KVec.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc.rs           |  2 +-
 rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

rust/kernel/alloc.rs +1 -1

index 9932f21b0539..477dbe3c5a2f 100644
@@ -4,7 +4,7 @@
 
 #[cfg(not(test))]
 #[cfg(not(testlib))]
-mod allocator;
+pub mod allocator;
 pub mod box_ext;
 pub mod vec_ext;
 

rust/kernel/alloc/allocator.rs +0 -0

index e32182f91167..78e7d5488843 100644

rust/kernel/alloc/allocator.rs line 9

Quoted 9 lines

rust/kernel/alloc/allocator.rs +6 -0

@@ -1,12 +1,28 @@
 // SPDX-License-Identifier: GPL-2.0
 
 //! Allocator support.
+//!
+//! Documentation for the kernel's memory allocators can found in the "Memory Allocation Guide"
+//! linked below. For instance, this includes the concept of "get free page" (GFP) flags and the
+//! typical application of the different kernel allocators.
+//!
+//! Reference: <https://docs.kernel.org/core-api/memory-allocation.html>

Thanks, this nice.

Quoted 4 lines

rust/kernel/alloc/allocator.rs +0 -0

 use super::{flags::*, Flags};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;

rust/kernel/alloc/allocator.rs line 22

Quoted 10 lines

rust/kernel/alloc/allocator.rs +8 -1

+use core::ptr::NonNull;
 
-struct Kmalloc;
+use crate::alloc::{AllocError, Allocator};
+use crate::bindings;
+
+/// The contiguous kernel allocator.
+///
+/// `Kmalloc` is typically used for physically contiguous allocations up to page size, but also
+/// supports larger allocations up to `bindings::KMALLOC_MAX_SIZE`, which is hardware specific.

Does putting a link here work? (I guess we don't yet export the bindings documentation, so it will probably fail... When we decide to enable it, we should create an issue to add missing links)

Quoted 32 lines

rust/kernel/alloc/allocator.rs +25 -0

+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -36,6 +52,60 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let size = aligned_size(layout);
+        let ptr = match ptr {

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 81

Quoted 5 lines

rust/kernel/alloc/allocator.rs +5 -0

+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.

You need some justification as to why calling the three allowed functions here.

Quoted 2 lines

What kind of justification do I need? Can you please share some more details on what you think is missing here?

So, you are calling a function pointer to an unsafe function. This means that through some invariant you have to know what the safety requirements are (otherwise how can you guarantee that this is OK?). You have the invariant that the pointer points at one of the three functions mentioned above. What are the safety requirements of those functions? I would assume that the only one is that ptr is valid. So you can use:

    // SAFETY:
    // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
    //   NULL or valid.

    // - `ptr` is either NULL or valid by the safety requirements of this function.

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

rust/kernel/alloc/allocator.rs +10 -0

+        let raw_ptr = unsafe {
+            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
+            self.0(ptr.cast(), size, flags.0).cast()
+        };
+
+        let ptr = if size == 0 {
+            NonNull::dangling()
+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 97

Quoted 6 lines

rust/kernel/alloc/allocator.rs +6 -0

+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+
+unsafe impl Allocator for Kmalloc {

Missing SAFETY comment.

Quoted 6 lines

Yeah, I think we came across this in an earlier version of the series. I asked you about the content and usefulness of a comment here, since I'd just end up re-iterating what the Allocator trait documentation says.

IIRC, you replied that you want to think of something that'd make sense to add here.

Oh yeah, sorry I forgot about that.

Quoted 1 line

What do you think should be written here?

I think the best way to do it, would be to push this question down into ReallocFunc::call. So we would put this on the trait:

    // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
    // - memory remains valid until it is explicitly freed,
    // - passing a pointer to a vaild memory allocation is OK,
    // - `realloc` satisfies the guarantees, since `ReallocFunc::call` has the same.

We then need to put this on ReallocFunc::call:

    /// # Guarantees
    ///
    /// This method has the same guarantees as `Allocator::realloc`. Additionally
    /// - it accepts any pointer to a valid memory allocation allocated by this function.

    /// - memory allocated by this function remains valid until it is passed to this function.

Finally, we need a GUARANTEE comment (just above the return [^1] value) that establishes these guarantees:

    // GUARANTEE: Since we called `self.0` with `size` above and by the type invariants of `Self`,
    // `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc`. Those functions provide the guarantees of
    // this function.

I am not really happy with the last sentence, but I also don't think that there is value in listing out all the guarantees, only to then say "all of this is guaranteed by us calling one of these three functions.

[^1]: I am not sure that there is the right place. If you have any
suggestions, feel free to share them.

Quoted 12 lines

Quoted 12 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 107

Quoted 12 lines

rust/kernel/alloc/allocator.rs +10 -0

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}

Oh one more thing, I know that you already have a lot of patches in this series, but could you split this one into two? So the first one should introduce ReallocFunc and the second one add the impl for Kmalloc? I managed to confuse me twice because of that :)

---
Cheers,
Benno

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

rust/kernel/alloc/allocator.rs +1 -1

+
 unsafe impl GlobalAlloc for Kmalloc {
     unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
         // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
--

2.46.0

Quoted 54 lines · Benno Lossin

Quoted 43 lines · Danilo Krummrich

Quoted 33 lines · Benno Lossin

Quoted 32 lines · Danilo Krummrich

rust/kernel/alloc/allocator.rs +25 -0

+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -36,6 +52,60 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let size = aligned_size(layout);
+        let ptr = match ptr {

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 81

Quoted 5 lines

rust/kernel/alloc/allocator.rs +5 -0

+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.

You need some justification as to why calling the three allowed functions here.

Quoted 2 lines

What kind of justification do I need? Can you please share some more details on what you think is missing here?

So, you are calling a function pointer to an unsafe function. This means that through some invariant you have to know what the safety requirements are (otherwise how can you guarantee that this is OK?). You have the invariant that the pointer points at one of the three functions mentioned above. What are the safety requirements of those functions? I would assume that the only one is that ptr is valid. So you can use:

Quoted 3 lines

    // SAFETY:
    // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
    //   NULL or valid.

I'm fine adding it, but I'd like to understand why you think it's required in the safety comment here? Isn't this implicit by being the type invariant?

Quoted 1 line

    // - `ptr` is either NULL or valid by the safety requirements of this function.

This is the part I already have.

Quoted 33 lines

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

rust/kernel/alloc/allocator.rs +10 -0

+        let raw_ptr = unsafe {
+            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
+            self.0(ptr.cast(), size, flags.0).cast()
+        };
+
+        let ptr = if size == 0 {
+            NonNull::dangling()
+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 97

Quoted 6 lines

rust/kernel/alloc/allocator.rs +6 -0

+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+
+unsafe impl Allocator for Kmalloc {

Missing SAFETY comment.

Quoted 6 lines

Yeah, I think we came across this in an earlier version of the series. I asked you about the content and usefulness of a comment here, since I'd just end up re-iterating what the Allocator trait documentation says.

IIRC, you replied that you want to think of something that'd make sense to add here.

Oh yeah, sorry I forgot about that.

Quoted 1 line

What do you think should be written here?

I think the best way to do it, would be to push this question down into ReallocFunc::call. So we would put this on the trait:

Quoted 4 lines

    // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
    // - memory remains valid until it is explicitly freed,
    // - passing a pointer to a vaild memory allocation is OK,
    // - `realloc` satisfies the guarantees, since `ReallocFunc::call` has the same.

So, we'd also need the same for:
- unsafe impl Allocator for Vmalloc
- unsafe impl Allocator for KVmalloc

Quoted 6 lines

We then need to put this on ReallocFunc::call:

    /// # Guarantees
    ///
    /// This method has the same guarantees as `Allocator::realloc`. Additionally
    /// - it accepts any pointer to a valid memory allocation allocated by this function.

You propose this, since for Allocator::realloc memory allocated with Allocator::alloc would be fine too I guess.

But if e.g. Kmalloc wouldn't use the default Allocator::alloc, this would be valid too.

We could instead write something like:

"it accepts any pointer to a valid memory allocation allocated with the same kernel allocator."

Quoted 1 line

    /// - memory allocated by this function remains valid until it is passed to this function.

Same here, Kmalloc could implement its own Allocator::free.

Maybe just "...until it is explicitly freed.".

Anyway, I'm fine with both, since non of the kernel allocators uses anything else than ReallocFunc::call to allocate and free memory.

Quoted 8 lines

Finally, we need a GUARANTEE comment (just above the return [^1] value) that establishes these guarantees:

    // GUARANTEE: Since we called `self.0` with `size` above and by the type invariants of `Self`,
    // `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc`. Those functions provide the guarantees of
    // this function.

Quoted 7 lines

I am not really happy with the last sentence, but I also don't think that there is value in listing out all the guarantees, only to then say "all of this is guaranteed by us calling one of these three functions.

[^1]: I am not sure that there is the right place. If you have any
suggestions, feel free to share them.

Either way, I'm fine with this proposal.

Quoted 13 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

rust/kernel/alloc/allocator.rs +10 -0

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}

Quoted 4 lines

Oh one more thing, I know that you already have a lot of patches in this series, but could you split this one into two? So the first one should introduce ReallocFunc and the second one add the impl for Kmalloc? I managed to confuse me twice because of that :)

Generally, I'm fine with that, but I'm not sure if I can avoid an intermediate compiler warning about unused code doing that.

Quoted 59 lines · Danilo Krummrich

Quoted 57 lines · Benno Lossin

Quoted 43 lines · Danilo Krummrich

Quoted 33 lines · Benno Lossin

Quoted 32 lines · Danilo Krummrich

rust/kernel/alloc/allocator.rs +25 -0

+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -36,6 +52,60 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let size = aligned_size(layout);
+        let ptr = match ptr {

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 81

Quoted 5 lines

rust/kernel/alloc/allocator.rs +5 -0

+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.

You need some justification as to why calling the three allowed functions here.

Quoted 2 lines

What kind of justification do I need? Can you please share some more details on what you think is missing here?

So, you are calling a function pointer to an unsafe function. This means that through some invariant you have to know what the safety requirements are (otherwise how can you guarantee that this is OK?). You have the invariant that the pointer points at one of the three functions mentioned above. What are the safety requirements of those functions? I would assume that the only one is that ptr is valid. So you can use:

    // SAFETY:
    // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
    //   NULL or valid.

Quoted 2 lines

I'm fine adding it, but I'd like to understand why you think it's required in the safety comment here? Isn't this implicit by being the type invariant?

You are calling a function pointer to an unsafe function that takes a raw pointer. Without this comment it is not clear what the function pointer's safety requirements are for the raw pointer parameter.

Quoted 4 lines

Quoted 2 lines

    // - `ptr` is either NULL or valid by the safety requirements of this function.

This is the part I already have.

I kept it to ensure that you also keep it.

Quoted 33 lines

Quoted 33 lines

Quoted 19 lines

Quoted 10 lines

Quoted 10 lines

rust/kernel/alloc/allocator.rs +10 -0

+        let raw_ptr = unsafe {
+            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
+            self.0(ptr.cast(), size, flags.0).cast()
+        };
+
+        let ptr = if size == 0 {
+            NonNull::dangling()
+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };

Quoted 8 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 97

Quoted 6 lines

rust/kernel/alloc/allocator.rs +6 -0

+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+
+unsafe impl Allocator for Kmalloc {

Missing SAFETY comment.

Quoted 6 lines

Yeah, I think we came across this in an earlier version of the series. I asked you about the content and usefulness of a comment here, since I'd just end up re-iterating what the Allocator trait documentation says.

IIRC, you replied that you want to think of something that'd make sense to add here.

Oh yeah, sorry I forgot about that.

Quoted 1 line

What do you think should be written here?

I think the best way to do it, would be to push this question down into ReallocFunc::call. So we would put this on the trait:

Quoted 8 lines

Quoted 4 lines

    // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
    // - memory remains valid until it is explicitly freed,
    // - passing a pointer to a vaild memory allocation is OK,
    // - `realloc` satisfies the guarantees, since `ReallocFunc::call` has the same.

So, we'd also need the same for:
- unsafe impl Allocator for Vmalloc
- unsafe impl Allocator for KVmalloc

Yes.

Quoted 10 lines

Quoted 6 lines

We then need to put this on ReallocFunc::call:

    /// # Guarantees
    ///
    /// This method has the same guarantees as `Allocator::realloc`. Additionally
    /// - it accepts any pointer to a valid memory allocation allocated by this function.

You propose this, since for Allocator::realloc memory allocated with Allocator::alloc would be fine too I guess.

Quoted 2 lines

But if e.g. Kmalloc wouldn't use the default Allocator::alloc, this would be valid too.

So if Kmalloc were to implement alloc by not calling ReallocFun::call, then we couldn't use this comment. Do you think that such a change might be required at some point?

Quoted 5 lines

We could instead write something like:

"it accepts any pointer to a valid memory allocation allocated with the same kernel allocator."

It would be better, if we can keep it simpler (ie only realloc is implemented).

Quoted 5 lines

Quoted 1 line

    /// - memory allocated by this function remains valid until it is passed to this function.

Same here, Kmalloc could implement its own Allocator::free.

Maybe just "...until it is explicitly freed.".

I don't really like that, since by that any other function could be meant. Do you need to override the free function? If not then it would be better.

Quoted 39 lines

Anyway, I'm fine with both, since non of the kernel allocators uses anything else than ReallocFunc::call to allocate and free memory.

Quoted 8 lines

Finally, we need a GUARANTEE comment (just above the return [^1] value) that establishes these guarantees:

    // GUARANTEE: Since we called `self.0` with `size` above and by the type invariants of `Self`,
    // `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc`. Those functions provide the guarantees of
    // this function.

Quoted 7 lines

I am not really happy with the last sentence, but I also don't think that there is value in listing out all the guarantees, only to then say "all of this is guaranteed by us calling one of these three functions.

[^1]: I am not sure that there is the right place. If you have any
suggestions, feel free to share them.

Either way, I'm fine with this proposal.

Quoted 17 lines

Quoted 12 lines

Quoted 12 lines

rust/kernel/alloc/allocator.rs pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 107

Quoted 12 lines

rust/kernel/alloc/allocator.rs +10 -0

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}

Oh one more thing, I know that you already have a lot of patches in this series, but could you split this one into two? So the first one should introduce ReallocFunc and the second one add the impl for Kmalloc? I managed to confuse me twice because of that :)

Quoted 2 lines

Generally, I'm fine with that, but I'm not sure if I can avoid an intermediate compiler warning about unused code doing that.

You can just use #[expect(dead_code)] for that in the intermediate patches.

---
Cheers,
Benno

Quoted 63 lines · Benno Lossin

Quoted 61 lines · Danilo Krummrich

Quoted 54 lines · Benno Lossin

Quoted 43 lines · Danilo Krummrich

Quoted 33 lines · Benno Lossin

Quoted 32 lines · Danilo Krummrich

(quoted diff) +25 -0

+///
+/// For more details see [self].
+pub struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -36,6 +52,60 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags.0) as *mut u8 }
 }
 
+/// # Invariants
+///
+/// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
+struct ReallocFunc(
+    unsafe extern "C" fn(*const core::ffi::c_void, usize, u32) -> *mut core::ffi::c_void,
+);
+
+impl ReallocFunc {
+    // INVARIANT: `krealloc` satisfies the type invariants.
+    const KREALLOC: Self = Self(bindings::krealloc);
+
+    /// # Safety
+    ///
+    /// This method has the same safety requirements as [`Allocator::realloc`].
+    unsafe fn call(
+        &self,
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let size = aligned_size(layout);
+        let ptr = match ptr {

Quoted 8 lines

Review pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F · line 81

Quoted 5 lines

(quoted diff) +5 -0

+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is either NULL or valid by the safety requirements of this function.

You need some justification as to why calling the three allowed functions here.

Quoted 2 lines

What kind of justification do I need? Can you please share some more details on what you think is missing here?

So, you are calling a function pointer to an unsafe function. This means that through some invariant you have to know what the safety requirements are (otherwise how can you guarantee that this is OK?). You have the invariant that the pointer points at one of the three functions mentioned above. What are the safety requirements of those functions? I would assume that the only one is that ptr is valid. So you can use:

Quoted 3 lines

    // SAFETY:
    // - `self.0` is one of `krealloc`, `vrealloc`, `kvrealloc` and thus only requires that `ptr` is
    //   NULL or valid.

I'm fine adding it, but I'd like to understand why you think it's required in the safety comment here? Isn't this implicit by being the type invariant?

Quoted 3 lines

You are calling a function pointer to an unsafe function that takes a raw pointer. Without this comment it is not clear what the function pointer's safety requirements are for the raw pointer parameter.

That's my point, isn't this implicitly clear by the type invariant? If needed, shouldn't it be:

// INVARIANT:
// - self.0 is one of [...]
//
// SAFETY:
// - ptr is either NULL or [...]

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

    // - `ptr` is either NULL or valid by the safety requirements of this function.

This is the part I already have.

I kept it to ensure that you also keep it.

Quoted 19 lines

Quoted 18 lines

Quoted 11 lines

Quoted 10 lines

Quoted 10 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
+        unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
+    }
+}

Quoted 4 lines

Oh one more thing, I know that you already have a lot of patches in this series, but could you split this one into two? So the first one should introduce ReallocFunc and the second one add the impl for Kmalloc? I managed to confuse me twice because of that :)

Generally, I'm fine with that, but I'm not sure if I can avoid an intermediate compiler warning about unused code doing that.

Quoted 2 lines

You can just use #[expect(dead_code)] for that in the intermediate patches.

I usually try to avoid that, because it can be misleading when bisecting things.

If the temporarily unused code contains a bug, your bisection doesn't end up at this patch, but some other patch that starts using it.

Allocators, such as Kmalloc, will be used by e.g. Box and Vec in subsequent patches, and hence this dependency propagates throughout the whole kernel.

Add the allocator_test module that provides an empty implementation for all Allocators in the kernel, such that we don't break the rusttest make target in subsequent patches.

Reviewed-by Alice Ryhl <[email protected]>

Reviewed-by Benno Lossin <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc.rs                |  9 +++++++--
 rust/kernel/alloc/allocator_test.rs | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 rust/kernel/alloc/allocator_test.rs

rust/kernel/alloc.rs +7 -2

index 477dbe3c5a2f..91a367a58f36 100644
@@ -2,12 +2,17 @@
 
 //! Extensions to the [`alloc`] crate.
 
-#[cfg(not(test))]
-#[cfg(not(testlib))]
+#[cfg(not(any(test, testlib)))]
 pub mod allocator;
 pub mod box_ext;
 pub mod vec_ext;
 
+#[cfg(any(test, testlib))]
+pub mod allocator_test;
+
+#[cfg(any(test, testlib))]
+pub use self::allocator_test as allocator;
+
 /// Indicates an allocation error.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub struct AllocError;

rust/kernel/alloc/allocator_test.rs new file +19 -0

new file mode 100644
index 000000000000..4785efc474a7
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#![allow(missing_docs)]
+
+use super::{AllocError, Allocator, Flags};
+use core::alloc::Layout;
+use core::ptr::NonNull;
+
+pub struct Kmalloc;
+
+unsafe impl Allocator for Kmalloc {
+    unsafe fn realloc(
+        _ptr: Option<NonNull<u8>>,
+        _layout: Layout,
+        _flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        panic!();
+    }
+}

rust/kernel/alloc/allocator.rs +3 -0

@@ -63,6 +73,9 @@ impl ReallocFunc {
     // INVARIANT: `krealloc` satisfies the type invariants.
     const KREALLOC: Self = Self(bindings::krealloc);
 
+    // INVARIANT: `vrealloc` satisfies the type invariants.
+    const VREALLOC: Self = Self(bindings::vrealloc);
+
     /// # Safety
     ///
     /// This method has the same safety requirements as [`Allocator::realloc`].

rust/kernel/alloc/allocator.rs +18 -0

+    #[inline]
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        // TODO: Support alignments larger than PAGE_SIZE.
+        if layout.align() > bindings::PAGE_SIZE {
+            pr_warn!("Vmalloc does not support alignments larger than PAGE_SIZE yet.\n");
+            return Err(AllocError);
+        }
+
+        // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously
+        // allocated with this `Allocator`.
+        unsafe { ReallocFunc::VREALLOC.call(ptr, layout, flags) }
+    }
+}
+
 #[global_allocator]
 static ALLOCATOR: Kmalloc = Kmalloc;
 

rust/kernel/alloc/allocator_test.rs +1 -1

index 4785efc474a7..e7bf2982f68f 100644
@@ -7,6 +7,7 @@
 use core::ptr::NonNull;
 
 pub struct Kmalloc;
+pub type Vmalloc = Kmalloc;
 
 unsafe impl Allocator for Kmalloc {
     unsafe fn realloc(
--

2.46.0

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 <[email protected]>

Quoted 11 lines · Danilo Krummrich

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 <[email protected]>

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

rust/kernel/alloc/kbox.rs line 384

Quoted 12 lines

rust/kernel/alloc/kbox.rs new file +11 -0

+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/[email protected]/

Quoted 15 lines · Alice Ryhl

Quoted 11 lines · Danilo Krummrich

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 <[email protected]>

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

Quoted 16 lines

rust/kernel/alloc/kbox.rs line 384

Quoted 12 lines

rust/kernel/alloc/kbox.rs new file +11 -0

+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

Quoted 5 lines

Alice

[1]: https://lore.kernel.org/all/[email protected]/

rust/kernel/alloc/kbox.rs new file +10 -0

new file mode 100644
index 000000000000..93b1ab9de6e8
@@ -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;

rust/kernel/alloc/kbox.rs new file +12 -0

+///
+/// `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);

rust/kernel/alloc/kbox.rs new file +53 -0

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

rust/kernel/alloc/kbox.rs new file +50 -0

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

rust/kernel/alloc/kbox.rs new file +3 -0

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

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

rust/kernel/alloc/kbox.rs new file +4 -0

+            unsafe { A::free(self.0.cast()) };
+        }
+    }
+}

rust/kernel/prelude.rs +1 -2

index 4571daec0961..a9210634a8c3 100644
@@ -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

Quoted 16 lines · Danilo Krummrich

rust/kernel/alloc/kbox.rs new file +10 -0

new file mode 100644
index 000000000000..93b1ab9de6e8
@@ -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;

rust/kernel/alloc/kbox.rs line 20

Quoted 10 lines

rust/kernel/alloc/kbox.rs new file +10 -0

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

rust/kernel/alloc/kbox.rs line 26

Quoted 6 lines

rust/kernel/alloc/kbox.rs new file +6 -0

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

Quoted 18 lines

rust/kernel/alloc/kbox.rs new file +18 -0

+///
+/// `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;

rust/kernel/alloc/kbox.rs line 48

Quoted 4 lines

rust/kernel/alloc/kbox.rs new file +4 -0

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

rust/kernel/alloc/kbox.rs line 56

Quoted 9 lines

rust/kernel/alloc/kbox.rs new file +8 -0

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

rust/kernel/alloc/kbox.rs line 60

Quoted 4 lines

rust/kernel/alloc/kbox.rs new file +4 -0

+///
+/// # Invariants
+///
+/// The [`Box`]' pointer is always properly aligned and either points to memory allocated with `A`

Please use self.0 instead of "[Box]'".

rust/kernel/alloc/kbox.rs line 61

Quoted 1 line

rust/kernel/alloc/kbox.rs new file +1 -0

+/// or, for zero-sized types, is a dangling pointer.

Probably "dangling, well aligned pointer.".

rust/kernel/alloc/kbox.rs line 65

Quoted 5 lines

rust/kernel/alloc/kbox.rs new file +4 -0

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

Quoted 57 lines

rust/kernel/alloc/kbox.rs new file +57 -0

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

rust/kernel/alloc/kbox.rs line 128

Quoted 6 lines

rust/kernel/alloc/kbox.rs new file +6 -0

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

Quoted 51 lines

rust/kernel/alloc/kbox.rs new file +50 -0

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

rust/kernel/alloc/kbox.rs line 188

Quoted 10 lines

rust/kernel/alloc/kbox.rs new file +10 -0

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

rust/kernel/alloc/kbox.rs line 194

Quoted 6 lines

rust/kernel/alloc/kbox.rs new file +6 -0

+    }
+
+    /// 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.

Quoted 3 lines

rust/kernel/alloc/kbox.rs new file +3 -0

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

[...]

rust/kernel/alloc/kbox.rs line 472

Quoted 9 lines

rust/kernel/alloc/kbox.rs new file +9 -0

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

Quoted 1 line

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

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

rust/kernel/alloc/kbox.rs line 476

Quoted 3 lines

rust/kernel/alloc/kbox.rs new file +3 -0

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

Quoted 20 lines

rust/kernel/alloc/kbox.rs new file +4 -0

+            unsafe { A::free(self.0.cast()) };
+        }
+    }
+}

rust/kernel/prelude.rs +1 -2

index 4571daec0961..a9210634a8c3 100644
@@ -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