Message-Id: <DCCKZ2VUONAR.R50ZFLTWE4T@kernel.org>
Date: Tue, 26 Aug 2025 20:45:41 +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>, "Dirk Beheme"
 <dirk.behme@...bosch.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v10 2/7] rust: debugfs: Add support for read-only files

On Wed Aug 20, 2025 at 12:53 AM CEST, Matthew Maurer wrote:
> +    /// Creates a DebugFS file which will own the data produced by the initializer provided in
> +    /// `data`.
> +    fn create_file<'a, T: Sync + 'static, E: 'a, TI: PinInit<T, E> + 'a>(
> +        &'a self,
> +        name: &'a CStr,
> +        data: TI,

Can you please use `impl PinInit<T, E> + 'a` directly? It's a bit more in line
with how we usually write this and I also think it makes things quite a bit more
obvious compared to hiding it behind the `TI` generic.

Also, can you please use a where clause when the trait and lifetime bounds are
getting a bit lengthy? I think for this method this is already justified.

Obviously, this applies to the whole patch (and likely subsequent ones).

> +        file_ops: &'static FileOps<T>,
> +    ) -> impl PinInit<File<T>, E> + 'a {
> +        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 +110,113 @@ 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 [`Render::render`] 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: Render + Send + Sync + 'static, E: 'a, TI: PinInit<T, E> + 'a>(
> +        &'a self,
> +        name: &'a CStr,
> +        data: TI,
> +    ) -> impl PinInit<File<T>, E> + 'a {
> +        let file_ops = &<T as ReadFile<_>>::FILE_OPS;
> +        self.create_file(name, data, file_ops)
> +    }
> +}
> +
> +#[pin_data]
> +/// Handle to a DebugFS scope, which allows a variety of DebugFS files/directories to hang off a
> +/// single structure.
> +pub struct Scope<T> {

I think this doesn't need the Scope indirection just yet, but fine for me to add
it right away.

However, please make sure to expand the documentation a bit. You probably
don't want to mention where this is exactly going just yet, since technically
those patches should work out standalone in case the more sophisticated scope
stuff doesn't land.

But, I think you can still describe a bit more in detail that a debugfs::Scope
handles the lifetime of some private data T for a debugfs::Entry.

Subsequently I would mention that this is used for file entries only.

> +    // 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<T> Scope<T> {
> +    fn new<E, TI: PinInit<T, E>, F: for<'a> FnOnce(&'a T)>(
> +        data: TI,
> +        init: F,
> +    ) -> impl PinInit<Self, E> {
> +        try_pin_init! {
> +            Self {
> +                data <- data,
> +                _pin: PhantomPinned
> +            } ? E
> +        }
> +        .pin_chain(|scope| {
> +            init(&scope.data);
> +            Ok(())
> +        })
> +    }
> +}
> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +impl<T> Scope<T> {
> +    fn entry_mut(self: Pin<&mut Self>) -> &mut Entry {
> +        // SAFETY: _entry is not structurally pinned

Please end with a period.

> +        unsafe { &mut Pin::into_inner_unchecked(self)._entry }
> +    }

Missing empty line.

> +    fn new<'b, E: 'b, TI: PinInit<T, E> + 'b, F: for<'a> FnOnce(&'a T) -> Entry + 'b>(
> +        data: TI,
> +        init: F,
> +    ) -> impl PinInit<Self, E> + 'b
> +    where
> +        T: 'b,
> +    {
> +        try_pin_init! {
> +            Self {
> +                _entry: Entry::empty(),
> +                data <- data,
> +                _pin: PhantomPinned
> +            } ? E
> +        }
> +        .pin_chain(|scope| {
> +            *scope.entry_mut() = init(&scope.data);
> +            Ok(())
> +        })
> +    }
> +}

<snip>

> +/// # 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

Missing empty line.

> +    /// 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,
> +        }
> +    }

Same here...

> +    #[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;

...and here.

> +    fn deref(&self) -> &Self::Target {
> +        &self.operations
> +    }
> +}
> +
> +struct RenderAdapter<T>(T);
> +
> +impl<'a, T: Render> Display for RenderAdapter<&'a T> {
> +    fn fmt(&self, f: &mut Formatter<'_>) -> Result {
> +        self.0.render(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 render_open<T: Render + 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(render_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 live pointer to a `T` which may

We usually say "valid".

> +/// not have any unique references alias it during the call.
> +unsafe extern "C" fn render_act<T: Render + Sync>(
> +    seq: *mut bindings::seq_file,
> +    _: *mut c_void,
> +) -> c_int {
> +    // SAFETY: By caller precondition, this pointer is live, points to a value of type `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, "{}", RenderAdapter(data));
> +    0
> +}
> +
> +// Work around lack of generic const items.
> +pub(crate) trait ReadFile<T> {
> +    const FILE_OPS: FileOps<T>;
> +}
> +
> +impl<T: Render + 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(render_open::<Self>),
> +            // SAFETY: `file_operations` supports zeroes in all fields.
> +            ..unsafe { core::mem::zeroed() }
> +        };
> +        // SAFETY: `operations` is all stock `seq_file` implementations except for `render_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..2939e18e3dda39571cd7255505e5f605f0e3d154
> --- /dev/null
> +++ b/rust/kernel/debugfs/traits.rs
> @@ -0,0 +1,28 @@
> +// 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 rendered 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 renderable type inside a `Mutex`.

I think the blanket impls make sense, but we should probably add a note that the
contents derived via #[derive(Debug)] are not guaranteed to be stable [1].

[1] https://doc.rust-lang.org/std/fmt/trait.Debug.html#stability

> +pub trait Render {

Should we really call this Render? Maybe it'd be better to call it
debugfs::Writer?

Yes, I know it's called in read(), but that's what the kernel does, it writes
on read(). :)

> +    /// Formats the value using the given formatter.
> +    fn render(&self, f: &mut Formatter<'_>) -> fmt::Result;
> +}
> +
> +impl<T: Render> Render for Mutex<T> {
> +    fn render(&self, f: &mut Formatter<'_>) -> fmt::Result {
> +        self.lock().render(f)
> +    }
> +}
> +
> +impl<T: Debug> Render for T {
> +    fn render(&self, f: &mut Formatter<'_>) -> fmt::Result {
> +        writeln!(f, "{self:?}")
> +    }
> +}

Powered by blists - more mailing lists