Re: [PATCH v4 09/28] rust: alloc: implement kernel `Box`

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote:
> >> 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.

Yes, I think it needs to be added.

> 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())`).

Please see my reply in [1] - let's continue the discussion there.

[1] https://lore.kernel.org/rust-for-linux/ZrNIaAcGkGU0d8I3@pollux/

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

I don't understand this. What do you mean with "correctly attributed patch" in
this case?

There are various existing implementations around `Box` and `BoxExt`, are you
saying that I should create separate patches for moving / adapting all of them,
e.g. this patch adapts parts from `BoxExt`, the implementation of
`ForeignOwnable` for `Box<T>`, the implementation of `InPlaceInit<T>` for
`Box<T>`.

I don't think this is necessary.

I probably shouldn't anticipate a future rebase though, it just leads to
confusion. I'll drop it for now and re-add it once your patch lands in rust-next.

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

Oh, yes, that's indeed a good suggestion.

> 
> ---
> Cheers,
> Benno
>