linux-kernel - Re: [PATCH v1 2/3] rust: add pin-init API

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] [day] [month] [year] [list]

Message-ID: <mtmuEYnjtcWcHUOLMmGQ35qZ6AhxE7bna6gGB4u1Ldh9MLvSdZT2xKL43LYy71MMNQbwEvkWmUkEOO3HiiNrIgmYvIVjDt1kBVxaBI23ERY=@protonmail.com>
Date:   Thu, 16 Mar 2023 23:08:09 +0000
From:   y86-dev <y86-dev@...tonmail.com>
To:     Gary Guo <gary@...yguo.net>
Cc:     "ojeda@...nel.org" <ojeda@...nel.org>,
        "alex.gaynor@...il.com" <alex.gaynor@...il.com>,
        "wedsonaf@...il.com" <wedsonaf@...il.com>,
        "boqun.feng@...il.com" <boqun.feng@...il.com>,
        "bjorn3_gh@...tonmail.com" <bjorn3_gh@...tonmail.com>,
        "rust-for-linux@...r.kernel.org" <rust-for-linux@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "patches@...ts.linux.dev" <patches@...ts.linux.dev>
Subject: Re: [PATCH v1 2/3] rust: add pin-init API

On Thursday, March 16th, 2023 at 18:38, Gary Guo <gary@...yguo.net> wrote:
> On Thu, 16 Mar 2023 09:38:16 +0000
> y86-dev <y86-dev@...tonmail.com> wrote:
>
> > > > +
> > > > +/// Trait facilitating pinned destruction.
> > > > +///
> > > > +/// Use [`pinned_drop`] to implement this trait safely:
> > > > +///
> > > > +/// ```rust
> > > > +/// # use kernel::sync::Mutex;
> > > > +/// use kernel::macros::pinned_drop;
> > > > +/// use core::pin::Pin;
> > > > +/// #[pin_data(PinnedDrop)]
> > > > +/// struct Foo {
> > > > +///     #[pin]
> > > > +///     mtx: Mutex<usize>,
> > > > +/// }
> > > > +///
> > > > +/// #[pinned_drop]
> > > > +/// impl PinnedDrop for Foo {
> > > > +///     fn drop(self: Pin<&mut Self>) {
> > > > +///         pr_info!("Foo is being dropped!");
> > > > +///     }
> > > > +/// }
> > > > +/// ```
> > > > +///
> > > > +/// # Safety
> > > > +///
> > > > +/// This trait must be implemented with [`pinned_drop`].
> > > > +///
> > > > +/// [`pinned_drop`]: kernel::macros::pinned_drop
> > > > +pub unsafe trait PinnedDrop: __PinData {
> > > > +    /// Executes the pinned destructor of this type.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// Only call this from `<Self as Drop>::drop`.
> > > > +    unsafe fn drop(self: Pin<&mut Self>);
> > > > +
> > > > +    // Used by the `pinned_drop` proc-macro to ensure that only safe operations are used in `drop`.
> > > > +    // the function should not be called.
> > > > +    #[doc(hidden)]
> > > > +    fn __ensure_no_unsafe_op_in_drop(self: Pin<&mut Self>);
> > >
> > > One idea to avoid this extra function is to have an unsafe token to the
> > > drop function.
> > >
> > > fn drop(self: Pin<&mut Self>, token: TokenThatCanOnlyBeCreatedUnsafely);
> >
> > What is wrong with having this extra function? If the problem is that this
> > function might be called, then we could add a parameter with an
> > unconstructable type.
> >
> > I think that `drop` should be `unsafe`, since it really does have
> > the requirement of only being called in the normal drop impl.
>
> The point to avoid having two functions with the same body. This would
> require double the amount of checks needed by the compiler (and make
> error message worth if anything's wrong in the body of `drop`).
>
> This current approach is really just a hack to avoid code from doing
> unsafe stuff without using `unsafe` block -- and the best solution is
> just to avoid make `drop` function unsafe. However we don't want drop
> function to be actually called from safe code, and that's the point of
> a token that can only be created unsafely is force `drop` to *not* be
> called by safe code. The token is a proof that `unsafe` is being used.
>
> This way the `__ensure_no_unsafe_op_in_drop` function would not be
> needed.

That makes sense.

> > > > +// This trait is only implemented via the `#[pin_data]` proc-macro. It is used to facilitate
> > > > +// the pin projections within the initializers.
> > > > +#[doc(hidden)]
> > > > +pub unsafe trait __PinData {
> > > > +    type __PinData;
> > > > +}
> > > > +
> > > > +/// Stack initializer helper type. Use [`stack_pin_init`] instead of this primitive.
> > >
> > > `#[doc(hidden)]`?
> >
> > This trait is implementation detail of the `#[pin_data]` macro. Why should
> > it be visible in the rust-docs?
>
> I am commenting about `stack_pin_init` (note the doc comment above my
> comment). `StackInit` is an implementation detail of `stack_pin_init`
> and shouldn't be exposed, IMO. Or do you think manual use of
> `StackInit` is needed?

I thought that it could be used by something else, but I will hide it for
now.

Cheers,
Benno