Message-ID: <962b7014-4f8b-4abe-8774-636b612a051c@proton.me>
Date: Fri, 30 Aug 2024 14:45:35 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...sung.com, aliceryhl@...gle.com, akpm@...ux-foundation.org, 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
Subject: Re: [PATCH v6 04/26] rust: alloc: implement `Allocator` for `Kmalloc`

On 30.08.24 00:04, Danilo Krummrich wrote:
> On Thu, Aug 29, 2024 at 06:32:42PM +0000, Benno Lossin wrote:
>> On 16.08.24 02:10, Danilo Krummrich wrote:
>>> 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 <dakr@...nel.org>
>>> ---
>>>  rust/kernel/alloc.rs           |  2 +-
>>>  rust/kernel/alloc/allocator.rs | 72 +++++++++++++++++++++++++++++++++-
>>>  2 files changed, 72 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
>>> index 9932f21b0539..477dbe3c5a2f 100644
>>> --- a/rust/kernel/alloc.rs
>>> +++ b/rust/kernel/alloc.rs
>>> @@ -4,7 +4,7 @@
>>>
>>>  #[cfg(not(test))]
>>>  #[cfg(not(testlib))]
>>> -mod allocator;
>>> +pub mod allocator;
>>>  pub mod box_ext;
>>>  pub mod vec_ext;
>>>
>>> diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
>>> index e32182f91167..78e7d5488843 100644
>>> --- a/rust/kernel/alloc/allocator.rs
>>> +++ b/rust/kernel/alloc/allocator.rs
>>> @@ -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.
>>
>>>
>>>  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.
>>> +///
>>> +/// `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)
>>
>>> +///
>>> +/// 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 {
>>> +            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?

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.

>>> +        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)?
>>> +        };
>>> +
>>> +        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.

Oh yeah, sorry I forgot about that.
 
> 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.


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

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

Powered by blists - more mailing lists