| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZtDwduKjIEZ3RQtk@pollux.localdomain>
Date: Fri, 30 Aug 2024 00:04:38 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
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 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?
>
> > + 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.
What do you think should be written here?
>
> ---
> Cheers,
> Benno
>
> > + #[inline]
> > + unsafe fn realloc(
> > + ptr: Option<NonNull<u8>>,
> > + layout: Layout,
> > + flags: Flags,
> > + ) -> Result<NonNull<[u8]>, AllocError> {
> > + // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
> > + unsafe { ReallocFunc::KREALLOC.call(ptr, layout, flags) }
> > + }
> > +}
> > +
> > unsafe impl GlobalAlloc for Kmalloc {
> > unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
> > // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
> > --
> > 2.46.0
> >
>
Powered by blists - more mailing lists