linux-kernel - Re: [PATCH 2/3] rust: devres: implement Devres::access

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: <78853ac7-c9d2-4485-bbb3-859d2425e729@gmail.com>
Date: Sat, 26 Apr 2025 19:18:37 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
 kwilczynski@...nel.org, zhiw@...dia.com, cjia@...dia.com,
 jhubbard@...dia.com, bskeggs@...dia.com, acurrid@...dia.com,
 joelagnelf@...dia.com, ttabi@...dia.com, acourbot@...dia.com,
 ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
 gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
 linux-pci@...r.kernel.org, rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: devres: implement Devres::access_with()

On 26.04.25 7:08 PM, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 06:53:10PM +0200, Christian Schrefl wrote:
>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>> Implement a direct accessor for the data stored within the Devres for
>>> cases where we can proof that we own a reference to a Device<Bound>
>>> (i.e. a bound device) of the same device that was used to create the
>>> corresponding Devres container.
>>>
>>> Usually, when accessing the data stored within a Devres container, it is
>>> not clear whether the data has been revoked already due to the device
>>> being unbound and, hence, we have to try whether the access is possible
>>> and subsequently keep holding the RCU read lock for the duration of the
>>> access.
>>>
>>> However, when we can proof that we hold a reference to Device<Bound>
>>> matching the device the Devres container has been created with, we can
>>> guarantee that the device is not unbound for the duration of the
>>> lifetime of the Device<Bound> reference and, hence, it is not possible
>>> for the data within the Devres container to be revoked.
>>>
>>> Therefore, in this case, we can bypass the atomic check and the RCU read
>>> lock, which is a great optimization and simplification for drivers.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
>>> ---
>>>  rust/kernel/devres.rs | 35 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 35 insertions(+)
>>>
>>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>>> index 1e58f5d22044..ec2cd9cdda8b 100644
>>> --- a/rust/kernel/devres.rs
>>> +++ b/rust/kernel/devres.rs
>>> @@ -181,6 +181,41 @@ pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
>>>  
>>>          Ok(())
>>>      }
>>> +
>>> +    /// Obtain `&'a T`, bypassing the [`Revocable`].
>>> +    ///
>>> +    /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
>>> +    /// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
>>> +    ///
>>> +    /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
>>> +    /// has been created with.
>>
>> I would prefer this as a `# Errors` section.
> 
> I can make this an # Errors section.
> 
>> Also are there any cases where this is actually wanted as an error?
>> I'm not very familiar with the `Revocable` infrastructure,
>> but I would assume a mismatch here to be a bug in almost every case,
>> so a panic here might be reasonable.
> 
> Passing in a device reference that doesn't match the device the Devres instance
> was created with would indeed be a bug, but a panic isn't the solution, since we
> can handle this error just fine.
> 
> We never panic the whole kernel unless things go so utterly wrong that we can't
> can't recover from it; e.g. if otherwise we'd likely corrupt memory, etc.
>> (I would be fine with a reason for using an error here in the 
>> commit message or documentation/comments)
> 
> I don't think we need this in this commit or the method's documentation, it's a
> general kernel rule not to panic unless there's really no other way.

Alright I'm fine with it then.

I just don't think that most users of the function would be able to
gracefully recover from that other than failing the probe
or whatever, but it makes sense to allow the caller to deal with this.

Cheers
Christian