linux-kernel - Re: [PATCH v6 2/5] rust: debugfs: Bind file creation for long-lived Display

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: <aFLdmYLyDucTAiZx@pollux>
Date: Wed, 18 Jun 2025 17:39:05 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Matthew Maurer <mmaurer@...gle.com>
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>,
	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>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Timur Tabi <ttabi@...dia.com>, Benno Lossin <lossin@...nel.org>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v6 2/5] rust: debugfs: Bind file creation for long-lived
 Display

On Wed, Jun 18, 2025 at 02:28:14AM +0000, Matthew Maurer wrote:
> diff --git a/rust/kernel/debugfs/display_file.rs b/rust/kernel/debugfs/display_file.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..65e37e34d7b587482492dc296637a3bc517b9fe3
> --- /dev/null
> +++ b/rust/kernel/debugfs/display_file.rs
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2025 Google LLC.
> +
> +use crate::seq_file::SeqFile;
> +use crate::seq_print;
> +use core::fmt::Display;
> +
> +/// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`.
> +///
> +/// # Safety
> +///
> +/// * `inode`'s private pointer must point to a value of type `T` which will outlive the `inode`
> +///   and will not be mutated during this call.
> +/// * `file` must point to a live, not-yet-initialized file object.
> +pub(crate) unsafe extern "C" fn display_open<T: Display>(
> +    inode: *mut bindings::inode,
> +    file: *mut bindings::file,
> +) -> i32 {

Please use kernel::ffi::c_int instead.

> +    // SAFETY:
> +    // * `file` is acceptable by caller precondition.
> +    // * `print_act` will be called on a `seq_file` with private data set to the third argument,
> +    //   so we meet its safety requirements.
> +    // * The `data` pointer passed in the third argument is a valid `T` pointer that outlives
> +    //   this call by caller preconditions.
> +    unsafe { bindings::single_open(file, Some(display_act::<T>), (*inode).i_private) }
> +}
> +
> +/// Prints private data stashed in a seq_file to that seq file.
> +///
> +/// # Safety
> +///
> +/// `seq` must point to a live `seq_file` whose private data is a live pointer to a `T` which is
> +/// not being mutated.
> +pub(crate) unsafe extern "C" fn display_act<T: Display>(
> +    seq: *mut bindings::seq_file,
> +    _: *mut core::ffi::c_void,
> +) -> i32 {

kernel::ffi::c_int

> +    // SAFETY: By caller precondition, this pointer is live, points to a value of type `T`, and
> +    // is not being mutated.
> +    let data = unsafe { &*((*seq).private as *mut T) };
> +    // SAFETY: By caller precondition, `seq_file` points to a live `seq_file`, so we can lift
> +    // it.
> +    let seq_file = unsafe { SeqFile::from_raw(seq) };
> +    seq_print!(seq_file, "{}", data);
> +    0
> +}
> +
> +// Work around lack of generic const items.
> +pub(crate) trait DisplayFile: Display + Sized {
> +    const VTABLE: bindings::file_operations = bindings::file_operations {
> +        read: Some(bindings::seq_read),
> +        llseek: Some(bindings::seq_lseek),
> +        release: Some(bindings::single_release),
> +        open: Some(display_open::<Self> as _),

Why the cast? That shouldn't be needed.

> +        // SAFETY: `file_operations` supports zeroes in all fields.
> +        ..unsafe { core::mem::zeroed() }
> +    };
> +}