Message-ID: <CAH5fLggiSU9Ossy5gc+S_rSiX8v-JCDKPL_tRDYdjMYGfOt-0w@mail.gmail.com>
Date: Wed, 7 Aug 2024 09:51:21 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Danilo Krummrich <dakr@...nel.org>, 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, 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, acurrid@...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 v4 09/28] rust: alloc: implement kernel `Box`

On Wed, Aug 7, 2024 at 9:49 AM Benno Lossin <benno.lossin@...ton.me> wrote:
>
> On 07.08.24 01:01, Danilo Krummrich wrote:
> > On Tue, Aug 06, 2024 at 07:47:17PM +0000, Benno Lossin wrote:
> >> On 05.08.24 17:19, Danilo Krummrich wrote:
> >>> `Box` provides the simplest way to allocate memory for a generic type
> >>> with one of the kernel's allocators, e.g. `Kmalloc`, `Vmalloc` or
> >>> `KVmalloc`.
> >>>
> >>> In contrast to Rust's `Box` type, the kernel `Box` type considers the
> >>> kernel's GFP flags for all appropriate functions, always reports
> >>> allocation failures through `Result<_, AllocError>` and remains
> >>> independent from unstable features.
> >>>
> >>> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> >>> ---
> >>>  rust/kernel/alloc.rs      |   6 +
> >>>  rust/kernel/alloc/kbox.rs | 330 ++++++++++++++++++++++++++++++++++++++
> >>>  rust/kernel/init.rs       |  35 +++-
> >>>  rust/kernel/prelude.rs    |   2 +-
> >>>  rust/kernel/types.rs      |  56 +++++++
> >>>  5 files changed, 427 insertions(+), 2 deletions(-)
> >>>  create mode 100644 rust/kernel/alloc/kbox.rs
> >>>
> >>> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> >>> index 942e2755f217..d7beaf0372af 100644
> >>> --- a/rust/kernel/alloc.rs
> >>> +++ b/rust/kernel/alloc.rs
> >>> @@ -5,6 +5,7 @@
> >>>  #[cfg(not(any(test, testlib)))]
> >>>  pub mod allocator;
> >>>  pub mod box_ext;
> >>> +pub mod kbox;
> >>>  pub mod vec_ext;
> >>>
> >>>  #[cfg(any(test, testlib))]
> >>> @@ -13,6 +14,11 @@
> >>>  #[cfg(any(test, testlib))]
> >>>  pub use self::allocator_test as allocator;
> >>>
> >>> +pub use self::kbox::Box;
> >>> +pub use self::kbox::KBox;
> >>> +pub use self::kbox::KVBox;
> >>> +pub use self::kbox::VBox;
> >>> +
> >>>  /// Indicates an allocation error.
> >>>  #[derive(Copy, Clone, PartialEq, Eq, Debug)]
> >>>  pub struct AllocError;
> >>> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> >>> new file mode 100644
> >>> index 000000000000..4a4379980745
> >>> --- /dev/null
> >>> +++ b/rust/kernel/alloc/kbox.rs
> >>> @@ -0,0 +1,330 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +//! Implementation of [`Box`].
> >>> +
> >>> +use super::{AllocError, Allocator, Flags};
> >>> +use core::fmt;
> >>> +use core::marker::PhantomData;
> >>> +use core::mem::ManuallyDrop;
> >>> +use core::mem::MaybeUninit;
> >>> +use core::ops::{Deref, DerefMut};
> >>> +use core::pin::Pin;
> >>> +use core::result::Result;
> >>> +
> >>> +use crate::types::Unique;
> >>> +
> >>> +/// The kernel's [`Box`] type.
> >>> +///
> >>> +/// `Box` provides the simplest way to allocate memory for a generic type with one of the kernel's
> >>> +/// allocators, e.g. `Kmalloc`, `Vmalloc` or `KVmalloc`.
> >>> +///
> >>> +/// For non-zero-sized values, a [`Box`] will use the given allocator `A` for its allocation. For
> >>> +/// the most common allocators the type aliases `KBox`, `VBox` and `KVBox` exist.
> >>> +///
> >>> +/// It is valid to convert both ways between a [`Box`] and a raw pointer allocated with any
> >>> +/// `Allocator`, given that the `Layout` used with the allocator is correct for the type.
> >>> +///
> >>> +/// For zero-sized values the [`Box`]' pointer must be `dangling_mut::<T>`; no memory is allocated.
> >>
> >> Why do we need this to be in the docs?
> >
> > Probably not - do you suggest to remove it entirely? Otherwise, where do you
> > think we should move it?
>
> I would remove it, since it's just implementation detail and
> allocator-dependent.
>
> >>> +impl<T, A> Box<T, A>
> >>> +where
> >>> +    T: ?Sized,
> >>> +    A: Allocator,
> >>> +{
> >>> +    /// Constructs a `Box<T, A>` from a raw pointer.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// `raw` must point to valid memory, previously allocated with `A`, and at least the size of
> >>> +    /// type `T`.
> >>
> >> With this requirement and the invariant on `Box`, I am lead to believe
> >> that you can't use this for ZSTs, since they are not allocated with `A`.
> >> One solution would be to adjust this requirement. But I would rather use
> >> a different solution: we move the dangling pointer stuff into the
> >> allocator and also call it when `T` is a ZST (ie don't special case them
> >> in `Box` but in the impls of `Allocator`). That way this can stay as-is
> >> and the part about ZSTs in the invariant can be removed.
> >
> > Actually, we already got that. Every zero sized allocation will return a
> > dangling pointer. However, we can't call `Allocator::free` with (any) dangling
> > pointer though.
>
> The last part is rather problematic in my opinion, since the safety
> requirements of the functions in `Allocator` don't ensure that you're
> not allowed to do it. We should make it possible to free dangling
> pointers that were previously "allocated" by the allocator (ie returned
> by `realloc`).
> Maybe we do need an `old_layout` parameter for that (that way we can
> also `debug_assert_eq!(old_layout.align(), new_layout.align())`).

The std allocators generally prohibit zero sized allocations, so it
seems sensible for us to do the same?

Alice

Powered by blists - more mailing lists