Message-ID: <761a05ce-b711-439c-bbb5-44c570867a50@ralfj.de>
Date: Fri, 2 May 2025 10:57:57 +0200
From: Ralf Jung <post@...fj.de>
To: Benno Lossin <lossin@...nel.org>,
 Christian Schrefl <chrisi.schrefl@...il.com>, Sky <sky@...9.dev>,
 Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>,
 Gerald Wisböck <gerald.wisboeck@...ther.ink>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 1/3] rust: add UnsafePinned type

Hi all,

On 01.05.25 20:55, Benno Lossin wrote:
> On Thu May 1, 2025 at 7:12 PM CEST, Christian Schrefl wrote:
>> On 30.04.25 10:36 AM, Christian Schrefl wrote:
>>> +/// This type provides a way to opt-out of typical aliasing rules;
>>> +/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
>>> +///
>>> +/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
>>> +/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
>>> +/// generically on `&mut T` assume that the memory that stores `T` is uniquely owned (such as
>>> +/// `mem::swap`). In other words, while having aliasing with `&mut Wrapper` is not immediate
>>> +/// Undefined Behavior, it is still unsound to expose such a mutable reference to code you do not
>>> +/// control! Techniques such as pinning via [`Pin`](core::pin::Pin) are needed to ensure soundness.
>>> +///
>>> +/// Similar to [`UnsafeCell`], [`UnsafePinned`] will not usually show up in
>>> +/// the public API of a library. It is an internal implementation detail of libraries that need to
>>> +/// support aliasing mutable references.
>>> +///
>>> +/// Further note that this does *not* lift the requirement that shared references must be read-only!
>>> +/// Use [`UnsafeCell`] for that.
>>
>> [CC Ralf]
>>
>> Ralf has replied to me on Github that this will most likely change [0]. How should this be handled?
>>
>> I would fine with submitting a patch once it changes on the rust side (possibly waiting until the
>> feature is close to stabilization). I think it is better to only add this guarantee later as it
>> will be easier to remove unnecessary `UnsafeCell`s than it would be to later add them back in ever
>> case where they would be needed (in case rust doesn't change `UnsafePinned` to act like `UnsafeCell`).
> 
> Agreed, unless Ralf suggests a different way, we should do it like this.

Sorry, I replied only on github since that was easier to do more quickly. ;)

Yes I would recommend for now to keep the `UnsafeCell` in the kernel sources, 
until the compiler actually implements the change that removes `noalias` from 
`&UnsafePinned`. And even then you should probably keep the `UnsafeCell` when 
building for older compiler versions from before that patch (or keep it until 
you drop support for those older compiler versions).

This is not a spec-only change, codegen really has to be adjusted to make 
`&UnsafePinned` properly compatible with mutable aliasing, and I see no reason 
to risk potential problems here by prematurely removing that `UnsafeCell`.

Kind regards,
Ralf

> 
> ---
> Cheers,
> Benno
> 
>> Also see to the tracking issue [1] for the reason why `UnsafeCell` behavior is most likely required.
>>
>> [0]: https://github.com/rust-lang/rust/issues/125735#issuecomment-2842926832
>> [1]: https://github.com/rust-lang/rust/issues/137750
>>
>> Cheers
>> Christian

Powered by blists - more mailing lists