linux-kernel - Re: [PATCH v3 1/2] rust: irq: add support for request

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: <39C56E3E-07C6-44BB-B5F6-38090F037032@collabora.com>
Date: Thu, 15 May 2025 09:27:51 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: 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>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

> On 15 May 2025, at 09:04, Danilo Krummrich <dakr@...nel.org> wrote:
> 
> On Thu, May 15, 2025 at 08:54:35AM -0300, Daniel Almeida wrote:
>> Hi Danilo,
>> 
>>> On 14 May 2025, at 18:53, Danilo Krummrich <dakr@...nel.org> wrote:
>>> 
>>> On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
>>>> +/// // This is running in process context.
>>>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
>>>> +///     let registration = Registration::register(irq, flags::SHARED, c_str!("my-device"), handler);
>>>> +///
>>>> +///     // You can have as many references to the registration as you want, so
>>>> +///     // multiple parts of the driver can access it.
>>>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
>>> 
>>> This makes it possible to arbitrarily extend the lifetime of an IRQ
>>> registration. However, we must guarantee that the IRQ is unregistered when the
>>> corresponding device is unbound. We can't allow drivers to hold on to device
>>> resources after the corresponding device has been unbound.
>>> 
>>> Why does the data need to be part of the IRQ registration itself? Why can't we
>>> pass in an Arc<T> instance already when we register the IRQ?
>>> 
>>> This way we'd never have a reason to ever access the Registration instance
>>> itself ever again and we can easily wrap it as Devres<irq::Registration> -
>>> analogously to devm_request_irq() on the C side - without any penalties.
>>> 
>>>> +///     // The handler may be called immediately after the function above
>>>> +///     // returns, possibly in a different CPU.
>>>> +///
>>>> +///     {
>>>> +///         // The data can be accessed from the process context too.
>>>> +///         let mut data = registration.handler().0.lock();
>>>> +///         *data = 42;
>>>> +///     }
>>>> +///
>>>> +///     Ok(registration)
>>>> +/// }
>>> 
>> 
>> Up until this point, there was no need for the data to not be inline with the
>> registration. This new design would force an Arc, which, apart from the
>> heap-allocation, is restrictive for users.
> 
> Does the current design not also imply a heap allocation heap allocation? With
> my proposal irq::Registration::new() can just return an irq::Registration
> instance, not an impl PinInit that you need to stuff into a Box or Arc instead.
> Hence, there shouldn't be a difference.

Well, not really, because this impl PinInit can be assigned to something larger
that is already pinned, like drm::Device::Data for example, which is (or was)
already behind an Arc, or any other private data in other subsystems.

IIUC what you proposed has yet another indirection. If we reuse the example
from above, that would be an Arc for the drm Data, and another Arc for the
handler itself?

I definitely see your point here, I am just trying to brainstorm another way of
doing this.

> 
>> Can’t we use Devres with the current implementation?
>> 
>> IIUC from a very cursory glance, all that would mean is that you'd have to call
>> try_access() on your handler, which should be fine?
> 
> Well, that would work indeed.
> 
> But people will - with good reason - be upset that every access to the handler's
> data needs to be guarded with the RCU read side critical section implied by
> Revocable and hence Devres.

True, I totally missed that.

> 
> We can easily avoid that in this case, hence we should do it.

— Daniel