Message-ID: <2025090807-bootleg-trophy-a031@gregkh>
Date: Mon, 8 Sep 2025 12:17:01 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.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>,
	Danilo Krummrich <dakr@...nel.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Timur Tabi <ttabi@...dia.com>, Benno Lossin <lossin@...nel.org>,
	Dirk Beheme <dirk.behme@...bosch.com>, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v11 2/7] rust: debugfs: Add support for read-only files

On Thu, Sep 04, 2025 at 09:13:53PM +0000, Matthew Maurer wrote:
> Extends the `debugfs` API to support creating read-only files. This
> is done via the `Dir::read_only_file` method, which takes a data object
> that implements the `Writer` trait.
> 
> The file's content is generated by the `Writer` implementation, and the
> file is automatically removed when the returned `File` handle is
> dropped.
> 
> Signed-off-by: Matthew Maurer <mmaurer@...gle.com>
> ---
>  rust/kernel/debugfs.rs          | 148 +++++++++++++++++++++++++++++++++++++++-
>  rust/kernel/debugfs/entry.rs    |  42 ++++++++++++
>  rust/kernel/debugfs/file_ops.rs | 128 ++++++++++++++++++++++++++++++++++
>  rust/kernel/debugfs/traits.rs   |  33 +++++++++
>  4 files changed, 350 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index 65be71600b8eda83c0d313f3d205d0713e8e9510..b28665f58cd6a17e188aef5e8c539f6c7433a3b0 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -8,12 +8,18 @@
>  // When DebugFS is disabled, many parameters are dead. Linting for this isn't helpful.
>  #![cfg_attr(not(CONFIG_DEBUG_FS), allow(unused_variables))]
>  
> -#[cfg(CONFIG_DEBUG_FS)]
>  use crate::prelude::*;
>  use crate::str::CStr;
>  #[cfg(CONFIG_DEBUG_FS)]
>  use crate::sync::Arc;
> +use core::marker::PhantomPinned;
> +use core::ops::Deref;
> +
> +mod traits;
> +pub use traits::Writer;
>  
> +mod file_ops;
> +use file_ops::{FileOps, ReadFile};
>  #[cfg(CONFIG_DEBUG_FS)]
>  mod entry;
>  #[cfg(CONFIG_DEBUG_FS)]
> @@ -53,6 +59,34 @@ fn create(name: &CStr, parent: Option<&Dir>) -> Self {
>          Self()
>      }
>  
> +    /// Creates a DebugFS file which will own the data produced by the initializer provided in
> +    /// `data`.
> +    fn create_file<'a, T, E: 'a>(
> +        &'a self,
> +        name: &'a CStr,
> +        data: impl PinInit<T, E> + 'a,
> +        file_ops: &'static FileOps<T>,
> +    ) -> impl PinInit<File<T>, E> + 'a
> +    where
> +        T: Sync + 'static,
> +    {
> +        let scope = Scope::<T>::new(data, move |data| {
> +            #[cfg(CONFIG_DEBUG_FS)]
> +            if let Some(parent) = &self.0 {
> +                // SAFETY: Because data derives from a scope, and our entry will be dropped before
> +                // the data is dropped, it is guaranteed to outlive the entry we return.
> +                unsafe { Entry::dynamic_file(name, parent.clone(), data, file_ops) }
> +            } else {
> +                Entry::empty()
> +            }
> +        });
> +        try_pin_init! {
> +            File {
> +                scope <- scope
> +            } ? E
> +        }
> +    }
> +
>      /// Create a new directory in DebugFS at the root.
>      ///
>      /// # Examples
> @@ -79,4 +113,116 @@ pub fn new(name: &CStr) -> Self {
>      pub fn subdir(&self, name: &CStr) -> Self {
>          Dir::create(name, Some(self))
>      }
> +
> +    /// Creates a read-only file in this directory.
> +    ///
> +    /// The file's contents are produced by invoking [`Writer::write`] on the value initialized by
> +    /// `data`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// # use kernel::prelude::*;
> +    /// # let dir = Dir::new(c_str!("my_debugfs_dir"));
> +    /// let file = KBox::pin_init(dir.read_only_file(c_str!("foo"), 200), GFP_KERNEL)?;
> +    /// // "my_debugfs_dir/foo" now contains the number 200.
> +    /// // The file is removed when `file` is dropped.
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn read_only_file<'a, T, E: 'a>(
> +        &'a self,
> +        name: &'a CStr,
> +        data: impl PinInit<T, E> + 'a,
> +    ) -> impl PinInit<File<T>, E> + 'a
> +    where
> +        T: Writer + Send + Sync + 'static,
> +    {
> +        let file_ops = &<T as ReadFile<_>>::FILE_OPS;
> +        self.create_file(name, data, file_ops)
> +    }
> +}
> +
> +#[pin_data]
> +/// Handle to a DebugFS scope, which ensures that attached `data` will outlive the provided
> +/// [`Entry`] without moving.
> +/// Currently, this is used to back [`File`] so that its `read` and/or `write` implementations
> +/// can assume that their backing data is still alive.
> +struct Scope<T> {
> +    // This order is load-bearing for drops - `_entry` must be dropped before `data`.
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    _entry: Entry,
> +    #[pin]
> +    data: T,
> +    // Even if `T` is `Unpin`, we still can't allow it to be moved.
> +    #[pin]
> +    _pin: PhantomPinned,
> +}
> +
> +#[pin_data]
> +/// Handle to a DebugFS file, owning its backing data.
> +///
> +/// When dropped, the DebugFS file will be removed and the attached data will be dropped.
> +pub struct File<T> {
> +    #[pin]
> +    scope: Scope<T>,
> +}
> +
> +#[cfg(not(CONFIG_DEBUG_FS))]
> +impl<'b, T: 'b> Scope<T> {
> +    fn new<E: 'b, F>(data: impl PinInit<T, E> + 'b, init: F) -> impl PinInit<Self, E> + 'b
> +    where
> +        F: for<'a> FnOnce(&'a T) -> Entry + 'b,
> +    {
> +        try_pin_init! {
> +            Self {
> +                data <- data,
> +                _pin: PhantomPinned
> +            } ? E
> +        }
> +        .pin_chain(|scope| {
> +            init(&scope.data);
> +            Ok(())
> +        })
> +    }
> +}
> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +impl<'b, T: 'b> Scope<T> {
> +    fn entry_mut(self: Pin<&mut Self>) -> &mut Entry {
> +        // SAFETY: _entry is not structurally pinned.
> +        unsafe { &mut Pin::into_inner_unchecked(self)._entry }
> +    }
> +
> +    fn new<E: 'b, F>(data: impl PinInit<T, E> + 'b, init: F) -> impl PinInit<Self, E> + 'b
> +    where
> +        F: for<'a> FnOnce(&'a T) -> Entry + 'b,
> +    {
> +        try_pin_init! {
> +            Self {
> +                _entry: Entry::empty(),
> +                data <- data,
> +                _pin: PhantomPinned
> +            } ? E
> +        }
> +        .pin_chain(|scope| {
> +            *scope.entry_mut() = init(&scope.data);
> +            Ok(())
> +        })
> +    }
> +}
> +
> +impl<T> Deref for Scope<T> {
> +    type Target = T;
> +    fn deref(&self) -> &T {
> +        &self.data
> +    }
> +}
> +
> +impl<T> Deref for File<T> {
> +    type Target = T;
> +    fn deref(&self) -> &T {
> +        &self.scope
> +    }
>  }
> diff --git a/rust/kernel/debugfs/entry.rs b/rust/kernel/debugfs/entry.rs
> index d2fba0e65e20e954e2a33e776b872bac4adb12e8..227fa50b7a79aeab49779e54b8c4241f455777c3 100644
> --- a/rust/kernel/debugfs/entry.rs
> +++ b/rust/kernel/debugfs/entry.rs
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (C) 2025 Google LLC.
>  
> +use crate::debugfs::file_ops::FileOps;
> +use crate::ffi::c_void;
>  use crate::str::CStr;
>  use crate::sync::Arc;
>  
> @@ -40,6 +42,46 @@ pub(crate) fn dynamic_dir(name: &CStr, parent: Option<Arc<Self>>) -> Self {
>          }
>      }
>  
> +    /// # Safety
> +    ///
> +    /// * `data` must outlive the returned `Entry`.
> +    pub(crate) unsafe fn dynamic_file<T>(
> +        name: &CStr,
> +        parent: Arc<Self>,
> +        data: &T,
> +        file_ops: &'static FileOps<T>,
> +    ) -> Self {
> +        // SAFETY: The invariants of this function's arguments ensure the safety of this call.
> +        // * `name` is a valid C string by the invariants of `&CStr`.
> +        // * `parent.as_ptr()` is a pointer to a valid `dentry` by invariant.
> +        // * The caller guarantees that `data` will outlive the returned `Entry`.
> +        // * The guarantees on `FileOps` assert the vtable will be compatible with the data we have
> +        //   provided.
> +        let entry = unsafe {
> +            bindings::debugfs_create_file_full(
> +                name.as_char_ptr(),
> +                file_ops.mode(),
> +                parent.as_ptr(),
> +                core::ptr::from_ref(data) as *mut c_void,
> +                core::ptr::null(),
> +                &**file_ops,
> +            )
> +        };
> +
> +        Entry {
> +            entry,
> +            _parent: Some(parent),
> +        }
> +    }
> +
> +    /// Constructs a placeholder DebugFS [`Entry`].
> +    pub(crate) fn empty() -> Self {
> +        Self {
> +            entry: core::ptr::null_mut(),
> +            _parent: None,
> +        }
> +    }
> +
>      /// Returns the pointer representation of the DebugFS directory.
>      ///
>      /// # Guarantees
> diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..c2fbef96580eaa2fab7cc8c1ba559c3284d12e1b
> --- /dev/null
> +++ b/rust/kernel/debugfs/file_ops.rs
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2025 Google LLC.
> +
> +use super::Writer;
> +use crate::prelude::*;
> +use crate::seq_file::SeqFile;
> +use crate::seq_print;
> +use core::fmt::{Display, Formatter, Result};
> +use core::marker::PhantomData;
> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +use core::ops::Deref;
> +
> +/// # Invariant
> +///
> +/// `FileOps<T>` will always contain an `operations` which is safe to use for a file backed
> +/// off an inode which has a pointer to a `T` in its private data that is safe to convert
> +/// into a reference.
> +pub(super) struct FileOps<T> {
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    operations: bindings::file_operations,
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    mode: u16,
> +    _phantom: PhantomData<T>,
> +}
> +
> +impl<T> FileOps<T> {
> +    /// # Safety
> +    ///
> +    /// The caller asserts that the provided `operations` is safe to use for a file whose
> +    /// inode has a pointer to `T` in its private data that is safe to convert into a reference.
> +    const unsafe fn new(operations: bindings::file_operations, mode: u16) -> Self {
> +        Self {
> +            #[cfg(CONFIG_DEBUG_FS)]
> +            operations,
> +            #[cfg(CONFIG_DEBUG_FS)]
> +            mode,
> +            _phantom: PhantomData,
> +        }
> +    }
> +
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    pub(crate) const fn mode(&self) -> u16 {
> +        self.mode
> +    }
> +}
> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +impl<T> Deref for FileOps<T> {
> +    type Target = bindings::file_operations;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.operations
> +    }
> +}
> +
> +struct WriterAdapter<T>(T);
> +
> +impl<'a, T: Writer> Display for WriterAdapter<&'a T> {
> +    fn fmt(&self, f: &mut Formatter<'_>) -> Result {
> +        self.0.write(f)
> +    }
> +}
> +
> +/// 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 have any unique references alias it during the call.
> +/// * `file` must point to a live, not-yet-initialized file object.
> +unsafe extern "C" fn writer_open<T: Writer + Sync>(
> +    inode: *mut bindings::inode,
> +    file: *mut bindings::file,
> +) -> c_int {
> +    // SAFETY: The caller ensures that `inode` is a valid pointer.
> +    let data = unsafe { (*inode).i_private };
> +    // 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(writer_act::<T>), data) }
> +}
> +
> +/// 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 valid pointer to a `T` which may
> +/// not have any unique references alias it during the call.
> +unsafe extern "C" fn writer_act<T: Writer + Sync>(
> +    seq: *mut bindings::seq_file,
> +    _: *mut c_void,
> +) -> c_int {
> +    // SAFETY: By caller precondition, this pointer is valid pointer to a `T`, and
> +    // there are not and will not be any unique references until we are done.
> +    let data = unsafe { &*((*seq).private.cast::<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, "{}", WriterAdapter(data));
> +    0
> +}
> +
> +// Work around lack of generic const items.
> +pub(crate) trait ReadFile<T> {
> +    const FILE_OPS: FileOps<T>;
> +}
> +
> +impl<T: Writer + Sync> ReadFile<T> for T {
> +    const FILE_OPS: FileOps<T> = {
> +        let operations = bindings::file_operations {
> +            read: Some(bindings::seq_read),
> +            llseek: Some(bindings::seq_lseek),
> +            release: Some(bindings::single_release),
> +            open: Some(writer_open::<Self>),
> +            // SAFETY: `file_operations` supports zeroes in all fields.
> +            ..unsafe { core::mem::zeroed() }
> +        };
> +        // SAFETY: `operations` is all stock `seq_file` implementations except for `writer_open`.
> +        // `open`'s only requirement beyond what is provided to all open functions is that the
> +        // inode's data pointer must point to a `T` that will outlive it, which matches the
> +        // `FileOps` requirements.
> +        unsafe { FileOps::new(operations, 0o400) }
> +    };
> +}
> diff --git a/rust/kernel/debugfs/traits.rs b/rust/kernel/debugfs/traits.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..0e6e461324de42a3d80b692264d50e78a48f561d
> --- /dev/null
> +++ b/rust/kernel/debugfs/traits.rs
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2025 Google LLC.
> +
> +//! Traits for rendering or updating values exported to DebugFS.
> +
> +use crate::sync::Mutex;
> +use core::fmt::{self, Debug, Formatter};
> +
> +/// A trait for types that can be written into a string.
> +///
> +/// This works very similarly to `Debug`, and is automatically implemented if `Debug` is
> +/// implemented for a type. It is also implemented for any writable type inside a `Mutex`.
> +///
> +/// The derived implementation of `Debug` [may
> +/// change](https://doc.rust-lang.org/std/fmt/trait.Debug.html#stability)
> +/// between Rust versions, so if stability is key for your use case, please implement `Writer`
> +/// explicitly instead.
> +pub trait Writer {
> +    /// Formats the value using the given formatter.
> +    fn write(&self, f: &mut Formatter<'_>) -> fmt::Result;
> +}
> +
> +impl<T: Writer> Writer for Mutex<T> {
> +    fn write(&self, f: &mut Formatter<'_>) -> fmt::Result {
> +        self.lock().write(f)
> +    }
> +}
> +
> +impl<T: Debug> Writer for T {
> +    fn write(&self, f: &mut Formatter<'_>) -> fmt::Result {
> +        writeln!(f, "{self:?}")
> +    }
> +}

I tried using this in a "tiny" test module I had written, and I get the
following build error:

   --> samples/rust/rust_debugfs2.rs:64:53
    |
64  |         _file = root.read_only_file(c_str!("name"), &hw_soc_info.name);
    |                      --------------                 ^^^^^^^^^^^^^^^^^ expected `&u32`, found `&&CStr`
    |                      |
    |                      arguments to this method are incorrect
    |
    = note: expected reference `&u32`
               found reference `&&'static kernel::prelude::CStr`

I'm trying to "just" print a CStr, which is defined as:

struct HwSocInfo {
    id: u32,
    ver: u32,
    raw_id: u32,
    foundry: u32,
    name: &'static CStr,
}

Is this just a "user is holding it wrong" error on my side, or can this api not
handle CStr values?

thanks,

greg k-h

Powered by blists - more mailing lists