linux-kernel - Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`

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]

Date:   Mon, 24 Jul 2023 13:07:04 -0300
From:   Martin Rodriguez Reboredo <yakoyoku@...il.com>
To:     Benno Lossin <benno.lossin@...ton.me>,
        Miguel Ojeda <ojeda@...nel.org>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Alex Gaynor <alex.gaynor@...il.com>
Cc:     Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Andreas Hindborg <nmi@...aspace.dk>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
        Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to
 `{Pin}Init<T, E>`

On 7/24/23 11:08, Benno Lossin wrote:
> On 21.07.23 02:23, Martin Rodriguez Reboredo wrote:
>> On 7/19/23 11:21, Benno Lossin wrote:
>>> +/// An initializer returned by [`PinInit::pin_chain`].
>>> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
>>> +
>>> +// SAFETY: the `__pinned_init` function is implemented such that it
>>> +// - returns `Ok(())` on successful initialization,
>>> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
>>> +// - considers `slot` pinned.
>>> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
>>> +where
>>> +    I: PinInit<T, E>,
>>> +    F: FnOnce(Pin<&mut T>) -> Result<(), E>,
>>> +{
>>> +    unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>>> +        // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
>>> +        unsafe { self.0.__pinned_init(slot)? };
>>> +        // SAFETY: The above call initialized `slot` and we still have unique access.
>>> +        let val = unsafe { &mut *slot };
>>> +        // SAFETY: `slot` is considered pinned
>>> +        let val = unsafe { Pin::new_unchecked(val) };
>>> +        (self.1)(val).map_err(|e| {
>>> +            // SAFETY: `slot` was initialized above.
>>> +            unsafe { core::ptr::drop_in_place(slot) };
>>> +            e
>>
>> I might stumble upon an error like EAGAIN if I call `pin_chain` but that
>> means `slot` will be dropped. So my recommendation is to either not drop
>> the value or detail in `pin_chain`'s doc comment that the closure will
>> drop on error.
> 
> This is a bit confusing to me, because dropping the value on returning `Err`
> is a safety requirement of `PinInit`. Could you elaborate why this is
> surprising? I can of course add it to the documentation, but I do not see
> how it could be implemented differently. Since if you do not drop the value
> here, nobody would know that it is still initialized.

I knew about the requirement of dropping on `Err`, but what has caught my
attention is that `{pin_}chain` might not abide with it per the doc
comment as it says that `self` is initialized before calling `f`...

     /// First initializes the value using `self` then calls the function
     /// `f` with the initialized value.

But one can not know what would happen when `f` fails, specially if
such failure can be ignored or it's only temporarily.

So then, the best course IMO is to mention that in some way the value is
still being initialized, kinda setting it up, and that it will be dropped
when an error is returned. WDYT?