[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aBRrniLfCzWX7nbR@pollux>
Date: Fri, 2 May 2025 08:52:14 +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>,
Benno Lossin <benno.lossin@...ton.me>,
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>, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 2/4] rust: debugfs: Bind file creation for long-lived
Display
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.
Powered by blists - more mailing lists