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.

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

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

rust/kernel/alloc.rs +66 -0

index 1966bd407017..8a71a589469d 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,81 @@ 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
+///
+/// Memory returned from an allocator must point to a valid memory buffer and remain valid until
+/// it is explicitly freed.
+///
+/// Any pointer to a memory buffer which is currently allocated must be valid to be passed to any
+/// other [`Allocator`] function of the same type. The same applies for a NULL pointer.
+///
+/// If `realloc` is called with:
+///   - a size of zero, the given memory allocation, if any, must be freed
+///   - a NULL pointer, a new memory allocation must be created
+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 a NULL pointer.
+    fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: Passing a NULL pointer 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
+    ///
+    /// `Some(ptr)` must point to an existing and valid memory allocation created by this allocator
+    /// instance. 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.
+    ///
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError>;

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.

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

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

rust/kernel/alloc.rs +1 -0

index 1966bd407017..8a71a589469d 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,81 @@ pub mod flags {
     /// small allocations.

rust/kernel/alloc.rs +32 -0

+///
+/// # Safety
+///
+/// Memory returned from an allocator must point to a valid memory buffer and remain valid until
+/// it is explicitly freed.
+///
+/// Any pointer to a memory buffer which is currently allocated must be valid to be passed to any
+/// other [`Allocator`] function of the same type. The same applies for a NULL pointer.
+///
+/// If `realloc` is called with:
+///   - a size of zero, the given memory allocation, if any, must be freed
+///   - a NULL pointer, a new memory allocation must be created
+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 a NULL pointer.
+    fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: Passing a NULL pointer 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.
+    ///

rust/kernel/alloc.rs +8 -0

+    /// instance. 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.
+    ///
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,

rust/kernel/alloc.rs +6 -1

+    unsafe fn free(ptr: NonNull<u8>) {
+        // SAFETY: `ptr` is guaranteed to be previously allocated with this `Allocator` or NULL.
+        // Calling `realloc` with a buffer size of zero, frees the buffer `ptr` points to.
+        let _ = unsafe { Self::realloc(Some(ptr), Layout::new::<()>(), Flags(0)) };
+    }
+}
--

2.45.2

Quoted 31 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.

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

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

rust/kernel/alloc.rs +4 -0

index 1966bd407017..8a71a589469d 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,81 @@ pub mod flags {
     /// small allocations.
     pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
 }
+
+/// The kernel's [`Allocator`] trait.
+///

rust/kernel/alloc.rs pub mod flags { · line 97

Quoted 5 lines

rust/kernel/alloc.rs +5 -0

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

This will prevent us from implementing arena-type allocators [^1]. Do we want/need those?

I have heard that some people use them in embedded systems, but I can't say for sure. But this is a rather big design decision, so we should discuss it now.

[^1]: For those who don't know what I mean by that here is a quick sketch (without handling flags and optimizations):

    pub struct ArenaAlloc<const SIZE: usize> {
        memory: Opaque<[u8; SIZE]>,
        head: Cell<usize>,
    }

    impl<const SIZE: usize> ArenaAlloc<SIZE> {
        pub fn new() -> Self {
            Self {
                memory: Opaque::uninit(),
                head: 0,
            }
        }
    }

    impl<const SIZE: usize> Allocator for ArenaAlloc<SIZE> {
        fn alloc(&self, layout: Layout, _flags: Flags) -> Result<NonNull<u8>, AllocError> {
            let head = self.head.get();
            if head + layout.size() >= SIZE {
                return Err(AllocError);
            }
            let ptr = self.memory.get();
            let ptr = ptr.cast::<u8>();
            let ptr = unsafe { ptr.add(head) };
            self.head.set(head + layout.size());
            unsafe { NonNull::new_unchecked(ptr) }
        }

        unsafe fn realloc(
            &self,
            ptr: Option<NonNull<u8>>,
            old_layout: Layout, // Note that we also need `old_layout`!
            layout: Layout,
            flags: Flags
        ) -> Result<NonNull<u8>, AllocError> {
            let new = self.alloc(layout, flags)?;
            let Some(ptr) = ptr else { return Ok(new); };
            unsafe { core::ptr::copy_nonoverlapping(ptr.as_ptr(), new.as_ptr(), old_layout.size()) };
            self.free(ptr);
            Ok(new)
        }

        fn free(&self, ptr: NonNull<u8>) { /* noop */ }
    }

rust/kernel/alloc.rs pub mod flags { · line 101

Quoted 4 lines

rust/kernel/alloc.rs +4 -0

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

Ah I see, so since #[derive(SmartPointer)] needs Box to only consist of one non ZST field... I skimmed the RFC discussion and it seems like a problem that *might* be solved in the future, but probably not in the (very) near future. I guess this is just a bullet that we have to bite. We can always have an ArenaBox that can deal with that (although without DispatchFromDyn).
We should revisit this when #[derive(SmartPointer)] becomes advanced enough.

Quoted 39 lines

rust/kernel/alloc.rs +38 -0

+///
+/// # Safety
+///
+/// Memory returned from an allocator must point to a valid memory buffer and remain valid until
+/// it is explicitly freed.
+///
+/// Any pointer to a memory buffer which is currently allocated must be valid to be passed to any
+/// other [`Allocator`] function of the same type. The same applies for a NULL pointer.
+///
+/// If `realloc` is called with:
+///   - a size of zero, the given memory allocation, if any, must be freed
+///   - a NULL pointer, a new memory allocation must be created
+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 a NULL pointer.
+    fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: Passing a NULL pointer 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]>`.

rust/kernel/alloc.rs pub mod flags { · line 143

Quoted 4 lines

rust/kernel/alloc.rs +4 -0

+    ///
+    /// # Safety
+    ///
+    /// `Some(ptr)` must point to an existing and valid memory allocation created by this allocator

This is the wrong way around, ptr: Option<NonNull<u8>>, so Some(ptr): Option<Option<NonNull<u8>>>. Instead I would write "If ptr = Some(p), then p must point to...".

Quoted 9 lines

rust/kernel/alloc.rs +8 -0

+    /// instance. 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.
+    ///
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,

rust/kernel/alloc.rs pub mod flags { · line 161

Quoted 10 lines

rust/kernel/alloc.rs +10 -0

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

Additionally, you need "The memory allocation at ptr must never again be read from or written to.".

---
Cheers,
Benno

Quoted 9 lines

rust/kernel/alloc.rs +6 -1

+    unsafe fn free(ptr: NonNull<u8>) {
+        // SAFETY: `ptr` is guaranteed to be previously allocated with this `Allocator` or NULL.
+        // Calling `realloc` with a buffer size of zero, frees the buffer `ptr` points to.
+        let _ = unsafe { Self::realloc(Some(ptr), Layout::new::<()>(), Flags(0)) };
+    }
+}
--

2.45.2

Quoted 32 lines · Benno Lossin

Quoted 31 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.

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

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

rust/kernel/alloc.rs +4 -0

index 1966bd407017..8a71a589469d 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,81 @@ pub mod flags {
     /// small allocations.
     pub const GFP_NOWAIT: Flags = Flags(bindings::GFP_NOWAIT);
 }
+
+/// The kernel's [`Allocator`] trait.
+///

Quoted 8 lines

rust/kernel/alloc.rs pub mod flags { · line 97

Quoted 5 lines

rust/kernel/alloc.rs +5 -0

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

This will prevent us from implementing arena-type allocators [^1]. Do we want/need those?

I'm not aware of any code in the kernel that does exactly this, but kmem_cache is rather close to that.

Quoted 51 lines

I have heard that some people use them in embedded systems, but I can't say for sure. But this is a rather big design decision, so we should discuss it now.

[^1]: For those who don't know what I mean by that here is a quick sketch (without handling flags and optimizations):

    pub struct ArenaAlloc<const SIZE: usize> {
        memory: Opaque<[u8; SIZE]>,
        head: Cell<usize>,
    }

    impl<const SIZE: usize> ArenaAlloc<SIZE> {
        pub fn new() -> Self {
            Self {
                memory: Opaque::uninit(),
                head: 0,
            }
        }
    }

    impl<const SIZE: usize> Allocator for ArenaAlloc<SIZE> {
        fn alloc(&self, layout: Layout, _flags: Flags) -> Result<NonNull<u8>, AllocError> {
            let head = self.head.get();
            if head + layout.size() >= SIZE {
                return Err(AllocError);
            }
            let ptr = self.memory.get();
            let ptr = ptr.cast::<u8>();
            let ptr = unsafe { ptr.add(head) };
            self.head.set(head + layout.size());
            unsafe { NonNull::new_unchecked(ptr) }
        }

        unsafe fn realloc(
            &self,
            ptr: Option<NonNull<u8>>,
            old_layout: Layout, // Note that we also need `old_layout`!
            layout: Layout,
            flags: Flags
        ) -> Result<NonNull<u8>, AllocError> {
            let new = self.alloc(layout, flags)?;
            let Some(ptr) = ptr else { return Ok(new); };
            unsafe { core::ptr::copy_nonoverlapping(ptr.as_ptr(), new.as_ptr(), old_layout.size()) };
            self.free(ptr);
            Ok(new)
        }

        fn free(&self, ptr: NonNull<u8>) { /* noop */ }
    }

Quoted 13 lines

rust/kernel/alloc.rs pub mod flags { · line 101

Quoted 4 lines

rust/kernel/alloc.rs +4 -0

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

Ah I see, so since #[derive(SmartPointer)] needs Box to only consist of one non ZST field... I skimmed the RFC discussion and it seems like a problem that *might* be solved in the future, but probably not in the (very) near future. I guess this is just a bullet that we have to bite. We can always have an ArenaBox that can deal with that (although without DispatchFromDyn).
We should revisit this when #[derive(SmartPointer)] becomes advanced enough.

Agreed.

Quoted 38 lines

Quoted 38 lines

rust/kernel/alloc.rs +38 -0

+///
+/// # Safety
+///
+/// Memory returned from an allocator must point to a valid memory buffer and remain valid until
+/// it is explicitly freed.
+///
+/// Any pointer to a memory buffer which is currently allocated must be valid to be passed to any
+/// other [`Allocator`] function of the same type. The same applies for a NULL pointer.
+///
+/// If `realloc` is called with:
+///   - a size of zero, the given memory allocation, if any, must be freed
+///   - a NULL pointer, a new memory allocation must be created
+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 a NULL pointer.
+    fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: Passing a NULL pointer 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]>`.

Quoted 8 lines

rust/kernel/alloc.rs pub mod flags { · line 143

Quoted 4 lines

rust/kernel/alloc.rs +4 -0

+    ///
+    /// # Safety
+    ///
+    /// `Some(ptr)` must point to an existing and valid memory allocation created by this allocator

This is the wrong way around, ptr: Option<NonNull<u8>>, so Some(ptr): Option<Option<NonNull<u8>>>. Instead I would write "If ptr = Some(p), then p must point to...".

Yes, makes sense.

Quoted 20 lines

Quoted 19 lines

rust/kernel/alloc.rs +18 -0

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

Quoted 2 lines

Additionally, you need "The memory allocation at ptr must never again be read from or written to.".

I'm fine adding it, but I wonder if technically this is really required? The condition whether the pointer is ever accessed again in any way is not relevant in terms of being a precondition for free not causing UB, right?

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

rust/kernel/alloc.rs +18 -0

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

Quoted 6 lines

Quoted 2 lines

Additionally, you need "The memory allocation at ptr must never again be read from or written to.".

I'm fine adding it, but I wonder if technically this is really required? The condition whether the pointer is ever accessed again in any way is not relevant in terms of being a precondition for free not causing UB, right?

I don't see how else we would find the mistake in the following code:

    let ptr = Box::into_raw(Box::<i32, Kmalloc>::new(42));
    // SAFETY: `ptr` came from `Box::into_raw` and thus is pointing to a
    // valid and existing memory allocation allocated by `Kmalloc`.
    unsafe { Kmalloc::free(ptr) };
    // SAFETY: `ptr` came from `Box::into_raw` and thus is pointing at a
    // valid `i32`.
    let v = unsafe { ptr.read() };

Also see the from_raw for our Arc:

    /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
    ///
    /// # Safety
    ///
    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
    /// must not be called more than once for each previous call to [`Arc::into_raw`].
    pub unsafe fn from_raw(ptr: *const T) -> Self {

That also requires that the function must not be called more than once. This reminds me, I forgot to say that about Box::from_raw.

---
Cheers,
Benno

Quoted 28 lines · Benno Lossin

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

rust/kernel/alloc.rs +18 -0

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

Quoted 6 lines

Quoted 2 lines

Additionally, you need "The memory allocation at ptr must never again be read from or written to.".

I'm fine adding it, but I wonder if technically this is really required? The condition whether the pointer is ever accessed again in any way is not relevant in terms of being a precondition for free not causing UB, right?

I don't see how else we would find the mistake in the following code:

Quoted 7 lines

    let ptr = Box::into_raw(Box::<i32, Kmalloc>::new(42));
    // SAFETY: `ptr` came from `Box::into_raw` and thus is pointing to a
    // valid and existing memory allocation allocated by `Kmalloc`.
    unsafe { Kmalloc::free(ptr) };
    // SAFETY: `ptr` came from `Box::into_raw` and thus is pointing at a
    // valid `i32`.
    let v = unsafe { ptr.read() };

Sure, but what I mean is that my understanding is that the "Safety" section in a comment describes the requirements of the function it documents. I.e. free itself doesn't care whether the pointer is read or writted ever again.

Or in other words, what are the rules where this belongs to? E.g. why not document this exact aspect in the safety section of Allocator?

Quoted 10 lines

Also see the from_raw for our Arc:

    /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
    ///
    /// # Safety
    ///
    /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it
    /// must not be called more than once for each previous call to [`Arc::into_raw`].
    pub unsafe fn from_raw(ptr: *const T) -> Self {

Quoted 2 lines

That also requires that the function must not be called more than once. This reminds me, I forgot to say that about Box::from_raw.

Indeed, I also wonder if we ever have cases where C code gives us ownership of a memory allocation of a certain type that fulfills the requirements we have for a Box, such that Rust code is tempted to pass it to Box::from_raw.

It sounds a bit scary design wise, but in theory it's possible.

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.

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

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

rust/kernel/alloc.rs +46 -0

index 1966bd407017..8a71a589469d 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,81 @@ 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
+///
+/// Memory returned from an allocator must point to a valid memory buffer and remain valid until
+/// it is explicitly freed.
+///
+/// Any pointer to a memory buffer which is currently allocated must be valid to be passed to any
+/// other [`Allocator`] function of the same type. The same applies for a NULL pointer.
+///
+/// If `realloc` is called with:
+///   - a size of zero, the given memory allocation, if any, must be freed
+///   - a NULL pointer, a new memory allocation must be created
+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 a NULL pointer.
+    fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
+        // SAFETY: Passing a NULL pointer 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

rust/kernel/alloc.rs +7 -0

+    /// 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.
+    ///
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,

rust/kernel/alloc.rs +6 -0

+    unsafe fn free(ptr: NonNull<u8>) {
+        // SAFETY: `ptr` is guaranteed to be previously allocated with this `Allocator` or NULL.
+        // Calling `realloc` with a buffer size of zero, frees the buffer `ptr` points to.
+        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]>

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

---
 rust/kernel/alloc/allocator.rs | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

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

index e6ea601f38c6..e7b7eba84acb 100644
@@ -8,27 +8,36 @@
 
 struct KernelAllocator;
 
-/// Calls `krealloc` with a proper size to alloc a new object aligned to `new_layout`'s alignment.
-///
-/// # Safety
-///
-/// - `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 {
+/// 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).
-    let size = layout.size();
+    layout.size()
+}
 
+/// Calls `krealloc` with a proper size to alloc a new object aligned to `new_layout`'s alignment.
+///
+/// # Safety
+///
+/// - `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 {

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

+            flags.0,
+        ) as *mut u8
+    }
 }
 
 unsafe impl GlobalAlloc for KernelAllocator {
--

2.45.2

Subsequent patches implement Vmalloc and KVmalloc allocators, hence align KernelAllocator to this naming scheme.

Reviewed-by Alice Ryhl <[email protected]>

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

---
 rust/kernel/alloc/allocator.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Subsequent patches implement Vmalloc and KVmalloc allocators, hence align KernelAllocator to this naming scheme.

Reviewed-by Alice Ryhl <[email protected]>

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

---
 rust/kernel/alloc/allocator.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index e7b7eba84acb..2c1eae25da84 100644
@@ -6,7 +6,7 @@
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;
 
-struct KernelAllocator;
+struct Kmalloc;
 
 /// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
 fn aligned_size(new_layout: Layout) -> usize {

@@ -40,7 +40,7 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     }
 }
 
-unsafe impl GlobalAlloc for KernelAllocator {
+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
         // requirement.

@@ -76,7 +76,7 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
 }
 
 #[global_allocator]
-static ALLOCATOR: KernelAllocator = KernelAllocator;
+static ALLOCATOR: Kmalloc = Kmalloc;
 
 // See <https://github.com/rust-lang/rust/pull/86844>.
 #[no_mangle]

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/helpers.c                 |  3 +-
 rust/kernel/alloc.rs           |  2 +-
 rust/kernel/alloc/allocator.rs | 68 +++++++++++++++++++++++++++++++++-
 3 files changed, 69 insertions(+), 4 deletions(-)

rust/helpers.c +1 -2

index 92d3c03ae1bd..9f7275493365 100644
@@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
 }
 EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
 
-void * __must_check __realloc_size(2)
-rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
+void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
 {
 	return krealloc(objp, new_size, flags);
 }

rust/kernel/alloc.rs +1 -1

index 8a71a589469d..bc01a17df5e0 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 +9 -1

index 2c1eae25da84..c6ad1dd59dd0 100644
@@ -5,8 +5,16 @@
 use super::{flags::*, Flags};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;
+use core::ptr::NonNull;
 
-struct Kmalloc;
+use crate::alloc::{AllocError, Allocator};
+use crate::bindings;
+
+/// The contiguous kernel allocator.
+///
+/// The contiguous kernel allocator only ever allocates physically contiguous memory through
+/// `bindings::krealloc`.
+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 {

@@ -40,6 +48,64 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     }
 }

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

+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };
+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+

rust/kernel/alloc/allocator.rs +3 -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.45.2

Quoted 67 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/helpers.c                 |  3 +-
 rust/kernel/alloc.rs           |  2 +-
 rust/kernel/alloc/allocator.rs | 68 +++++++++++++++++++++++++++++++++-
 3 files changed, 69 insertions(+), 4 deletions(-)

rust/helpers.c +1 -2

index 92d3c03ae1bd..9f7275493365 100644
@@ -193,8 +193,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
 }
 EXPORT_SYMBOL_GPL(rust_helper_init_work_with_key);
 
-void * __must_check __realloc_size(2)
-rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
+void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
 {
 	return krealloc(objp, new_size, flags);
 }

rust/kernel/alloc.rs +1 -1

index 8a71a589469d..bc01a17df5e0 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 +9 -1

index 2c1eae25da84..c6ad1dd59dd0 100644
@@ -5,8 +5,16 @@
 use super::{flags::*, Flags};
 use core::alloc::{GlobalAlloc, Layout};
 use core::ptr;
+use core::ptr::NonNull;
 
-struct Kmalloc;
+use crate::alloc::{AllocError, Allocator};
+use crate::bindings;
+
+/// The contiguous kernel allocator.
+///
+/// The contiguous kernel allocator only ever allocates physically contiguous memory through
+/// `bindings::krealloc`.
+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 {

@@ -40,6 +48,64 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: F
     }
 }

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

Quoted 10 lines

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

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

This INVARIANT comment should be moved one line downwards.

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

Quoted 7 lines

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

+    fn krealloc() -> Self {
+        Self(bindings::krealloc)
+    }
+
+    /// # Safety
+    ///
+    /// This method has the exact same safety requirements as `Allocator::realloc`.

I would remove "exact", I don't think we want to mean "almost the same" when we write just "same".

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

Quoted 13 lines

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

+    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 {
+            Some(ptr) => ptr.as_ptr(),
+            None => ptr::null(),
+        };
+
+        // SAFETY: `ptr` is valid by the safety requirements of this function.

"ptr is either NULL or valid by the safety requirements of this function."

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

Quoted 8 lines

rust/kernel/alloc/allocator.rs +7 -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()

If we call realloc(Some(ptr), <layout with size = 0>, ...), then this leaks the pointer returned by the call to self.0 above. I don't know what the return value of the different functions are that can appear in self.0, do they return NULL?

What about the following sequence:

    let ptr = realloc(None, <layout with size = 0>, ...);
    let ptr = realloc(Some(ptr), <layout with size = 0>, ...);

Then the above call to self.0 is done with a dangling pointer, can the functions that appear in self.0 handle that?

Quoted 14 lines

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

+        } else {
+            NonNull::new(raw_ptr).ok_or(AllocError)?
+        };
+
+        Ok(NonNull::slice_from_raw_parts(ptr, size))
+    }
+}
+
+unsafe impl Allocator for Kmalloc {
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,

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

Quoted 5 lines

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

+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let realloc = ReallocFunc::krealloc();
+
+        // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously
+        // allocated with this `Allocator`.

What about the other requirements? (they should be satisfied, since they are also requirements for calling this function)

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

Quoted 2 lines

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

+        unsafe { realloc.call(ptr, layout, flags) }

If you make ReallocFunc::krealloc() into a constant ReallocFunc::KREALLOC, then we could avoid the let binding above.

---
Cheers,
Benno

Quoted 9 lines

rust/kernel/alloc/allocator.rs +3 -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.45.2

Quoted 9 lines · Benno Lossin

Quoted 7 lines · Danilo Krummrich

rust/kernel/alloc/allocator.rs +7 -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()

Quoted 4 lines

If we call realloc(Some(ptr), <layout with size = 0>, ...), then this leaks the pointer returned by the call to self.0 above. I don't know what the return value of the different functions are that can appear in self.0, do they return NULL?

That is fine, we don't care about the return value. All ReallocFunc free the memory behind ptr if called with a size of zero. But to answer the question, they return either NULL or ZERO_SIZE_PTR.

Quoted 5 lines

What about the following sequence:

    let ptr = realloc(None, <layout with size = 0>, ...);
    let ptr = realloc(Some(ptr), <layout with size = 0>, ...);

Quoted 2 lines

Then the above call to self.0 is done with a dangling pointer, can the functions that appear in self.0 handle that?

This would be incorrect.

Calling realloc(Some(ptr), <layout with size = 0>, ...) frees the memory behind ptr. This is guranteed behavior for all ReallocFuncs, i.e. krealloc(), vrealloc(), kvrealloc().

Quoted 10 lines · Danilo Krummrich

Quoted 9 lines · Benno Lossin

Quoted 7 lines · Danilo Krummrich

rust/kernel/alloc/allocator.rs +7 -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()

Quoted 8 lines

Quoted 4 lines

If we call realloc(Some(ptr), <layout with size = 0>, ...), then this leaks the pointer returned by the call to self.0 above. I don't know what the return value of the different functions are that can appear in self.0, do they return NULL?

That is fine, we don't care about the return value. All ReallocFunc free the memory behind ptr if called with a size of zero. But to answer the question, they return either NULL or ZERO_SIZE_PTR.

I see, then it's fine. I think it would help if we know the exact behavior of kmalloc & friends (either add a link to C docs or write it down on ReallocFunc).

Quoted 5 lines

Quoted 5 lines

What about the following sequence:

    let ptr = realloc(None, <layout with size = 0>, ...);
    let ptr = realloc(Some(ptr), <layout with size = 0>, ...);

Quoted 8 lines

Quoted 2 lines

Then the above call to self.0 is done with a dangling pointer, can the functions that appear in self.0 handle that?

This would be incorrect.

Calling realloc(Some(ptr), <layout with size = 0>, ...) frees the memory behind ptr. This is guranteed behavior for all ReallocFuncs, i.e. krealloc(), vrealloc(), kvrealloc().

Note that I don't use ptr afterwards, the code snippet above is equivalent to this:

    let ptr = Kmalloc::alloc(<layout with size = 0>, ...);
    unsafe { Kmalloc::free(ptr) };

internally exactly the realloc calls that I put above should be called.

---
Cheers,
Benno

Quoted 44 lines · Benno Lossin

Quoted 10 lines · Danilo Krummrich

Quoted 9 lines · Benno Lossin

Quoted 7 lines · Danilo Krummrich

rust/kernel/alloc/allocator.rs +7 -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()

Quoted 8 lines

Quoted 4 lines

If we call realloc(Some(ptr), <layout with size = 0>, ...), then this leaks the pointer returned by the call to self.0 above. I don't know what the return value of the different functions are that can appear in self.0, do they return NULL?

That is fine, we don't care about the return value. All ReallocFunc free the memory behind ptr if called with a size of zero. But to answer the question, they return either NULL or ZERO_SIZE_PTR.

I see, then it's fine. I think it would help if we know the exact behavior of kmalloc & friends (either add a link to C docs or write it down on ReallocFunc).

Quoted 5 lines

Quoted 5 lines

What about the following sequence:

    let ptr = realloc(None, <layout with size = 0>, ...);
    let ptr = realloc(Some(ptr), <layout with size = 0>, ...);

Quoted 8 lines

Quoted 2 lines

Then the above call to self.0 is done with a dangling pointer, can the functions that appear in self.0 handle that?

This would be incorrect.

Calling realloc(Some(ptr), <layout with size = 0>, ...) frees the memory behind ptr. This is guranteed behavior for all ReallocFuncs, i.e. krealloc(), vrealloc(), kvrealloc().

Note that I don't use ptr afterwards, the code snippet above is equivalent to this:

    let ptr = Kmalloc::alloc(<layout with size = 0>, ...);
    unsafe { Kmalloc::free(ptr) };

Quoted 1 line

internally exactly the realloc calls that I put above should be called.

I think I misunderstood what you mean here.

So, that's not permitted. free can't be called with a dangling pointer. The kernel free functions (*1) can't handle it, and I can't detect it, since a dangling pointer does not have a descrete value.

We can decide for a specific dangling pointer to be allowed, i.e. the dangling pointer returned by alloc for a zero sized allocation is always dangling<u8>, so we can assert that free is only allowed to be called with what was previously returned by alloc or free and therefore disallow dangling pointers with a different alignment.

Surely, we could also let the caller pass the old alignment, but this all sounds complicated for something that is very trivial for the caller to take care of, i.e. just don't try to free something that was never actually allocated.

It can also lead to subtle bugs, e.g. what if someone calls Box::from_raw for a ZST with some other random pointer? Currently, that doesn't hurt us, which for robustness, seems to be a good thing.

I think it's better to just let Box and Vec figure out if calling free is the right thing to do. The code for that is simple and obvious, i.e. check if T is a ZST.

*1: kfree() can handle dangling pointers up to 16 bytes aligned, see ZERO_OR_NULL_PTR(x).

Quoted 3 lines

So, that's not permitted. free can't be called with a dangling pointer. The kernel free functions (*1) can't handle it, and I can't detect it, since a dangling pointer does not have a descrete value.

That is true, but only if we do not have access to the old layout of the allocation. If we add old_layout as a parameter, then we can handle dangling pointers.

Quoted 4 lines

Surely, we could also let the caller pass the old alignment, but this all sounds complicated for something that is very trivial for the caller to take care of, i.e. just don't try to free something that was never actually allocated.

Quoted 3 lines

It can also lead to subtle bugs, e.g. what if someone calls Box::from_raw for a ZST with some other random pointer? Currently, that doesn't hurt us, which for robustness, seems to be a good thing.

I think we should forbid that. To me it's just plain wrong to take a random integer literal and cast it to a ZST. IIRC it even is UB if that points to a previously allocated object that has been freed (but I don't remember where I read it...).

Also if we use the size of the old layout instead of comparing alignment with the address of the pointer, then we avoid this issue.

Quoted 4 lines · Benno Lossin

Quoted 3 lines

So, that's not permitted. free can't be called with a dangling pointer. The kernel free functions (*1) can't handle it, and I can't detect it, since a dangling pointer does not have a descrete value.

Quoted 3 lines

That is true, but only if we do not have access to the old layout of the allocation. If we add old_layout as a parameter, then we can handle dangling pointers.

Then we'd need free to be fn free(ptr: NonNull<u8>, layout: Layout) just to compare whether ptr.as_ptr() == layout.align() as _. Same for realloc, but that's less weird.

It's also not that we safe code with that. Box, Vec, any other user, still would have to create the Layout before they call A::free.

Quoted 4 lines

Quoted 4 lines

Surely, we could also let the caller pass the old alignment, but this all sounds complicated for something that is very trivial for the caller to take care of, i.e. just don't try to free something that was never actually allocated.

Quoted 8 lines

Quoted 3 lines

It can also lead to subtle bugs, e.g. what if someone calls Box::from_raw for a ZST with some other random pointer? Currently, that doesn't hurt us, which for robustness, seems to be a good thing.

I think we should forbid that. To me it's just plain wrong to take a random integer literal and cast it to a ZST. IIRC it even is UB if that points to a previously allocated object that has been freed (but I don't remember where I read it...).

I think my argument about robustness still holds even if we forbid it.

The documentation says "For operations of size zero, every pointer is valid, including the null pointer." [1]

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

Quoted 3 lines

Also if we use the size of the old layout instead of comparing alignment with the address of the pointer, then we avoid this issue.

That is just another problem when passing the old Layout (or maybe just the old size as usize). Neither do we need the old size, nor do we honor it with any kernel allocator. This has the following implications:

(1) We never see any error if the old size that is passed is garbage (unless it's non-zero, when it should be zero and vice versa), which is bad.

(2) We'd need free to be fn free(ptr: NonNull<u8>, size: usize), which is confusing, because it implies that an actual free relies on this size for freeing the memory.

If we want to avoid (1) and (2), we'd need to make it
fn free(ptr: NonNull<u8>, zero: bool) instead, but then also the caller can just check this boolean and conditionally call free.

I don't really see why it's better to let free do the if !zero check.

Quoted 3 lines

I think it's better to just let Box and Vec figure out if calling free is the right thing to do. The code for that is simple and obvious, i.e. check if T is a ZST.

I don't think that it is as simple as you make it out to be. To me this is functionality that we can move into one place and never have to think about again.
Also if we at some point decide to add some sort of debugging allocator (am I right in assuming that such a thing already exists for C?) then we might want to tag on extra data in the allocation, in that case it would be very useful if the allocator also saw zero-sized allocations, since we should check all allocations.

Quoted 2 lines · Danilo Krummrich

It's also not that we safe code with that. Box, Vec, any other user, still would have to create the Layout before they call A::free.

But for Box it would just be Layout::<T>::new() and Vec needs Layout::<T>::new().repeat(self.cap).

Though for repeat we need the alloc_layout_extra feature, which is an argument against doing this.

Quoted 9 lines · Danilo Krummrich

Quoted 7 lines · Benno Lossin

Quoted 3 lines

So, that's not permitted. free can't be called with a dangling pointer. The kernel free functions (*1) can't handle it, and I can't detect it, since a dangling pointer does not have a descrete value.

That is true, but only if we do not have access to the old layout of the allocation. If we add old_layout as a parameter, then we can handle dangling pointers.

Quoted 3 lines

Then we'd need free to be fn free(ptr: NonNull<u8>, layout: Layout) just to compare whether ptr.as_ptr() == layout.align() as _. Same for realloc, but that's less weird.

I don't think that's a problem (though we would check against the size and not compare the address!). Allocator from stdlib also has the extra argument.

Quoted 3 lines

It's also not that we safe code with that. Box, Vec, any other user, still would have to create the Layout before they call A::free.

But for Box it would just be Layout::<T>::new() and Vec needs Layout::<T>::new().repeat(self.cap).

Though for repeat we need the alloc_layout_extra feature, which is an argument against doing this.

Quoted 15 lines

Quoted 4 lines

Quoted 4 lines

Surely, we could also let the caller pass the old alignment, but this all sounds complicated for something that is very trivial for the caller to take care of, i.e. just don't try to free something that was never actually allocated.

Quoted 8 lines

Quoted 3 lines

It can also lead to subtle bugs, e.g. what if someone calls Box::from_raw for a ZST with some other random pointer? Currently, that doesn't hurt us, which for robustness, seems to be a good thing.

I think we should forbid that. To me it's just plain wrong to take a random integer literal and cast it to a ZST. IIRC it even is UB if that points to a previously allocated object that has been freed (but I don't remember where I read it...).

I think my argument about robustness still holds even if we forbid it.

Quoted 2 lines

The documentation says "For operations of size zero, every pointer is valid, including the null pointer." [1]

How does this increase robustness? I am not allowed to call free with a random pointer, only pointers returned by that allocator. The same should also be true for Box::from_raw. That way the ZST dangling pointer stuff leaks into that API.

Quoted 7 lines

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

Quoted 3 lines

Also if we use the size of the old layout instead of comparing alignment with the address of the pointer, then we avoid this issue.

Quoted 6 lines

That is just another problem when passing the old Layout (or maybe just the old size as usize). Neither do we need the old size, nor do we honor it with any kernel allocator. This has the following implications:

(1) We never see any error if the old size that is passed is garbage (unless it's non-zero, when it should be zero and vice versa), which is bad.

We could add debug support for that though?

Quoted 4 lines

(2) We'd need free to be fn free(ptr: NonNull<u8>, size: usize), which is confusing, because it implies that an actual free relies on this size for freeing the memory.

I don't think that it is confusing to ask for the old layout.

Quoted 4 lines

If we want to avoid (1) and (2), we'd need to make it
fn free(ptr: NonNull<u8>, zero: bool) instead, but then also the caller can just check this boolean and conditionally call free.

Yeah having free with a zero: bool sounds like a bad idea.

Quoted 1 line

I don't really see why it's better to let free do the if !zero check.

You did not reply to my last argument:

Quoted 9 lines

Quoted 3 lines

I think it's better to just let Box and Vec figure out if calling free is the right thing to do. The code for that is simple and obvious, i.e. check if T is a ZST.

I don't think that it is as simple as you make it out to be. To me this is functionality that we can move into one place and never have to think about again.
Also if we at some point decide to add some sort of debugging allocator (am I right in assuming that such a thing already exists for C?) then we

Quoted 3 lines

might want to tag on extra data in the allocation, in that case it would be very useful if the allocator also saw zero-sized allocations, since we should check all allocations.

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

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 bc01a17df5e0..942e2755f217 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 +7 -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;

rust/kernel/alloc/allocator_test.rs new file +2 -1

+    }
+}
--

2.45.2

Quoted 7 lines · Danilo Krummrich

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.

This is confusing, since you are talking about both our new Allocator trait, allocators and the alloc crate Allocator.

Quoted 50 lines

Reviewed-by Alice Ryhl <[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 bc01a17df5e0..942e2755f217 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 +11 -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 {

rust/kernel/alloc/allocator_test.rs line 17

Quoted 6 lines

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

+    unsafe fn realloc(
+        _ptr: Option<NonNull<u8>>,
+        _layout: Layout,
+        _flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        panic!();

Does build_error!() also work? If yes, then I would prefer that.

---
Cheers,
Benno

Quoted 5 lines

rust/kernel/alloc/allocator_test.rs new file +2 -1

+    }
+}
--

2.45.2

Quoted 5 lines · Benno Lossin

Quoted 4 lines · Danilo Krummrich

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.

Quoted 6 lines

Quoted 3 lines

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.

This is confusing, since you are talking about both our new Allocator trait, allocators and the alloc crate Allocator.

I never mention the alloc crate Allocator here.

Quoted 49 lines

Quoted 49 lines

Reviewed-by Alice Ryhl <[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 bc01a17df5e0..942e2755f217 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 +11 -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 {

Quoted 8 lines

rust/kernel/alloc/allocator_test.rs line 17

Quoted 6 lines

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

+    unsafe fn realloc(
+        _ptr: Option<NonNull<u8>>,
+        _layout: Layout,
+        _flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        panic!();

Does build_error!() also work? If yes, then I would prefer that.

Probably, but it also probably doesn't matter too much. A later patch in the series replaces this with a Cmalloc implementation and type aliases all kernel allocators (e.g. Kmalloc) to it.

Quoted 6 lines · Danilo Krummrich

Quoted 5 lines · Benno Lossin

Quoted 4 lines · Danilo Krummrich

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.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

This is confusing, since you are talking about both our new Allocator trait, allocators and the alloc crate Allocator.

I never mention the alloc crate Allocator here.

Seems like I confused myself...

Quoted 58 lines

Quoted 57 lines

Quoted 45 lines

Reviewed-by Alice Ryhl <[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 bc01a17df5e0..942e2755f217 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 +7 -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;

rust/kernel/alloc/allocator_test.rs line 17

Quoted 10 lines

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

+
+pub struct Kmalloc;
+
+unsafe impl Allocator for Kmalloc {
+    unsafe fn realloc(
+        _ptr: Option<NonNull<u8>>,
+        _layout: Layout,
+        _flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        panic!();

Does build_error!() also work? If yes, then I would prefer that.

Quoted 3 lines

Probably, but it also probably doesn't matter too much. A later patch in the series replaces this with a Cmalloc implementation and type aliases all kernel allocators (e.g. Kmalloc) to it.

What prevents you from doing the Cmalloc patch here? build_error! probably doesn't work, since we probably allocate in rusttest, right?

---
Cheers,
Benno

Implement Allocator for Vmalloc, the kernel's virtually contiguous allocator, typically used for larger objects, (much) larger than page size.

All memory allocations made with Vmalloc end up in vrealloc().

Reviewed-by Alice Ryhl <[email protected]>

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

---
 rust/helpers.c                      |  7 +++++++
 rust/kernel/alloc/allocator.rs      | 32 +++++++++++++++++++++++++++++
 rust/kernel/alloc/allocator_test.rs |  1 +
 3 files changed, 40 insertions(+)

rust/helpers.c +7 -0

index 9f7275493365..7406943f887d 100644
@@ -33,6 +33,7 @@
 #include <linux/sched/signal.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/vmalloc.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 

@@ -199,6 +200,12 @@ void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
 }
 EXPORT_SYMBOL_GPL(rust_helper_krealloc);
 
+void *rust_helper_vrealloc(const void *p, size_t size, gfp_t flags)
+{
+	return vrealloc(p, size, flags);
+}
+EXPORT_SYMBOL_GPL(rust_helper_vrealloc);
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.

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

index c6ad1dd59dd0..bb55895cbd03 100644
@@ -9,6 +9,7 @@
 
 use crate::alloc::{AllocError, Allocator};
 use crate::bindings;
+use crate::pr_warn;
 
 /// The contiguous kernel allocator.
 ///

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

+    fn vrealloc() -> Self {
+        Self(bindings::vrealloc)
+    }
+
     /// # Safety
     ///
     /// This method has the exact same safety requirements as `Allocator::realloc`.

@@ -141,6 +153,26 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
     }
 }
 
+unsafe impl Allocator for Vmalloc {
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let realloc = ReallocFunc::vrealloc();

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

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

Quoted 55 lines · Danilo Krummrich

Implement Allocator for Vmalloc, the kernel's virtually contiguous allocator, typically used for larger objects, (much) larger than page size.

All memory allocations made with Vmalloc end up in vrealloc().

Reviewed-by Alice Ryhl <[email protected]>

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

---
 rust/helpers.c                      |  7 +++++++
 rust/kernel/alloc/allocator.rs      | 32 +++++++++++++++++++++++++++++
 rust/kernel/alloc/allocator_test.rs |  1 +
 3 files changed, 40 insertions(+)

rust/helpers.c +7 -0

index 9f7275493365..7406943f887d 100644
@@ -33,6 +33,7 @@
 #include <linux/sched/signal.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/vmalloc.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 

@@ -199,6 +200,12 @@ void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
 }
 EXPORT_SYMBOL_GPL(rust_helper_krealloc);
 
+void *rust_helper_vrealloc(const void *p, size_t size, gfp_t flags)
+{
+	return vrealloc(p, size, flags);
+}
+EXPORT_SYMBOL_GPL(rust_helper_vrealloc);
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.

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

index c6ad1dd59dd0..bb55895cbd03 100644
@@ -9,6 +9,7 @@
 
 use crate::alloc::{AllocError, Allocator};
 use crate::bindings;
+use crate::pr_warn;
 
 /// The contiguous kernel allocator.
 ///

@@ -16,6 +17,12 @@
 /// `bindings::krealloc`.
 pub struct Kmalloc;
 

rust/kernel/alloc/allocator.rs line 24

Quoted 5 lines

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

+/// The virtually contiguous kernel allocator.
+///
+/// The vmalloc allocator allocates pages from the page level allocator and maps them into the
+/// contiguous kernel virtual space.
+pub struct Vmalloc;

One thing that I should also have mentioned for Kmalloc, do we want these types to also have values? I don't think that we need them to be, so we could declare them as pub enum Vmalloc {}.

rust/kernel/alloc/allocator.rs fn krealloc() -> Self { · line 71

Quoted 10 lines

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

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

@@ -61,6 +68,11 @@ fn krealloc() -> Self {
         Self(bindings::krealloc)
     }
 
+    // INVARIANT: `vrealloc` satisfies the type invariants.

This should be moved down a line.

---
Cheers,
Benno

Quoted 24 lines

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

+    fn vrealloc() -> Self {
+        Self(bindings::vrealloc)
+    }
+
     /// # Safety
     ///
     /// This method has the exact same safety requirements as `Allocator::realloc`.

@@ -141,6 +153,26 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
     }
 }
 
+unsafe impl Allocator for Vmalloc {
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let realloc = ReallocFunc::vrealloc();
+
+        // 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);
+        }

rust/kernel/alloc/allocator.rs unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { · line 172

Quoted 4 lines

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

+
+        // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously
+        // allocated with this `Allocator`.
+        unsafe { realloc.call(ptr, layout, flags) }

I am a bit confused, for Kmalloc, you manually returned NonNull::dangling when allocating a zero-sized allocation, but here you don't?

---
Cheers,
Benno

Quoted 21 lines

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

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

Quoted 56 lines · Benno Lossin

Quoted 55 lines · Danilo Krummrich

Implement Allocator for Vmalloc, the kernel's virtually contiguous allocator, typically used for larger objects, (much) larger than page size.

All memory allocations made with Vmalloc end up in vrealloc().

Reviewed-by Alice Ryhl <[email protected]>

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

---
 rust/helpers.c                      |  7 +++++++
 rust/kernel/alloc/allocator.rs      | 32 +++++++++++++++++++++++++++++
 rust/kernel/alloc/allocator_test.rs |  1 +
 3 files changed, 40 insertions(+)

rust/helpers.c +7 -0

index 9f7275493365..7406943f887d 100644
@@ -33,6 +33,7 @@
 #include <linux/sched/signal.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/vmalloc.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 

@@ -199,6 +200,12 @@ void *rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
 }
 EXPORT_SYMBOL_GPL(rust_helper_krealloc);
 
+void *rust_helper_vrealloc(const void *p, size_t size, gfp_t flags)
+{
+	return vrealloc(p, size, flags);
+}
+EXPORT_SYMBOL_GPL(rust_helper_vrealloc);
+
 /*
  * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can
  * use it in contexts where Rust expects a `usize` like slice (array) indices.

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

index c6ad1dd59dd0..bb55895cbd03 100644
@@ -9,6 +9,7 @@
 
 use crate::alloc::{AllocError, Allocator};
 use crate::bindings;
+use crate::pr_warn;
 
 /// The contiguous kernel allocator.
 ///

@@ -16,6 +17,12 @@
 /// `bindings::krealloc`.
 pub struct Kmalloc;
 

Quoted 9 lines

rust/kernel/alloc/allocator.rs line 24

Quoted 5 lines

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

+/// The virtually contiguous kernel allocator.
+///
+/// The vmalloc allocator allocates pages from the page level allocator and maps them into the
+/// contiguous kernel virtual space.
+pub struct Vmalloc;

One thing that I should also have mentioned for Kmalloc, do we want these types to also have values? I don't think that we need them to be, so we could declare them as pub enum Vmalloc {}.

What the difference? Would pub enum Vmalloc {} be better for some reason?

Quoted 17 lines

Quoted 17 lines

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

@@ -141,6 +153,26 @@ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
     }
 }
 
+unsafe impl Allocator for Vmalloc {
+    unsafe fn realloc(
+        ptr: Option<NonNull<u8>>,
+        layout: Layout,
+        flags: Flags,
+    ) -> Result<NonNull<[u8]>, AllocError> {
+        let realloc = ReallocFunc::vrealloc();
+
+        // 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);
+        }

Quoted 9 lines

rust/kernel/alloc/allocator.rs unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { · line 172

Quoted 4 lines

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

+
+        // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously
+        // allocated with this `Allocator`.
+        unsafe { realloc.call(ptr, layout, flags) }

I am a bit confused, for Kmalloc, you manually returned NonNull::dangling when allocating a zero-sized allocation, but here you don't?

I do, it's the exact same implementation for krealloc(), vrealloc() and kvrealloc(). That why I added the ReallocFunc abstraction.