Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Jul 2023 10:28:00 +0000
In-Reply-To: <ZK2UeFE1A4iRfePS@boqun-archlinux>
Mime-Version: 1.0
References: <ZK2UeFE1A4iRfePS@boqun-archlinux>
Message-ID: <20230714102800.2196236-1-aliceryhl@google.com>
Subject: Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut`
From:   Alice Ryhl <aliceryhl@google.com>
To:     boqun.feng@gmail.com
Cc:     alex.gaynor@gmail.com, aliceryhl@google.com,
        benno.lossin@proton.me, bjorn3_gh@protonmail.com, gary@garyguo.net,
        linux-kernel@vger.kernel.org, ojeda@kernel.org,
        patches@lists.linux.dev, rust-for-linux@vger.kernel.org,
        wedsonaf@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Boqun Feng <boqun.feng@gmail.com> writes:
> On Mon, Jul 10, 2023 at 07:46:42AM +0000, Alice Ryhl wrote:
>> Previously, the `ForeignOwnable` trait had a method called `borrow_mut`
>> that was intended to provide mutable access to the inner value. However,
>> the method accidentally made it possible to change the address of the
>> object being modified, which usually isn't what we want. (And when we
>> want that, it can be done by calling `from_foreign` and `into_foreign`,
>> like how the old `borrow_mut` was implemented.)
>> 
>> In this patch, we introduce an alternate definition of `borrow_mut` that
>> solves the previous problem. Conceptually, given a pointer type `P` that
>> implements `ForeignOwnable`, the `borrow_mut` method gives you the same
>> kind of access as an `&mut P` would, except that it does not let you
>> change the pointer `P` itself.
>> 
>> This is analogous to how the existing `borrow` method provides the same
>> kind of access to the inner value as an `&P`.
>> 
>> Note that for types like `Arc`, having an `&mut Arc<T>` only gives you
>> immutable access to the inner `T`. This is because mutable references
>> assume exclusive access, but there might be other handles to the same
>> reference counted value, so the access isn't exclusive. The `Arc` type
>> implements this by making `borrow_mut` return the same type as `borrow`.
>> 
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> ---
>> 
>> This patch depends on https://lore.kernel.org/all/20230706094615.3080784-1-aliceryhl@google.com/
>> 
>>  rust/kernel/sync/arc.rs | 31 +++++++++-----
>>  rust/kernel/types.rs    | 93 ++++++++++++++++++++++++++++++-----------
>>  2 files changed, 89 insertions(+), 35 deletions(-)
>>  
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index d479f8da8f38..1c2fb36906b6 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -20,66 +20,111 @@
>>  /// This trait is meant to be used in cases when Rust objects are stored in C objects and
>>  /// eventually "freed" back to Rust.
>>  pub trait ForeignOwnable: Sized {
>> -    /// Type of values borrowed between calls to [`ForeignOwnable::into_foreign`] and
>> -    /// [`ForeignOwnable::from_foreign`].
>> +    /// Type used to immutably borrow a value that is currently foreign-owned.
>>      type Borrowed<'a>;
>>  
>> +    /// Type used to mutably borrow a value that is currently foreign-owned.
>> +    type BorrowedMut<'a>;
>> +
> 
> I would probably want to say "if the `impl ForeignOwnable` doesn't have
> the exclusive ownership, `BorrowedMut` should be the same as `Borrowed`"
> for logical self-consistent,

I don't phrase it as a requirement, but I do already say something along
these lines.

> and even further make it as default as follow:
> 
> 	type BorrowedMut<'a> = Self::Borrowed<'a>;
> 
> 	unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> Self::BorrowedMut<'a> {
> 	    Self::borrow(ptr)
> 	}
> 
> but it might be over-engineering (and require associated_type_defaults
> and more)...

I don't think it makes sense to use defaults here. There are very few
implementers of this trait, and I'd like all of them to explicitly think
about whether `BorrowedMut` should be the same as `Borrowed`, or
different.

> Anyway,
> 
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> 
> Regards,
> Boqun

Alice