linux-kernel - Re: [PATCH v4 5/5] rust: devres: implement register

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: <411adfbb-1110-434f-a22d-d60914be6968@kernel.org>
Date: Thu, 26 Jun 2025 23:20:21 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Boqun Feng <boqun.feng@...il.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
 alex.gaynor@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
 lossin@...nel.org, a.hindborg@...nel.org, aliceryhl@...gle.com,
 tmgross@...ch.edu, david.m.ertman@...el.com, ira.weiny@...el.com,
 leon@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org
Subject: Re: [PATCH v4 5/5] rust: devres: implement register_release()

On 6/26/25 11:16 PM, Boqun Feng wrote:
> On Thu, Jun 26, 2025 at 10:48:03PM +0200, Danilo Krummrich wrote:
>> On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
>>> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
>>>> register_release() is useful when a device resource has associated data,
>>>> but does not require the capability of accessing it or manually releasing
>>>> it.
>>>>
>>>> If we would want to be able to access the device resource and release the
>>>> device resource manually before the device is unbound, but still keep
>>>> access to the associated data, we could implement it as follows.
>>>>
>>>> 	struct Registration<T> {
>>>> 	   inner: Devres<RegistrationInner>,
>>>> 	   data: T,
>>>> 	}
>>>>
>>>> However, if we never need to access the resource or release it manually,
>>>> register_release() is great optimization for the above, since it does not
>>>> require the synchronization of the Devres type.
>>>>
>>>> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
>>>> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
>>>> ---
>>>>   rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 73 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>>>> index 3ce8d6161778..92aca78874ff 100644
>>>> --- a/rust/kernel/devres.rs
>>>> +++ b/rust/kernel/devres.rs
>>>> @@ -353,3 +353,76 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
>>>>   
>>>>       register_foreign(dev, data)
>>>>   }
>>>> +
>>>> +/// [`Devres`]-releaseable resource.
>>>> +///
>>>> +/// Register an object implementing this trait with [`register_release`]. Its `release`
>>>> +/// function will be called once the device is being unbound.
>>>> +pub trait Release {
>>>> +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
>>>> +    type Ptr: ForeignOwnable;
>>>> +
>>>> +    /// Called once the [`Device`] given to [`register_release`] is unbound.
>>>> +    fn release(this: Self::Ptr);
>>>> +}
>>>> +
>>>
>>> I would like to point out the limitation of this design, say you have a
>>> `Foo` that can ipml `Release`, with this, I think you could only support
>>> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
>>> for `register_release()`. Maybe we want:
>>>
>>>      pub trait Release<Ptr: ForeignOwnable> {
>>>          fn release(this: Ptr);
>>>      }
>>
>> Good catch! I think this wasn't possible without ForeignOwnable::Target.
>>
>> Here's the diff for the change:
>>
> 
> Looks good to me.
> 
>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>> index 92aca78874ff..42a9cd2812d8 100644
>> --- a/rust/kernel/devres.rs
>> +++ b/rust/kernel/devres.rs
>> @@ -358,12 +358,9 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
>>   ///
>>   /// Register an object implementing this trait with [`register_release`]. Its `release`
>>   /// function will be called once the device is being unbound.
>> -pub trait Release {
>> -    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
>> -    type Ptr: ForeignOwnable;
>> -
>> +pub trait Release<Ptr: ForeignOwnable> {
> 
> My bad, is it possible to do
> 
> 	pub trait Release<Ptr: ForeignOwnable<Target=Self>> {
> 
> ? that's better IMO.

Sure, that works.