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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57f75d21-6bb4-4991-ab48-de5f08acba18@proton.me>
Date: Wed, 07 Aug 2024 19:45:50 +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 12:38, Danilo Krummrich wrote:
> On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote:
>>>>> +    {
>>>>> +        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?

So, seems like I mixed the two approaches I thought of. Here are they
separated:
1. base the series on top of my patches and then copy the implementation
   from the in-tree file this patch.
2. create a new patch that adds `drop_contents` to your Box (ie after
   this patch) that has `Signed-off-by: you` and `Co-Developed-by: me`.
   With this approach I would also ask privately (including the patch)
   if that would be ok.

---
Cheers,
Benno

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