Re: [PATCH v3 2/4] rust: debugfs: Bind file creation for long-lived Display

[Danilo Krummrich <dakr@kernel.org>]

On Thu, May 01, 2025 at 10:47:42PM +0000, Matthew Maurer wrote:
> +/// Handle to a DebugFS file.
> +#[repr(transparent)]
> +pub struct File(ManuallyDrop<Dir>);

Same as with SubDir, please keep your original approach with keep().

Exposing this as a separate type is much better, but I still find it a bit weird
that it uses Dir internally, which still provides methods that are not
applicable.

I think it would be good to have the following types instead:

	// Generic wrapper around the dentry pointer.
	struct Entry;

	// Based on Entry; provides Dir specific methods.
	struct Dir;

	// Based on Dir; implements Keep.
	struct SubDir;

	// Based on Entry; implements Keep.
	struct File;

	// Common trait that implements keep().
	trait Keep;

> +impl File {
> +    /// Remove the file from DebugFS.
> +    ///
> +    /// # Examples
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let dir = Dir::new(c_str!("foo"));
> +    /// {
> +    ///     let file = dir.display_file(c_str!("bar"), &0);
> +    ///     // "foo/bar" is created.
> +    /// }
> +    /// // "foo/bar" still exists.
> +    /// {
> +    ///     let file = dir.display_file(c_str!("baz"), &0);
> +    ///     // "foo/baz" is created.
> +    ///     file.remove();
> +    ///     // "foo/baz" is gone.
> +    /// }
> +    pub fn remove(self) {
> +        drop(ManuallyDrop::into_inner(self.0))
> +    }
> +}

Same as with my comment on Dir::subdir(), it really gets confusing if we invert
the normal drop() logic. Removing the file when it is dropped and keeping it
when calling keep() is much more intuitive..

> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +mod helpers {
> +    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 {
> +        // 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) }

Please split up unsafe operations.

> +    }
> +
> +    /// 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 {
> +        // 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) };

This creates an intermediate reference to private, which is UB. Please use
addr_of! instead.