linux-kernel - Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow

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: <20230714102800.2196236-1-aliceryhl@google.com>
Date:   Fri, 14 Jul 2023 10:28:00 +0000
From:   Alice Ryhl <aliceryhl@...gle.com>
To:     boqun.feng@...il.com
Cc:     alex.gaynor@...il.com, aliceryhl@...gle.com,
        benno.lossin@...ton.me, bjorn3_gh@...tonmail.com, gary@...yguo.net,
        linux-kernel@...r.kernel.org, ojeda@...nel.org,
        patches@...ts.linux.dev, rust-for-linux@...r.kernel.org,
        wedsonaf@...il.com
Subject: Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut`

Boqun Feng <boqun.feng@...il.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@...gle.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@...il.com>
> 
> Regards,
> Boqun

Alice