linux-kernel - Re: [PATCH v4 08/28] rust: types: implement `Unique<T>`

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: <f3457432-dc83-4a19-b75a-88b914430733@proton.me>
Date: Wed, 07 Aug 2024 07:27:43 +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 08/28] rust: types: implement `Unique<T>`

On 07.08.24 01:12, Danilo Krummrich wrote:
> On Tue, Aug 06, 2024 at 05:22:21PM +0000, Benno Lossin wrote:
>> On 05.08.24 17:19, Danilo Krummrich wrote:
>>> +impl<T: Sized> Unique<T> {
>>> +    /// Creates a new `Unique` that is dangling, but well-aligned.
>>> +    ///
>>> +    /// This is useful for initializing types which lazily allocate, like
>>> +    /// `Vec::new` does.
>>> +    ///
>>> +    /// Note that the pointer value may potentially represent a valid pointer to
>>> +    /// a `T`, which means this must not be used as a "not yet initialized"
>>> +    /// sentinel value. Types that lazily allocate must track initialization by
>>> +    /// some other means.
>>> +    #[must_use]
>>> +    #[inline]
>>> +    pub const fn dangling() -> Self {
>>> +        Unique {
>>> +            pointer: NonNull::dangling(),
>>> +            _marker: PhantomData,
>>> +        }
>>> +    }
>>
>> I think I already asked this, but the code until this point is copied
>> from the rust stdlib and nowhere cited, does that work with the
>> licensing?
>>
>> I also think that the code above could use some improvements:
>> - add an `# Invariants` section with appropriate invariants (what are
>>   they supposed to be?)
>> - Do we really want this type to be public and exported from the kernel
>>   crate? I think it would be better if it were crate-private.
>> - What do we gain from having this type? As I learned recently, the
>>   `Unique` type from `core` doesn't actually put the `noalias` onto
>>   `Box` and `Vec`. The functions are mostly delegations to `NonNull`, so
>>   if the only advantages are that `Send` and `Sync` are already
>>   implemented, then I think we should drop this.
> 
> I originally introduced it for the reasons described in [1], but mainly to make
> clear that the owner of this thing also owns the memory behind the pointer and
> the `Send` and `Sync` stuff you already mentioned.

I would prefer if we make that explicit, since it is rather error-prone
when creating new pointer types (and one should have to think about
thread safety).

---
Cheers,
Benno

> If no one else has objections we can also just drop it. Personally, I'm fine
> either way.
> 
> [1] https://docs.rs/rust-libcore/latest/core/ptr/struct.Unique.html