Message-ID: <012f5a12-2408-4658-8318-55fa8d4285e1@proton.me>
Date: Wed, 07 Aug 2024 07:49:31 +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, 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 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())`).

>>> +    {
>>> +        Ok(Self::new(x, flags)?.into())
>>> +    }
>>> +
>>> +    /// Drops the contents, but keeps the allocation.
>>> +    ///
>>> +    /// # Examples
>>> +    ///
>>> +    /// ```
>>> +    /// let value = KBox::new([0; 32], GFP_KERNEL)?;
>>> +    /// assert_eq!(*value, [0; 32]);
>>> +    /// let value = KBox::drop_contents(value);
>>> +    /// // Now we can re-use `value`:
>>> +    /// let value = KBox::write(value, [1; 32]);
>>> +    /// assert_eq!(*value, [1; 32]);
>>> +    /// # Ok::<(), Error>(())
>>> +    /// ```
>>> +    pub fn drop_contents(this: Self) -> Box<MaybeUninit<T>, A> {
>>> +        let ptr = Box::into_raw(this);
>>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>> +        unsafe { core::ptr::drop_in_place(ptr) };
>>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>> +        unsafe { Box::from_raw(ptr.cast()) }
>>> +    }
>>
>> I don't particularly care in this instance, but you just took my patch
>> and folded it into your own without asking me or specifying it in the
>> commit message. In general I would have assumed that you just put the
>> entire patch into the series (with correct From:... etc).
> 
> When I asked about this in [1] my understanding was that we expect [1] to land
> prior to this series. So, I'm just anticipating a future rebase where I move
> this code from box_ext.rs to kbox.rs, just like Alice suggested for her
> "ForeignOwnable for Pin<crate::alloc::Box<T, A>>" implementation.
> 
> I also understand your later reply, where you said: "[...] then you can just
> include it when you remove the `BoxExit` trait." as confirmation.
> 
> Probably that's a misunderstanding though. Sorry if that's the case.

Yeah what I meant by that was you base it on top and then move it from
the `BoxExt` trait over to `Box` in a correctly attributed patch. As I
said above, I don't really mind in this case, since it's trivial, so no
worries. Just a heads-up for occasions where it is non-trivial.

> [1] https://lore.kernel.org/lkml/24a8d381-dd13-4d19-a736-689b8880dbe1@proton.me/
> 
>>
>>> +}
>>> +
>>> +impl<T, A> From<Box<T, A>> for Pin<Box<T, A>>
>>> +where
>>> +    T: ?Sized,
>>> +    A: Allocator,
>>> +    A: 'static,
>>> +{
>>> +    /// Converts a `Box<T>` into a `Pin<Box<T>>`. If `T` does not implement [`Unpin`], then
>>> +    /// `*boxed` will be pinned in memory and unable to be moved.
>>> +    ///
>>> +    /// This conversion does not allocate on the heap and happens in place.
>>> +    ///
>>> +    /// This is also available via [`Box::into_pin`].
>>> +    ///
>>> +    /// Constructing and pinning a `Box` with <code><Pin<Box\<T>>>::from([Box::new]\(x))</code>
>>> +    /// can also be written more concisely using <code>[Box::pin]\(x)</code>.
>>> +    /// This `From` implementation is useful if you already have a `Box<T>`, or you are
>>> +    /// constructing a (pinned) `Box` in a different way than with [`Box::new`].
>>
>> This also looks very much like something from the stdlib...
> 
> Yeah, I'll replace that.
> 
>>
>>> +    fn from(b: Box<T, A>) -> Self {
>>> +        Box::into_pin(b)
>>> +    }
>>> +}
>>> +
>>> +impl<T, A> Deref for Box<T, A>
>>> +where
>>> +    T: ?Sized,
>>> +    A: Allocator,
>>> +{
>>> +    type Target = T;
>>> +
>>> +    fn deref(&self) -> &T {
>>> +        // SAFETY: `self.0` is always properly aligned, dereferenceable and points to an initialized
>>> +        // instance of `T`.
>>
>> If `T` is a ZST, then it is not dereferenceable.
> 
> Why not? If `T` is a ZST `self.0` is `Unique::<T>::dangling()`. So, in the end
> this is the same as `NonNull::<T>::dangling().as_ref()`.

You are right, I just looked at [1] again and they define
dereferenceable as "the memory range of the given size starting at the
pointer must all be within the bounds of a single allocated object", for
a zero-sized allocation, this holds vacuously.

[1]: https://doc.rust-lang.org/core/ptr/index.html#safety

>>> +        unsafe { self.0.as_ref() }
>>> +    }
>>> +}
>>> +
>>> +impl<T, A> DerefMut for Box<T, A>
>>> +where
>>> +    T: ?Sized,
>>> +    A: Allocator,
>>> +{
>>> +    fn deref_mut(&mut self) -> &mut T {
>>> +        // SAFETY: `self.0` is always properly aligned, dereferenceable and points to an initialized
>>> +        // instance of `T`.
>>> +        unsafe { self.0.as_mut() }
>>> +    }
>>> +}
>>> +
>>> +impl<T, A> fmt::Debug for Box<T, A>
>>> +where
>>> +    T: ?Sized + fmt::Debug,
>>> +    A: Allocator,
>>> +{
>>> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>>> +        fmt::Debug::fmt(&**self, f)
>>> +    }
>>> +}
>>> +
>>> +impl<T, A> Drop for Box<T, A>
>>> +where
>>> +    T: ?Sized,
>>> +    A: Allocator,
>>> +{
>>> +    fn drop(&mut self) {
>>> +        let ptr = self.0.as_ptr();
>>> +
>>> +        // SAFETY: `ptr` is always properly aligned, dereferenceable and points to an initialized
>>> +        // instance of `T`.
>>> +        let size = unsafe { core::mem::size_of_val(&*ptr) };
>>
>> 1. `size_of_val` is not `unsafe`.
> 
> Right, but dereferencing the `ptr` is unsafe.
> 
>> 2. why not use `&*self` instead of using the raw pointer? (then move the
>>    let binding below this line)
> 
> If we ever support non-ZST `Allocator`s using `self` would not always evaluate
> to the correct size. I think evaluating the size of `T` rather than `Box<T>` is
> the correct thing to do.

I mean use `Box::deref` (that's what `&*self` should do), you don't need
to repeat the same SAFETY comment when it already is wrapped by a safe
function.

---
Cheers,
Benno

Powered by blists - more mailing lists