linux-kernel - Re: [PATCH] rust: irq: request: touch up the documentation

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: <aMEc0-8mM4uaFwlB@google.com>
Date: Wed, 10 Sep 2025 06:38:11 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Danilo Krummrich <dakr@...nel.org>, 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 <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: irq: request: touch up the documentation

On Tue, Sep 09, 2025 at 05:46:55PM -0300, Daniel Almeida wrote:
> Parts of the documentation are either unclear or misplaced and can
> otherwise be improved. This commit fixes them.
> 
> This is specially important in the context of the safety requirements of
> functions or type invariants, as users have to uphold the former and may
> rely on the latter, so we should avoid anything that may create
> confusion.
> 
> Suggested-by: Andreas Hindborg <a.hindborg@...nel.org>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>

>  /// A request for an IRQ line for a given device.
> @@ -112,7 +117,7 @@ impl<'a> IrqRequest<'a> {
>      ///
>      /// - `irq` should be a valid IRQ number for `dev`.
>      pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
> -        // INVARIANT: `irq` is a valid IRQ number for `dev`.
> +        // By function safety requirement, irq` is a valid IRQ number for `dev`.
>          IrqRequest { dev, irq }

When creating a value with an Invariants section, we usually have an
INVARIANT comment. Why was this one removed?

>      }
>  
> @@ -183,6 +188,8 @@ pub fn irq(&self) -> u32 {
>  /// * We own an irq handler whose cookie is a pointer to `Self`.
>  #[pin_data]
>  pub struct Registration<T: Handler + 'static> {
> +    /// We need to drop inner before handler, as we must ensure that the handler
> +    /// is valid until `free_irq` is called.
>      #[pin]
>      inner: Devres<RegistrationInner>,
>  
> @@ -196,7 +203,8 @@ pub struct Registration<T: Handler + 'static> {
>  }
>  
>  impl<T: Handler + 'static> Registration<T> {
> -    /// Registers the IRQ handler with the system for the given IRQ number.
> +    /// Registers the IRQ handler with the system for the IRQ number represented
> +    /// by `request`.
>      pub fn new<'a>(
>          request: IrqRequest<'a>,
>          flags: Flags,
> @@ -208,7 +216,11 @@ pub fn new<'a>(
>              inner <- Devres::new(
>                  request.dev,
>                  try_pin_init!(RegistrationInner {
> -                    // INVARIANT: `this` is a valid pointer to the `Registration` instance
> +                    // INVARIANT: `this` is a valid pointer to the `Registration` instance.
> +                    // INVARIANT: `cookie` is being passed to `request_irq` as
> +                    // the cookie. It is guaranteed to be unique by the type
> +                    // system, since each call to `new` will return a different
> +                    // instance of `Registration`.
>                      cookie: this.as_ptr().cast::<c_void>(),

I believe these comments address the invariants of RegistrationInner. In
that case, we usually place them on the struct:

	// INVARIANT: `this` is a valid pointer to the `Registration` instance.
	// INVARIANT: `cookie` is being passed to `request_irq` as
	// the cookie. It is guaranteed to be unique by the type
	// system, since each call to `new` will return a different
	// instance of `Registration`.
	try_pin_init!(RegistrationInner {

Also, I don't really understand these comments. The first invariant is:

	`self.irq` is the same as the one passed to `request_{threaded}_irq`.

and the justification for that is that `this` is a valid pointer to the
`Registration` instance. That doesn't make sense to me because this
comment talks about `this`/`cookie` when it should talk about `irq`.

>                      irq: {
>                          // SAFETY:

Alice