Message-ID: <20240921153315.70860-1-dakr@kernel.org>
Date: Sat, 21 Sep 2024 17:32:46 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: ojeda@...nel.org,
	alex.gaynor@...il.com,
	wedsonaf@...il.com,
	boqun.feng@...il.com,
	gary@...yguo.net,
	bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me,
	a.hindborg@...sung.com,
	aliceryhl@...gle.com,
	akpm@...ux-foundation.org
Cc: daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com,
	boris.brezillon@...labora.com,
	lina@...hilina.net,
	mcanal@...lia.com,
	zhiw@...dia.com,
	cjia@...dia.com,
	jhubbard@...dia.com,
	airlied@...hat.com,
	ajanulgu@...hat.com,
	lyude@...hat.com,
	linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org,
	linux-mm@...ck.org,
	Danilo Krummrich <dakr@...nel.org>
Subject: [RFC PATCH] rust: alloc: pass `old_layout` to `Allocator`

Since this came up a few times, this patch shows how the implementation
looks like with an `old_layout` argument.

Signed-off-by: Danilo Krummrich <dakr@...nel.org>
---
 rust/kernel/alloc.rs           | 32 ++++++++++++--------------
 rust/kernel/alloc/allocator.rs | 28 +++++++++++++++++++----
 rust/kernel/alloc/kbox.rs      | 13 ++++++-----
 rust/kernel/alloc/kvec.rs      | 42 +++++++++++++++++++++++++++-------
 4 files changed, 78 insertions(+), 37 deletions(-)

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index 2170b53acd0c..78564eeb987d 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -166,7 +166,7 @@ pub unsafe trait Allocator {
     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) }
+        unsafe { Self::realloc(None, layout, Layout::new::<()>(), flags) }
     }
 
     /// Re-allocate an existing memory allocation to satisfy the requested `layout`.
@@ -186,26 +186,23 @@ fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
     ///
     /// # 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.
+    /// - If `ptr == Some(p)`, then `p` must point to an existing and valid memory allocation
+    ///   created by this allocator.
+    /// - `ptr` is allowed to be `None`; in this case a new memory allocation is created.
+    /// - `old_layout` must match the `Layout` the allocation has been created with.
     ///
     /// # Guarantees
     ///
     /// This function has the same guarantees as [`Allocator::alloc`]. When `ptr == Some(p)`, then
     /// it additionally guarantees that:
     /// - the contents of the memory pointed to by `p` are preserved up to the lesser of the new
-    ///   and old size,
-    ///   and old size, i.e.
-    ///   `ret_ptr[0..min(layout.size(), old_size)] == p[0..min(layout.size(), old_size)]`, where
-    ///   `old_size` is the size of the allocation that `p` points at.
-    /// - when the return value is `Err(AllocError)`, then `p` is still valid.
+    ///   and old size, i.e. `ret_ptr[0..min(layout.size(), old_layout.size())] ==
+    ///   p[0..min(layout.size(), old_layout.size())]`.
+    /// - when the return value is `Err(AllocError)`, then `ptr` is still valid.
     unsafe fn realloc(
         ptr: Option<NonNull<u8>>,
         layout: Layout,
+        old_layout: Layout,
         flags: Flags,
     ) -> Result<NonNull<[u8]>, AllocError>;
 
@@ -213,14 +210,13 @@ unsafe fn realloc(
     ///
     /// # 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>) {
+    /// - `ptr` must point to an existing and valid memory allocation created by this `Allocator`.
+    /// - `layout` must match the `Layout` the allocation has been created with.
+    /// - The memory allocation at `ptr` must never again be read from or written to.
+    unsafe fn free(ptr: NonNull<u8>, layout: Layout) {
         // 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)) };
+        let _ = unsafe { Self::realloc(Some(ptr), Layout::new::<()>(), layout, Flags(0)) };
     }
 }
diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
index 0b586c0361f4..07820c8c4e17 100644
--- a/rust/kernel/alloc/allocator.rs
+++ b/rust/kernel/alloc/allocator.rs
@@ -54,6 +54,14 @@ fn aligned_size(new_layout: Layout) -> usize {
     layout.size()
 }
 
+/// Returns a properly aligned dangling pointer from the given `layout`.
+fn zst_realloc(layout: Layout) -> NonNull<u8> {
+    let ptr = layout.align() as *mut u8;
+
+    // SAFETY: `layout.align()` (and hence `ptr`) is guaranteed to be non-zero.
+    unsafe { NonNull::new_unchecked(ptr) }
+}
+
 /// # Invariants
 ///
 /// One of the following `krealloc`, `vrealloc`, `kvrealloc`.
@@ -84,11 +92,18 @@ unsafe fn call(
         &self,
         ptr: Option<NonNull<u8>>,
         layout: Layout,
+        old_layout: Layout,
         flags: Flags,
     ) -> Result<NonNull<[u8]>, AllocError> {
         let size = aligned_size(layout);
         let ptr = match ptr {
-            Some(ptr) => ptr.as_ptr(),
+            Some(ptr) => {
+                if old_layout.size() == 0 {
+                    ptr::null()
+                } else {
+                    ptr.as_ptr()
+                }
+            }
             None => ptr::null(),
         };
 
@@ -106,7 +121,7 @@ unsafe fn call(
         };
 
         let ptr = if size == 0 {
-            NonNull::dangling()
+            zst_realloc(layout)
         } else {
             NonNull::new(raw_ptr).ok_or(AllocError)?
         };
@@ -124,10 +139,11 @@ unsafe impl Allocator for Kmalloc {
     unsafe fn realloc(
         ptr: Option<NonNull<u8>>,
         layout: Layout,
+        old_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 { ReallocFunc::KREALLOC.call(ptr, layout, old_layout, flags) }
     }
 }
 
@@ -140,6 +156,7 @@ unsafe impl Allocator for Vmalloc {
     unsafe fn realloc(
         ptr: Option<NonNull<u8>>,
         layout: Layout,
+        old_layout: Layout,
         flags: Flags,
     ) -> Result<NonNull<[u8]>, AllocError> {
         // TODO: Support alignments larger than PAGE_SIZE.
@@ -150,7 +167,7 @@ unsafe fn realloc(
 
         // 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) }
+        unsafe { ReallocFunc::VREALLOC.call(ptr, layout, old_layout, flags) }
     }
 }
 
@@ -163,6 +180,7 @@ unsafe impl Allocator for KVmalloc {
     unsafe fn realloc(
         ptr: Option<NonNull<u8>>,
         layout: Layout,
+        old_layout: Layout,
         flags: Flags,
     ) -> Result<NonNull<[u8]>, AllocError> {
         // TODO: Support alignments larger than PAGE_SIZE.
@@ -173,6 +191,6 @@ unsafe fn realloc(
 
         // SAFETY: If not `None`, `ptr` is guaranteed to point to valid memory, which was previously
         // allocated with this `Allocator`.
-        unsafe { ReallocFunc::KVREALLOC.call(ptr, layout, flags) }
+        unsafe { ReallocFunc::KVREALLOC.call(ptr, layout, old_layout, flags) }
     }
 }
diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index 6188494f040d..e9e2e94430ef 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -5,6 +5,7 @@
 #[allow(unused_imports)] // Used in doc comments.
 use super::allocator::{KVmalloc, Kmalloc, Vmalloc};
 use super::{AllocError, Allocator, Flags};
+use core::alloc::Layout;
 use core::fmt;
 use core::marker::PhantomData;
 use core::mem::ManuallyDrop;
@@ -233,7 +234,7 @@ pub fn new_uninit(flags: Flags) -> Result<Box<MaybeUninit<T>, A>, AllocError> {
         let ptr = if Self::is_zst() {
             NonNull::dangling()
         } else {
-            let layout = core::alloc::Layout::new::<MaybeUninit<T>>();
+            let layout = Layout::new::<MaybeUninit<T>>();
             let ptr = A::alloc(layout, flags)?;
 
             ptr.cast()
@@ -452,14 +453,14 @@ impl<T, A> Drop for Box<T, A>
     A: Allocator,
 {
     fn drop(&mut self) {
-        let size = core::mem::size_of_val::<T>(self);
+        let layout = Layout::for_value::<T>(self);
 
         // SAFETY: The pointer in `self.0` is guaranteed to be valid by the type invariant.
         unsafe { core::ptr::drop_in_place::<T>(self.deref_mut()) };
 
-        if size != 0 {
-            // SAFETY: As `size` is not zero, `self.0` was previously allocated with `A`.
-            unsafe { A::free(self.0.cast()) };
-        }
+        // SAFETY:
+        // - `self.0` was previously allocated with `A`.
+        // - `layout` is equal to the `Layout´ `self.0` was allocated with.
+        unsafe { A::free(self.0.cast(), layout) };
     }
 }
diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index 686e969463f8..fb979013562c 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -7,6 +7,7 @@
     AllocError, Allocator, Box, Flags,
 };
 use core::{
+    alloc::Layout,
     fmt,
     marker::PhantomData,
     mem::{ManuallyDrop, MaybeUninit},
@@ -452,21 +453,28 @@ pub fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocEr
         // We know `cap` is <= `isize::MAX` because of the type invariants of `Self`. So the
         // multiplication by two won't overflow.
         let new_cap = core::cmp::max(cap * 2, len.checked_add(additional).ok_or(AllocError)?);
-        let layout = core::alloc::Layout::array::<T>(new_cap).map_err(|_| AllocError)?;
+        let layout = Layout::array::<T>(new_cap).map_err(|_| AllocError)?;
+
+        let old_layout = Layout::array::<T>(self.cap).map_err(|_| AllocError)?;
 
         // We need to make sure that `ptr` is either NULL or comes from a previous call to
         // `realloc_flags`. A `Vec<T, A>`'s `ptr` value is not guaranteed to be NULL and might be
         // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T, A>`'s
         // capacity to be zero if no memory has been allocated yet.
+        //
+        // Still required? In `Vec::new` we use `NonNull::dangling`, which effectively would be
+        // valid to pass to `A::realloc`, but was never created by one the `Allocator`'s functions.
         let ptr = if cap == 0 {
             None
         } else {
             Some(self.ptr.cast())
         };
 
-        // SAFETY: `ptr` is valid because it's either `None` or comes from a previous call to
-        // `A::realloc`. We also verified that the type is not a ZST.
-        let ptr = unsafe { A::realloc(ptr, layout, flags)? };
+        // SAFETY:
+        // - `ptr` is valid because it's either `None` or comes from a previous call to
+        //   `A::realloc`.
+        // - `old_layout` matches the `Layout` of the preceeding allocation, if any.
+        let ptr = unsafe { A::realloc(ptr, layout, old_layout, flags)? };
 
         self.ptr = ptr.cast();
 
@@ -528,9 +536,16 @@ fn drop(&mut self) {
         };
 
         // If `cap == 0` we never allocated any memory in the first place.
+        //
+        // Same here, theoretically, we know that `NonNull::dangling` from `Vec::new` is valid for
+        // `A::free`, but it was never created by any of the `Allocator` functions.
         if self.cap != 0 {
+            // This can never fail; since this `Layout` is equivalent to the one of the original
+            // allocation.
+            let layout = Layout::array::<T>(self.cap).unwrap();
+
             // SAFETY: `self.ptr` was previously allocated with `A`.
-            unsafe { A::free(self.ptr.cast()) };
+            unsafe { A::free(self.ptr.cast(), layout) };
         }
     }
 }
@@ -751,13 +766,17 @@ pub fn collect(self, flags: Flags) -> Vec<T, A> {
             ptr = buf.as_ptr();
         }
 
+        // This can never fail; since this `Layout` is equivalent to the one of the original
+        // allocation.
+        let old_layout = Layout::array::<T>(cap).unwrap();
+
         // This can never fail, `len` is guaranteed to be smaller than `cap`.
-        let layout = core::alloc::Layout::array::<T>(len).unwrap();
+        let layout = Layout::array::<T>(len).unwrap();
 
         // SAFETY: `buf` points to the start of the backing buffer and `len` is guaranteed to be
         // smaller than `cap`. Depending on `alloc` this operation may shrink the buffer or leaves
         // it as it is.
-        ptr = match unsafe { A::realloc(Some(buf.cast()), layout, flags) } {
+        ptr = match unsafe { A::realloc(Some(buf.cast()), layout, old_layout, flags) } {
             // If we fail to shrink, which likely can't even happen, continue with the existing
             // buffer.
             Err(_) => ptr,
@@ -846,9 +865,16 @@ fn drop(&mut self) {
         unsafe { ptr::drop_in_place(self.as_raw_mut_slice()) };
 
         // If `cap == 0` we never allocated any memory in the first place.
+        //
+        // Same here, theoretically, we know that `NonNull::dangling` from `Vec::new` is valid for
+        // `A::free`, but it was never created by any of the `Allocator` functions.
         if self.cap != 0 {
+            // This can never fail; since this `Layout` is equivalent to the one of the original
+            // allocation.
+            let layout = Layout::array::<T>(self.cap).unwrap();
+
             // SAFETY: `self.buf` was previously allocated with `A`.
-            unsafe { A::free(self.buf.cast()) };
+            unsafe { A::free(self.buf.cast(), layout) };
         }
     }
 }
-- 
2.46.1

Powered by blists - more mailing lists