rust: DebugFS Bindings
Subsystems (2)
v3 cover rust: DebugFS Bindings
This series provides safe DebugFS bindings for Rust, with a sample driver using them.
This series currently only supports referencing 'static lifetime objects, because we need to know the objects outlive the DebugFS files. A subsequent series may be able to relax this.
* Directories created in the root of debugfs are assumed to be owning, and will clean up after themselves when dropped.
* Subdirectories are assumed non-owning, remaining alive until their containing directory is cleaned up. They can be promoted to owning variants if needed.
* Files are non-owning, but can be manually removed by handle if needed.
---
Changes in v3:
- Split Dir into Dir/SubDir/File to improve API.
- Add "." to end of all comments.
- Convert INVARIANT to # Invariants on types.
- Add backticks everywhere I found variables/types in my comments.
- Promoted invariant comment to doc comment.
- Extended sample commenting to make it clearer what is happening.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Drop support for builder / pinned bindings in initial series
- Remove ARef usage to abstract the dentry nature of handles
- Remove error handling to discourage users from caring whether DebugFS
is enabled.
- Support CONFIG_DEBUG_FS=n while leaving the API available
- Fixed mistaken decimal/octal mixup
- Doc/comment cleanup
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Matthew Maurer (4):
rust: debugfs: Bind DebugFS directory creation
rust: debugfs: Bind file creation for long-lived Display
rust: debugfs: Support format hooks
rust: samples: Add debugfs sample
MAINTAINERS | 2 + rust/bindings/bindings_helper.h | 1 + rust/kernel/debugfs.rs | 405 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + samples/rust/Kconfig | 11 ++ samples/rust/Makefile | 1 + samples/rust/rust_debugfs.rs | 54 ++++++ 7 files changed, 475 insertions(+)
---
base-commit: b6a7783d306baf3150ac54cd5124f6e85dd375b0
change-id: 20250428-debugfs-rust-3cd5c97eb7d1
Best regards,
Signature · 1 line
Matthew Maurer <[email protected]>
PATCH v3 1/4 rust: debugfs: Bind DebugFS directory creation
Support creating DebugFS directories and subdirectories. Similar to the original DebugFS API, errors are hidden.
By default, when a root directory handle leaves scope, it will be cleaned up.
Subdirectories will by default persist until their root directory is cleaned up, but can be converted into a root directory if a scoped lifecycle is desired.
--- MAINTAINERS | 1 + rust/bindings/bindings_helper.h | 1 + rust/kernel/debugfs.rs | 155 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 4 files changed, 158 insertions(+)
MAINTAINERS +1 -0
@@ -7271,6 +7271,7 @@ F: include/linux/kobj* F: include/linux/property.h F: include/linux/sysfs.h F: lib/kobj* +F: rust/kernel/debugfs.rs F: rust/kernel/device.rs F: rust/kernel/device_id.rs F: rust/kernel/devres.rs
rust/bindings/bindings_helper.h +1 -0
@@ -13,6 +13,7 @@#include <linux/blkdev.h> #include <linux/cpumask.h> #include <linux/cred.h> +#include <linux/debugfs.h> #include <linux/device/faux.h> #include <linux/dma-mapping.h> #include <linux/errname.h>
rust/kernel/debugfs.rs new file +155 -0
155 lines not on this page · raw message
rust/kernel/lib.rs +1 -0
@@ -45,6 +45,7 @@#[doc(hidden)] pub mod build_assert;pub mod cred; +pub mod debugfs;pub mod device;pub mod device_id;pub mod devres;
Signature · 1 line
2.49.0.906.g1f30a19c02-goog
Danilo Krummrich
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
rust/kernel/debugfs.rs line 21
Quoted 9 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +9
-0+/// Owning handle to a DebugFS directory. +/// +/// This directory will be cleaned up when it goes out of scope. +/// +/// # Invariants +/// +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`. +#[repr(transparent)] +pub struct Dir (#[cfg( CONFIG_DEBUG_FS )] *mut bindings::dentry );
Should probably use Opaque instead of a raw pointer.
Quoted 1 line
+// SAFETY: Dir is just a
dentryunder the hood, which the API promises can be transferred
[Dir]
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+// between threads. +unsafe impl Send for Dir {} + +// SAFETY: All the native functions we re-export use interior locking, and the contents of the +// struct are opaque to Rust. +unsafe impl Sync for Dir {} + +impl Dir { +/// Create a new directory in DebugFS at the root. +///
rust/kernel/debugfs.rs line 43
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// } +/// // It does not exist here.
This ready like an explanation for scopes; I think we should drop those comments and the scope.
Quoted 20 lines
rust/kernel/debugfs.rs new file +20
-0+/// ``` +pub fn new (name: &CStr ) ->Self { +Self ::create (name,None ) + } + +/// Create a DebugFS subdirectory. +/// +/// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope. +/// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`]. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// { +/// let child = parent.subdir(c_str!("child"));
rust/kernel/debugfs.rs line 73
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// // The path "parent/child" exists in DebugFS here. +/// } +/// // The path "parent/child" still exists. +/// { +/// let child2 = Dir::from(parent.subdir(c_str!("child2"))); +/// // The path "parent/child2" exists in DebugFS here. +/// } +/// // The path "parent/child2" is gone. +/// } +/// // None of the paths exist here.
I think the fact that you need all those comments here proves that it's not really intuitive. Please see me comment on SubDir below.
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// ``` +pub fn subdir (&self, name: &CStr ) ->SubDir { +SubDir ::new (Self ::create (name,Some (self))) + } + +/// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root. +#[cfg( CONFIG_DEBUG_FS )] +fn create (name: &CStr , parent:Option <&Self >) ->Self { +let parent_ptr =match parent { +Some (parent) => parent.as_ptr (),
rust/kernel/debugfs.rs line 93
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+ None => core::ptr::null_mut(), + }; +// SAFETY: +// * `name` argument points to a NUL-terminated string that lives across the call, by +// invariants of `&CStr`. +// * If `parent` is `None`, `parent` accepts null pointers to mean create at root. +// * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers. +// * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer, +// so we can call `Self::from_ptr`. +unsafe {Self ::from_ptr (bindings::debugfs_create_dir (name.as_char_ptr (), parent_ptr)) }
Please split up in two calls, such that we don't have two unsafe function calls in a single unsafe block.
Quoted 12 lines
rust/kernel/debugfs.rs new file +12
-0+ } + +#[cfg(not( CONFIG_DEBUG_FS ))] +fn create (_name: &CStr , _parent:Option <&Self >) ->Self { +Self () + } + +/// Constructs a new DebugFS [`Dir`] from the underlying pointer. +/// +/// # Safety +/// +/// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
rust/kernel/debugfs.rs line 115
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// live DebugFS directory. +#[cfg( CONFIG_DEBUG_FS )] +unsafe fn from_ptr (ptr: *mut bindings::dentry ) ->Self { +Self (ptr) + } + +/// Returns the pointer representation of the DebugFS directory. +/// +/// Due to the type invariant, the value returned from this function will always be an error +/// code, NUL, or a live DebugFS directory.
Maybe put this in a '# Guarantees' section.
Quoted 1 line
+ // If this function is ever needed with
not(CONFIG_DEBUG_FS), hardcode it to returnENODEV.
I think you mean ERR_PTR(ENODEV).
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+#[cfg( CONFIG_DEBUG_FS )] +fn as_ptr (&self) -> *mut bindings::dentry { + self.0 + } +} + +impl Drop for Dir { +fn drop (&mut self) { +// SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries. +// `as_ptr` guarantees that the pointer is of this form.
rust/kernel/debugfs.rs line 136
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+#[cfg( CONFIG_DEBUG_FS )] +unsafe { + bindings::debugfs_remove (self.as_ptr ()) + } + } +} + +/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);
I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with keep() consuming the SubDir was much better; please keep this approach.
Greg Kroah-Hartman
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 144 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 21
Quoted 9 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +9
-0+/// Owning handle to a DebugFS directory. +/// +/// This directory will be cleaned up when it goes out of scope. +/// +/// # Invariants +/// +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`. +#[repr(transparent)] +pub struct Dir (#[cfg( CONFIG_DEBUG_FS )] *mut bindings::dentry );Should probably use Opaque instead of a raw pointer.
Quoted 1 line
+// SAFETY: Dir is just a
dentryunder the hood, which the API promises can be transferred[
Dir]Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+// between threads. +unsafe impl Send for Dir {} + +// SAFETY: All the native functions we re-export use interior locking, and the contents of the +// struct are opaque to Rust. +unsafe impl Sync for Dir {} + +impl Dir { +/// Create a new directory in DebugFS at the root. +/// rust/kernel/debugfs.rs line 43
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// } +/// // It does not exist here. This ready like an explanation for scopes; I think we should drop those comments and the scope.
Quoted 20 lines
rust/kernel/debugfs.rs new file +20
-0+/// ``` +pub fn new (name: &CStr ) ->Self { +Self ::create (name,None ) + } + +/// Create a DebugFS subdirectory. +/// +/// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope. +/// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`]. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// { +/// let child = parent.subdir(c_str!("child")); rust/kernel/debugfs.rs line 73
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// // The path "parent/child" exists in DebugFS here. +/// } +/// // The path "parent/child" still exists. +/// { +/// let child2 = Dir::from(parent.subdir(c_str!("child2"))); +/// // The path "parent/child2" exists in DebugFS here. +/// } +/// // The path "parent/child2" is gone. +/// } +/// // None of the paths exist here. I think the fact that you need all those comments here proves that it's not really intuitive. Please see me comment on SubDir below.
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// ``` +pub fn subdir (&self, name: &CStr ) ->SubDir { +SubDir ::new (Self ::create (name,Some (self))) + } + +/// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root. +#[cfg( CONFIG_DEBUG_FS )] +fn create (name: &CStr , parent:Option <&Self >) ->Self { +let parent_ptr =match parent { +Some (parent) => parent.as_ptr (),rust/kernel/debugfs.rs line 93
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+ None => core::ptr::null_mut(), + }; +// SAFETY: +// * `name` argument points to a NUL-terminated string that lives across the call, by +// invariants of `&CStr`. +// * If `parent` is `None`, `parent` accepts null pointers to mean create at root. +// * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers. +// * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer, +// so we can call `Self::from_ptr`. +unsafe {Self ::from_ptr (bindings::debugfs_create_dir (name.as_char_ptr (), parent_ptr)) }Please split up in two calls, such that we don't have two unsafe function calls in a single unsafe block.
Quoted 12 lines
rust/kernel/debugfs.rs new file +12
-0+ } + +#[cfg(not( CONFIG_DEBUG_FS ))] +fn create (_name: &CStr , _parent:Option <&Self >) ->Self { +Self () + } + +/// Constructs a new DebugFS [`Dir`] from the underlying pointer. +/// +/// # Safety +/// +/// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a rust/kernel/debugfs.rs line 115
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// live DebugFS directory. +#[cfg( CONFIG_DEBUG_FS )] +unsafe fn from_ptr (ptr: *mut bindings::dentry ) ->Self { +Self (ptr) + } + +/// Returns the pointer representation of the DebugFS directory. +/// +/// Due to the type invariant, the value returned from this function will always be an error +/// code, NUL, or a live DebugFS directory. Maybe put this in a '# Guarantees' section.
Quoted 1 line
+ // If this function is ever needed with
not(CONFIG_DEBUG_FS), hardcode it to returnENODEV.I think you mean
ERR_PTR(ENODEV).Quoted 16 lines
rust/kernel/debugfs.rs new file +16
-0+#[cfg( CONFIG_DEBUG_FS )] +fn as_ptr (&self) -> *mut bindings::dentry { + self.0 + } +} + +impl Drop for Dir { +fn drop (&mut self) { +// SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries. +// `as_ptr` guarantees that the pointer is of this form. +#[cfg( CONFIG_DEBUG_FS )] +unsafe { + bindings::debugfs_remove (self.as_ptr ()) + } + } +}
Quoted 8 lines
rust/kernel/debugfs.rs line 136
Quoted 4 lines
rust/kernel/debugfs.rs new file +4
-0+ +/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.
Wait, let's step back. Why do we care about the difference between a "subdir" and a "dir"? They both are the same thing, and how do you describe a subdir of a subdir? :)
Why the "split" here, that just adds additional mental energy to both someone using the api, as well as someone having to review someone using it. A directory is a directory, no matter where in debugfs it lives, so let's just keep it as simple as possible please.
thanks,
greg k-h
Danilo Krummrich
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 10 lines · Greg Kroah-Hartman
Quoted 8 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 136
Quoted 3 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +3
-0+/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.
Quoted 3 lines
Wait, let's step back. Why do we care about the difference between a "subdir" and a "dir"? They both are the same thing, and how do you describe a subdir of a subdir? :)
We care about the difference, because Dir originally had keep() which drops the Dir instance without actually removing it. For subdirs this is fine, since they'll be cleaned up when the parent is removed.
However, we don't want users to be able to call keep() on the directory that has been created first, since if that's done we loose our root anchor to ever free the tree, which almost always would be a bug.
Please also see [1] and subsequent replies.
A subdir of a subdir would still be a SubDir.
Greg Kroah-Hartman
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 11 lines · Danilo Krummrich
Quoted 10 lines · Greg Kroah-Hartman
Quoted 8 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 136
Quoted 3 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +3
-0+/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.
Quoted 7 lines
Quoted 3 lines
Wait, let's step back. Why do we care about the difference between a "subdir" and a "dir"? They both are the same thing, and how do you describe a subdir of a subdir? :)
We care about the difference, because Dir originally had
keep()which drops the Dir instance without actually removing it. For subdirs this is fine, since they'll be cleaned up when the parent is removed.
But does that mean a subdir can not be cleaned up without dropping the parent first? For many subsystems, they make a "root" debugfs directory, and then add/remove subdirs all the time within that.
Quoted 3 lines
However, we don't want users to be able to call
keep()on the directory that has been created first, since if that's done we loose our root anchor to ever free the tree, which almost always would be a bug.
Then do a call to debugfs_lookup_and_remove() which is what I really recommend doing for any C user anyway. That way no dentry is ever "stored" anywhere.
Anyway, if Dir always has an implicit keep() call in it, then I guess this is ok. Let's see how this shakes out with some real-world users. We can always change it over time if it gets unwieldy.
thanks,
greg k-h
Danilo Krummrich
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · Danilo Krummrich
Quoted 13 lines · Greg Kroah-Hartman
Quoted 8 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 136
Quoted 3 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +3
-0+/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.Wait, let's step back. Why do we care about the difference between a "subdir" and a "dir"? They both are the same thing, and how do you describe a subdir of a subdir? :)
Quoted 7 lines
Quoted 3 lines
We care about the difference, because Dir originally had
keep()which drops the Dir instance without actually removing it. For subdirs this is fine, since they'll be cleaned up when the parent is removed.But does that mean a subdir can not be cleaned up without dropping the parent first? For many subsystems, they make a "root" debugfs directory, and then add/remove subdirs all the time within that.
In the following I will call the first top level directory created by a module / driver "root".
The logic I propose is that "root" is of type Dir, which means there is no keep() and if dropped the whole tree under root is removed.
A subdir created under a Dir is of type SubDir and has the keep() method and if called consumes the type instance and subsequently can only ever be removed by dropping root.
Alternatively a SubDir can be converted into a Dir, and hence don't has keep() anymore and if dropped will be removed.
So, the result is that we still can add / remove subdirs as we want.
The advantage is that we don't have keep() for root, which would be a dedicated API for driver / modules to create bugs. If a driver / module would call keep() on the root, it would not only mean that we leak the root directory, but also all subdirs and files that we called keep() on.
This becomes even more problematic if we start attaching data to files. Think of an Arc that is attached to a file, which keeps driver data alive just because we leaked the root.
Quoted 11 lines
Quoted 3 lines
However, we don't want users to be able to call
keep()on the directory that has been created first, since if that's done we loose our root anchor to ever free the tree, which almost always would be a bug.Then do a call to
debugfs_lookup_and_remove()which is what I really recommend doing for any C user anyway. That way no dentry is ever "stored" anywhere.Anyway, if Dir always has an implicit
keep()call in it, then I guess this is ok. Let's see how this shakes out with some real-world users. We can always change it over time if it gets unwieldy.
I really advise against it, Rust allows us to model such subtile differences properly (and easily) with the type system to avoid bugs. Let's take advantage of that.
Using debugfs_lookup_and_remove() wouldn't change anything, since we want to attach the lifetime of a directory to a corresponding object.
(Otherwise we're back to where we are with C, i.e. the user has to remember to call the correct thing at the correct time, rather than letting the type system take care instead.)
So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer vs. CString.
Danilo Krummrich
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 40 lines · Danilo Krummrich
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · Danilo Krummrich
Quoted 13 lines · Greg Kroah-Hartman
Quoted 8 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 136
Quoted 3 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +3
-0+/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.Wait, let's step back. Why do we care about the difference between a "subdir" and a "dir"? They both are the same thing, and how do you describe a subdir of a subdir? :)
Quoted 7 lines
Quoted 3 lines
We care about the difference, because Dir originally had
keep()which drops the Dir instance without actually removing it. For subdirs this is fine, since they'll be cleaned up when the parent is removed.But does that mean a subdir can not be cleaned up without dropping the parent first? For many subsystems, they make a "root" debugfs directory, and then add/remove subdirs all the time within that.
In the following I will call the first top level directory created by a module / driver "root".
The logic I propose is that "root" is of type Dir, which means there is no
keep()and if dropped the whole tree under root is removed.A subdir created under a Dir is of type SubDir and has the
keep()method and if called consumes the type instance and subsequently can only ever be removed by dropping root.Alternatively a SubDir can be converted into a Dir, and hence don't has
keep()anymore and if dropped will be removed.So, the result is that we still can add / remove subdirs as we want.
Quoted 8 lines
The advantage is that we don't have
keep()for root, which would be a dedicated API for driver / modules to create bugs. If a driver / module would callkeep()on the root, it would not only mean that we leak the root directory, but also all subdirs and files that we calledkeep()on.This becomes even more problematic if we start attaching data to files. Think of an Arc that is attached to a file, which keeps driver data alive just because we leaked the root.
Forgot to mention, this Arc could contain vtables into the (driver) module after the module has been removed already, which could be called into if reading from / writing to a corresponding (leaked) debugfs file.
I really think Dir::keep() is an invitation for potentially horrible bugs.
If we really don't want SubDir, then let's not have keep() at all.
Quoted 26 lines
Quoted 11 lines
Quoted 3 lines
However, we don't want users to be able to call
keep()on the directory that has been created first, since if that's done we loose our root anchor to ever free the tree, which almost always would be a bug.Then do a call to
debugfs_lookup_and_remove()which is what I really recommend doing for any C user anyway. That way no dentry is ever "stored" anywhere.Anyway, if Dir always has an implicit
keep()call in it, then I guess this is ok. Let's see how this shakes out with some real-world users. We can always change it over time if it gets unwieldy.I really advise against it, Rust allows us to model such subtile differences properly (and easily) with the type system to avoid bugs. Let's take advantage of that.
Using
debugfs_lookup_and_remove()wouldn't change anything, since we want to attach the lifetime of a directory to a corresponding object.(Otherwise we're back to where we are with C, i.e. the user has to remember to call the correct thing at the correct time, rather than letting the type system take care instead.)
So, instead of
debugfs_remove()we'd calldebugfs_lookup_and_remove()fromDir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer vs. CString.
Greg Kroah-Hartman
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
In reply to Danilo KrummrichQuoted 40 lines · Danilo Krummrich
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · Danilo Krummrich
Quoted 13 lines · Greg Kroah-Hartman
Quoted 8 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 136
Quoted 3 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +3
-0+/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.Wait, let's step back. Why do we care about the difference between a "subdir" and a "dir"? They both are the same thing, and how do you describe a subdir of a subdir? :)
Quoted 7 lines
Quoted 3 lines
We care about the difference, because Dir originally had
keep()which drops the Dir instance without actually removing it. For subdirs this is fine, since they'll be cleaned up when the parent is removed.But does that mean a subdir can not be cleaned up without dropping the parent first? For many subsystems, they make a "root" debugfs directory, and then add/remove subdirs all the time within that.
In the following I will call the first top level directory created by a module / driver "root".
The logic I propose is that "root" is of type Dir, which means there is no
keep()and if dropped the whole tree under root is removed.A subdir created under a Dir is of type SubDir and has the
keep()method and if called consumes the type instance and subsequently can only ever be removed by dropping root.Alternatively a SubDir can be converted into a Dir, and hence don't has
keep()anymore and if dropped will be removed.So, the result is that we still can add / remove subdirs as we want.
Quoted 8 lines
The advantage is that we don't have
keep()for root, which would be a dedicated API for driver / modules to create bugs. If a driver / module would callkeep()on the root, it would not only mean that we leak the root directory, but also all subdirs and files that we calledkeep()on.This becomes even more problematic if we start attaching data to files. Think of an Arc that is attached to a file, which keeps driver data alive just because we leaked the root.
Ok, fair enough, let's try it this way, without keep()
Quoted 19 lines
Quoted 11 lines
Quoted 3 lines
However, we don't want users to be able to call
keep()on the directory that has been created first, since if that's done we loose our root anchor to ever free the tree, which almost always would be a bug.Then do a call to
debugfs_lookup_and_remove()which is what I really recommend doing for any C user anyway. That way no dentry is ever "stored" anywhere.Anyway, if Dir always has an implicit
keep()call in it, then I guess this is ok. Let's see how this shakes out with some real-world users. We can always change it over time if it gets unwieldy.I really advise against it, Rust allows us to model such subtile differences properly (and easily) with the type system to avoid bugs. Let's take advantage of that.
Using
debugfs_lookup_and_remove()wouldn't change anything, since we want to attach the lifetime of a directory to a corresponding object.
Quoted 7 lines
(Otherwise we're back to where we are with C, i.e. the user has to remember to call the correct thing at the correct time, rather than letting the type system take care instead.)
So, instead of
debugfs_remove()we'd calldebugfs_lookup_and_remove()fromDir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer vs. CString.
Ok, that's fine, and it gives me an idea of how I can fix up the C api over time to get rid of exporting the dentry entirely :)
thanks,
greg k-h
Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 76 lines · Greg Kroah-Hartman
Quoted 40 lines · Danilo Krummrich
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · Danilo Krummrich
Quoted 13 lines · Greg Kroah-Hartman
Quoted 8 lines · Danilo Krummrich
Review
Quoted 3 lines · Matthew Maurer
(quoted diff) +3
-0+/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir(ManuallyDrop<Dir>);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.Wait, let's step back. Why do we care about the difference between a "subdir" and a "dir"? They both are the same thing, and how do you describe a subdir of a subdir? :)
Quoted 7 lines
Quoted 3 lines
We care about the difference, because Dir originally had
keep()which drops the Dir instance without actually removing it. For subdirs this is fine, since they'll be cleaned up when the parent is removed.But does that mean a subdir can not be cleaned up without dropping the parent first? For many subsystems, they make a "root" debugfs directory, and then add/remove subdirs all the time within that.
In the following I will call the first top level directory created by a module / driver "root".
The logic I propose is that "root" is of type Dir, which means there is no
keep()and if dropped the whole tree under root is removed.A subdir created under a Dir is of type SubDir and has the
keep()method and if called consumes the type instance and subsequently can only ever be removed by dropping root.Alternatively a SubDir can be converted into a Dir, and hence don't has
keep()anymore and if dropped will be removed.So, the result is that we still can add / remove subdirs as we want.
Quoted 8 lines
The advantage is that we don't have
keep()for root, which would be a dedicated API for driver / modules to create bugs. If a driver / module would callkeep()on the root, it would not only mean that we leak the root directory, but also all subdirs and files that we calledkeep()on.This becomes even more problematic if we start attaching data to files. Think of an Arc that is attached to a file, which keeps driver data alive just because we leaked the root.
Ok, fair enough, let's try it this way, without
keep()Quoted 23 lines
Quoted 11 lines
Quoted 3 lines
However, we don't want users to be able to call
keep()on the directory that has been created first, since if that's done we loose our root anchor to ever free the tree, which almost always would be a bug.Then do a call to
debugfs_lookup_and_remove()which is what I really recommend doing for any C user anyway. That way no dentry is ever "stored" anywhere.Anyway, if Dir always has an implicit
keep()call in it, then I guess this is ok. Let's see how this shakes out with some real-world users. We can always change it over time if it gets unwieldy.I really advise against it, Rust allows us to model such subtile differences properly (and easily) with the type system to avoid bugs. Let's take advantage of that.
Using
debugfs_lookup_and_remove()wouldn't change anything, since we want to attach the lifetime of a directory to a corresponding object.(Otherwise we're back to where we are with C, i.e. the user has to remember to call the correct thing at the correct time, rather than letting the type system take care instead.)
Quoted 6 lines
Quoted 3 lines
So, instead of
debugfs_remove()we'd calldebugfs_lookup_and_remove()fromDir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer vs. CString.Ok, that's fine, and it gives me an idea of how I can fix up the C api over time to get rid of exporting the dentry entirely :)
I've figured out that the SubDir API (and indeed, all patchsets after v1) have a flaw in them, so I'm wondering if your rework will resolve this.
If I do:
```
let dir = Dir::new(c_str!("foo"));
let subdir = dir.subdir(c_str!("bar"));
drop(dir);
// This next line will Oops because subdir is already removed
drop(Dir::from(subdir));
```Simply removing Dir::from from the API won't resolve this - as long as SubDir has any method, accessing it after its parent has been cleaned up with remove will result in an oops.
The options I can see given the existing API are:
A. SubDir needs to be inherently limited to a borrow of another Dir or SubDir. This will still break if someone uses debugfs_lookup_and_remove() in an antisocial fashion, but we haven't bound this in Rust, and we don't need to be robust against bad behavior from C. If we do this, the promotability of subdirectories back to directories would go away entirely,
B. I can leverage dget/dput to make it so that the directory I get back from the APIs are actually owning, and so the dentry will not be released while the Dir/SubDir exists. I understand this to be undesired, but putting it out there, since it'd make things safe even to debugfs_lookup_and_remove().
C. I can limit use of debugfs_remove to the builder API (exists in V1, no longer exists in this version, will be a separate patch soon for reference). This is the behavior that I had written in V1 before I started trying to adapt to requests. This means that the subdirectory handles will remain valid because the non-builder handles will be dput rather than recursively removed. Builder handles can't be smuggled out due to an existential lifetime. Again, I understand this is not desired, I'm just trying to lay out all ways to prevent this class of error.
Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
In reply to Danilo KrummrichQuoted 13 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 21
Quoted 9 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +9
-0+/// Owning handle to a DebugFS directory. +/// +/// This directory will be cleaned up when it goes out of scope. +/// +/// # Invariants +/// +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`. +#[repr(transparent)] +pub struct Dir (#[cfg( CONFIG_DEBUG_FS )] *mut bindings::dentry );Should probably use Opaque instead of a raw pointer.
Opaque usage is problematic here:
1. It was explicitly requested in the first patch that I not expose the reference counted nature of the dentry we were being provided. At the same time, the dentry we are provided isn't a borrow - it doesn't live for a fixed lifetime, it goes away when we clean it up. This means that if I do Dir(Opaque<dentry>), there's no way to represent the return value of directory creation - it's not Box<Dir> or Arc<Dir>, and I've been requested to hide the fact that it's ARef<Dir> and not decrement the refcount myself. We can't know the lifetime at the callsite, so we can't use a &'a Dir either. This lands with the raw pointer wrapper. I could, technically, produce InnerDir(Opaque<dentry>) and Dir(ManuallyDrop<ARef<Dir>>), and then impl Drop for Dir to remove instead of dput, but that seems like a bunch of work to construct infrastructure and then suppress it that doesn't actually help.
2. If you were suggesting Dir(*mut Opaque<dentry>), I think this is meaningless and will just cause cast noise. My understanding is that the use of Opaque is to remove restrictions Rust has over legal references and values, e.g. it's initialized, it's not being mutated, thread safety invariants, etc. However, pointers in Rust explicitly do not induce any of these requirements on their pointee (otherwise ptr::dangling() would be immediate UB in many situations).
3. Unless we produce an ARef<Opaque<dentry>> at some point (which again, we've been asked to pretend isn't there), I don't think there's any way for the DebugFS bindings to produce a legal &'a Opaque<dentry>, so there's not really a purpose.
Quoted 140 lines
Quoted 2 lines
+// SAFETY: Dir is just a
dentryunder the hood, which the API promises can be transferred[
Dir]Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+// between threads. +unsafe impl Send for Dir {} + +// SAFETY: All the native functions we re-export use interior locking, and the contents of the +// struct are opaque to Rust. +unsafe impl Sync for Dir {} + +impl Dir { +/// Create a new directory in DebugFS at the root. +/// rust/kernel/debugfs.rs line 43
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// } +/// // It does not exist here. This ready like an explanation for scopes; I think we should drop those comments and the scope.
Quoted 20 lines
rust/kernel/debugfs.rs new file +20
-0+/// ``` +pub fn new (name: &CStr ) ->Self { +Self ::create (name,None ) + } + +/// Create a DebugFS subdirectory. +/// +/// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope. +/// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`]. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// { +/// let child = parent.subdir(c_str!("child")); rust/kernel/debugfs.rs line 73
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// // The path "parent/child" exists in DebugFS here. +/// } +/// // The path "parent/child" still exists. +/// { +/// let child2 = Dir::from(parent.subdir(c_str!("child2"))); +/// // The path "parent/child2" exists in DebugFS here. +/// } +/// // The path "parent/child2" is gone. +/// } +/// // None of the paths exist here. I think the fact that you need all those comments here proves that it's not really intuitive. Please see me comment on SubDir below.
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// ``` +pub fn subdir (&self, name: &CStr ) ->SubDir { +SubDir ::new (Self ::create (name,Some (self))) + } + +/// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root. +#[cfg( CONFIG_DEBUG_FS )] +fn create (name: &CStr , parent:Option <&Self >) ->Self { +let parent_ptr =match parent { +Some (parent) => parent.as_ptr (),rust/kernel/debugfs.rs line 93
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+ None => core::ptr::null_mut(), + }; +// SAFETY: +// * `name` argument points to a NUL-terminated string that lives across the call, by +// invariants of `&CStr`. +// * If `parent` is `None`, `parent` accepts null pointers to mean create at root. +// * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers. +// * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer, +// so we can call `Self::from_ptr`. +unsafe {Self ::from_ptr (bindings::debugfs_create_dir (name.as_char_ptr (), parent_ptr)) }Please split up in two calls, such that we don't have two unsafe function calls in a single unsafe block.
Quoted 12 lines
rust/kernel/debugfs.rs new file +12
-0+ } + +#[cfg(not( CONFIG_DEBUG_FS ))] +fn create (_name: &CStr , _parent:Option <&Self >) ->Self { +Self () + } + +/// Constructs a new DebugFS [`Dir`] from the underlying pointer. +/// +/// # Safety +/// +/// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a rust/kernel/debugfs.rs line 115
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// live DebugFS directory. +#[cfg( CONFIG_DEBUG_FS )] +unsafe fn from_ptr (ptr: *mut bindings::dentry ) ->Self { +Self (ptr) + } + +/// Returns the pointer representation of the DebugFS directory. +/// +/// Due to the type invariant, the value returned from this function will always be an error +/// code, NUL, or a live DebugFS directory. Maybe put this in a '# Guarantees' section.
Quoted 1 line
+ // If this function is ever needed with
not(CONFIG_DEBUG_FS), hardcode it to returnENODEV.I think you mean
ERR_PTR(ENODEV).Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+#[cfg( CONFIG_DEBUG_FS )] +fn as_ptr (&self) -> *mut bindings::dentry { + self.0 + } +} + +impl Drop for Dir { +fn drop (&mut self) { +// SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries. +// `as_ptr` guarantees that the pointer is of this form. rust/kernel/debugfs.rs line 136
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+#[cfg( CONFIG_DEBUG_FS )] +unsafe { + bindings::debugfs_remove (self.as_ptr ()) + } + } +} + +/// Handle to a DebugFS directory that will stay alive after leaving scope. +#[repr(transparent)] +pub struct SubDir (ManuallyDrop <Dir >);I think it's not very intuitive if the default is that a SubDir still exists after it has been dropped. I think your first approach being explicit about this with
keep()consuming the SubDir was much better; please keep this approach.
Danilo Krummrich
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 15 lines · Matthew Maurer
Quoted 13 lines · Danilo Krummrich
rust/kernel/debugfs.rs line 21
Quoted 9 lines · Matthew Maurer
rust/kernel/debugfs.rs new file +9
-0+/// Owning handle to a DebugFS directory. +/// +/// This directory will be cleaned up when it goes out of scope. +/// +/// # Invariants +/// +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`. +#[repr(transparent)] +pub struct Dir (#[cfg( CONFIG_DEBUG_FS )] *mut bindings::dentry );Should probably use Opaque instead of a raw pointer.
Quoted 1 line
Opaque usage is problematic here:
Yes, when I wrote this I was back at your v1 in my mind -- the raw pointer is fine..
Benno Lossin
reviewerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
In reply to Matthew MaurerQuoted 11 lines
rust/kernel/debugfs.rs new file +5
-0@@ -0,0 +1,155 @@ +// SPDX-License-Identifier: GPL-2.0 + +// Copyright (C) 2025 Google LLC. + +//! DebugFS Abstraction
rust/kernel/debugfs.rs line 15
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+//! +//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h) + +use crate ::str::CStr ; +use core::mem::ManuallyDrop ; +use core::ops::Deref ; + +/// Owning handle to a DebugFS directory. +/// +/// This directory will be cleaned up when it goes out of scope.
We should also document that it's a unit struct when CONFIG_DEBUG_FS is disabled (and the operations are noops). Maybe even do something like:
#[cfg_attr(CONFIG_DEBUG_FS, doc = "`CONFIG_DEBUG_FS=y`")]
#[cfg_attr(not(CONFIG_DEBUG_FS), doc = "`CONFIG_DEBUG_FS=n`")]Quoted 27 lines
rust/kernel/debugfs.rs new file +27
-0+/// +/// # Invariants +/// +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`. +#[repr(transparent)] +pub struct Dir (#[cfg( CONFIG_DEBUG_FS )] *mut bindings::dentry ); + +// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred +// between threads. +unsafe impl Send for Dir {} + +// SAFETY: All the native functions we re-export use interior locking, and the contents of the +// struct are opaque to Rust. +unsafe impl Sync for Dir {} + +impl Dir { +/// Create a new directory in DebugFS at the root. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// }
rust/kernel/debugfs.rs line 52
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+/// // It does not exist here. +/// ``` +pub fn new (name: &CStr ) ->Self { +Self ::create (name,None ) + } + +/// Create a DebugFS subdirectory. +/// +/// This returns a [`SubDir`], which will not be automatically cleaned up when it leaves scope. +/// To convert this to a handle governing the lifetime of the directory, use [`Dir::from`].
But it will be cleaned up when the parent goes out of scope? We should also mention that.
Quoted 31 lines
rust/kernel/debugfs.rs new file +31
-0+/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// { +/// let parent = Dir::new(c_str!("parent")); +/// // The path "parent" exists in DebugFS here. +/// { +/// let child = parent.subdir(c_str!("child")); +/// // The path "parent/child" exists in DebugFS here. +/// } +/// // The path "parent/child" still exists. +/// { +/// let child2 = Dir::from(parent.subdir(c_str!("child2"))); +/// // The path "parent/child2" exists in DebugFS here. +/// } +/// // The path "parent/child2" is gone. +/// } +/// // None of the paths exist here. +/// ``` +pub fn subdir (&self, name: &CStr ) ->SubDir { +SubDir ::new (Self ::create (name,Some (self))) + } + +/// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root. +#[cfg( CONFIG_DEBUG_FS )] +fn create (name: &CStr , parent:Option <&Self >) ->Self { +let parent_ptr =match parent { +Some (parent) => parent.as_ptr (),
rust/kernel/debugfs.rs line 93
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+ None => core::ptr::null_mut(), + }; +// SAFETY: +// * `name` argument points to a NUL-terminated string that lives across the call, by +// invariants of `&CStr`. +// * If `parent` is `None`, `parent` accepts null pointers to mean create at root. +// * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers. +// * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer, +// so we can call `Self::from_ptr`. +unsafe {Self ::from_ptr (bindings::debugfs_create_dir (name.as_char_ptr (), parent_ptr)) }
What about when an error got returned? Should that be exposed to the user?
Quoted 4 lines
rust/kernel/debugfs.rs new file +4
-0+ } + +#[cfg(not( CONFIG_DEBUG_FS ))] +fn create (_name: &CStr , _parent:Option <&Self >) ->Self {
rust/kernel/debugfs.rs line 128
Quoted 10 lines
rust/kernel/debugfs.rs new file +9
-0+Self () + } + +impl Drop for Dir { +fn drop (&mut self) { +// SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries. +// `as_ptr` guarantees that the pointer is of this form. +#[cfg( CONFIG_DEBUG_FS )] +unsafe {
I feel a bit uneasy with seeing cfg on unsafe code, since now the correctness also depends on the configuration. Someone might add/modify it making it incorrect under certain configurations.
This case is pretty straight forward, but I'm not so sure if we already have such a case.
How about having two modules providing the two implementations and then just conditionally import one or the other?
---
Cheers,
Benno
Quoted 4 lines
rust/kernel/debugfs.rs new file +4
-0+ bindings::debugfs_remove (self.as_ptr ()) + } + } +}
Greg Kroah-Hartman
maintainerRe: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation
Quoted 16 lines · Benno Lossin
Quoted 5 lines
rust/kernel/debugfs.rs new file +5
-0+/// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root. +#[cfg( CONFIG_DEBUG_FS )] +fn create (name: &CStr , parent:Option <&Self >) ->Self { +let parent_ptr =match parent { +Some (parent) => parent.as_ptr (),rust/kernel/debugfs.rs line 93
Quoted 10 lines
rust/kernel/debugfs.rs new file +10
-0+ None => core::ptr::null_mut(), + }; +// SAFETY: +// * `name` argument points to a NUL-terminated string that lives across the call, by +// invariants of `&CStr`. +// * If `parent` is `None`, `parent` accepts null pointers to mean create at root. +// * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers. +// * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer, +// so we can call `Self::from_ptr`. +unsafe {Self ::from_ptr (bindings::debugfs_create_dir (name.as_char_ptr (), parent_ptr)) }
Quoted 2 lines
What about when an error got returned? Should that be exposed to the user?
No, not at all. See my comments on version 1 of this patchset. No error should ever go back to the caller, it should never know if a debugfs call succeeded or not so that it can just keep moving forward and not act any differently.
Many of the C debugfs apis are already changed to be this way, let's not go backwards and add this logic to the rust code only to rip it out in the future.
Quoted 15 lines
Quoted 4 lines
rust/kernel/debugfs.rs new file +4
-0+ } + +#[cfg(not( CONFIG_DEBUG_FS ))] +fn create (_name: &CStr , _parent:Option <&Self >) ->Self {rust/kernel/debugfs.rs line 128
Quoted 10 lines
rust/kernel/debugfs.rs new file +9
-0+Self () + } + +impl Drop for Dir { +fn drop (&mut self) { +// SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries. +// `as_ptr` guarantees that the pointer is of this form. +#[cfg( CONFIG_DEBUG_FS )] +unsafe {
Quoted 3 lines
I feel a bit uneasy with seeing
cfgonunsafecode, since now the correctness also depends on the configuration. Someone might add/modify it making it incorrect under certain configurations.
The option is either enabled or not, this should be fine.
Quoted 5 lines
This case is pretty straight forward, but I'm not so sure if we already have such a case.
How about having two modules providing the two implementations and then just conditionally import one or the other?
That would require a lot more duplicated code that you then have to always keep in sync. And from past experience, that's hard to do over time. So let's do it this way if at all possible.
thanks,
greg k-h
PATCH v3 2/4 rust: debugfs: Bind file creation for long-lived Display
In reply to Matthew MaurerAllows creation of files for references that live forever and lack metadata through the Display implementation.
The reference must live forever because we do not have a maximum lifetime for the file we are creating.
The Display implementation is used because seq_printf needs to route through %pA, which in turn routes through Arguments. A more generic API is provided later in the series, implemented in terms of this one.
--- rust/kernel/debugfs.rs | 134 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+)
rust/kernel/debugfs.rs +134 -0
@@ -7,6 +7,7 @@//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h) use crate ::str::CStr ; +use core::fmt::Display ;use core::mem::ManuallyDrop ;use core::ops::Deref ;
@@ -118,6 +119,48 @@ unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {fn as_ptr (&self) -> *mut bindings::dentry { self.0 } + +/// Create a file in a DebugFS directory with the provided name, and contents from invoking +/// [`Display::fmt`] on the provided reference. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// let dir = Dir::new(c_str!("my_debugfs_dir")); +/// dir.display_file(c_str!("foo"), &200); +/// // "my_debugfs_dir/foo" now contains the number 200. +/// ``` +pub fn display_file <T :Display +Sized >(&self, name: &CStr , data: &'staticT ) ->File { +// SAFETY: +// * `name` is a NUL-terminated C string, living across the call, by `CStr` invariant. +// * `parent` is a live `dentry` since we have a reference to it. +// * `vtable` is all stock `seq_file` implementations except for `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 we know because +// we have a static reference. +// * debugfs_create_file_full either returns an error code or a legal dentry pointer, so +// `Self::from_ptr` is safe to call here. +#[cfg( CONFIG_DEBUG_FS )] +let dir =unsafe { +Self ::from_ptr (bindings::debugfs_create_file_full ( + name.as_char_ptr (), +0o444 , + self.as_ptr (), + dataas *const _ as *mut _ , + core::ptr::null (), + &<T as DisplayFile >::VTABLE , + )) + }; +#[cfg(not( CONFIG_DEBUG_FS ))] +let dir = { +// Mark parameters used +let (_, _) = (name, data); +Self () + }; +File (ManuallyDrop ::new (dir)) + } }impl Drop for Dir {
@@ -153,3 +196,94 @@ fn new(dir: Dir) -> Self {Self (ManuallyDrop ::new (dir)) } } +/// Handle to a DebugFS file. +#[repr(transparent)] +pub struct File (ManuallyDrop <Dir >); + +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 )) + } +} + +#[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) } + } + +/// 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).privateas *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 _ ), +// SAFETY: `file_operations` supports zeroes in all fields. + ..unsafe { core::mem::zeroed () } + }; + } + +impl <T :Display +Sized >DisplayFile for T {} +} + +#[cfg( CONFIG_DEBUG_FS )] +use helpers::*;
Signature · 1 line
2.49.0.906.g1f30a19c02-goog
Danilo Krummrich
maintainerRe: [PATCH v3 2/4] rust: debugfs: Bind file creation for long-lived Display
rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 201
Quoted 3 lines · Matthew Maurer
rust/kernel/debugfs.rs +3
-0+/// 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;
Quoted 13 lines
rust/kernel/debugfs.rs +13
-0+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.
rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 225
Quoted 10 lines
rust/kernel/debugfs.rs +10
-0+/// { +/// 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..
Quoted 15 lines
rust/kernel/debugfs.rs +15
-0+ +#[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>(
rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 250
Quoted 10 lines
rust/kernel/debugfs.rs +10
-0+ 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.
Quoted 5 lines
rust/kernel/debugfs.rs +5
-0+ } + +/// Prints private data stashed in a seq_file to that seq file. +/// +/// # Safety
rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 265
Quoted 10 lines
rust/kernel/debugfs.rs +10
-0+/// +/// `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).privateas *mut T ) };
This creates an intermediate reference to private, which is UB. Please use addr_of! instead.
Re: [PATCH v3 2/4] rust: debugfs: Bind file creation for long-lived Display
Quoted 101 lines · Danilo Krummrich
rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 201
Quoted 3 lines · Matthew Maurer
rust/kernel/debugfs.rs +3
-0+/// 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;Quoted 13 lines
rust/kernel/debugfs.rs +13
-0+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. rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 225
Quoted 10 lines
rust/kernel/debugfs.rs +10
-0+/// { +/// 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 normaldrop()logic. Removing the file when it is dropped and keeping it when callingkeep()is much more intuitive..Quoted 15 lines
rust/kernel/debugfs.rs +15
-0+ +#[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>(rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 250
Quoted 10 lines
rust/kernel/debugfs.rs +10
-0+ 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.
Quoted 5 lines
rust/kernel/debugfs.rs +5
-0+ } + +/// Prints private data stashed in a seq_file to that seq file. +/// +/// # Safety rust/kernel/debugfs.rs fn new(dir: Dir) -> Self { · line 265
Quoted 10 lines
rust/kernel/debugfs.rs +10
-0+/// +/// `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).privateas *mut T ) };
Quoted 2 lines
This creates an intermediate reference to private, which is UB. Please use
addr_of! instead.
I'm making this change, but so that I can be correct in the future, can you explain why taking an intermediate reference to private is UB? My understanding is that my provided vtable are supposed to be the only methods looking at this field, and they don't mutate it. Additionally, the private_data field on file is accessed this way in miscdevice at the moment - what makes it safe there, and UB here?
Danilo Krummrich
maintainerRe: [PATCH v3 2/4] rust: debugfs: Bind file creation for long-lived Display
Quoted 4 lines · Matthew Maurer
I'm making this change, but so that I can be correct in the future, can you explain why taking an intermediate reference to private is UB? My understanding is that my provided vtable are supposed to be the only methods looking at this field, and they don't mutate it.
You're right, I confused it with something else.
PATCH v3 3/4 rust: debugfs: Support format hooks
In reply to Matthew MaurerRather than always using Display, allow hooking arbitrary functions to arbitrary files. Display technically has the expressiveness to do this, but requires a new type be declared for every different way to render things, which can be very clumsy.
--- rust/kernel/debugfs.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-)
rust/kernel/debugfs.rs +117 -1
@@ -7,6 +7,7 @@//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h) use crate ::str::CStr ; +use core::fmt;use core::fmt::Display ;use core::mem::ManuallyDrop ;use core::ops::Deref ;
@@ -133,6 +134,48 @@ fn as_ptr(&self) -> *mut bindings::dentry {/// // "my_debugfs_dir/foo" now contains the number 200. /// ``` pub fn display_file <T :Display +Sized >(&self, name: &CStr , data: &'staticT ) ->File { +// SAFETY: As `data` lives for the static lifetime, it outlives the file. +unsafe { self.display_file_raw (name, data) } + } + +/// Create a file in a DebugFS directory with the provided name, and contents from invoking `f` +/// on the provided reference. +/// +/// `f` must be a function item or a non-capturing closure, or this will fail to compile. +/// +/// # Examples +/// +/// ``` +/// # use core::sync::atomic::{AtomicU32, Ordering}; +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// let dir = Dir::new(c_str!("foo")); +/// static MY_ATOMIC: AtomicU32 = AtomicU32::new(3); +/// let file = dir.fmt_file(c_str!("bar"), &MY_ATOMIC, &|val, f| { +/// let out = val.load(Ordering::Relaxed); +/// writeln!(f, "{out:#010x}") +/// }); +/// MY_ATOMIC.store(10, Ordering::Relaxed); +/// ``` +pub fn fmt_file <T ,F :Fn (&T , &mut fmt::Formatter <'_>) -> fmt::Result >( + &self, + name: &CStr , + data: &'staticT , + f: &'staticF , + ) ->File { +// SAFETY: As `data` lives for the static lifetime, it outlives the file +unsafe { self.fmt_file_raw (name, data, f) } + } + +/// Creates a DebugFS file backed by the display implementation of the provided pointer. +/// +/// # Safety +/// +/// The pointee of `data` must outlive the accessibility of the `Dir` returned by this function. +/// This means that before `data` may become invalid, either: +/// * The refcount must go to zero. +/// * The file must be rendered inaccessible, e.g. via `debugfs_remove`. +unsafe fn display_file_raw <T :Display +Sized >(&self, name: &CStr , data: *const T ) ->File {// SAFETY: // * `name` is a NUL-terminated C string, living across the call, by `CStr` invariant. // * `parent` is a live `dentry` since we have a reference to it.
@@ -161,6 +204,32 @@ pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> };File (ManuallyDrop ::new (dir)) } + +/// Create a file in a DebugFS directory with the provided name, and contents from invoking the +/// fomatter on the attached data. +/// +/// The attached function must be a ZST, and will cause a compilation error if it is not. +/// +/// # Safety +/// +/// `data` must outlive the resulting file's accessibility +unsafe fn fmt_file_raw <T ,F :Fn (&T , &mut fmt::Formatter <'_>) -> fmt::Result >( + &self, + name: &CStr , + data: &T , + f: &'staticF , + ) ->File { +#[cfg( CONFIG_DEBUG_FS )] +let data_adapted =FormatAdapter ::new (data, f); +#[cfg(not( CONFIG_DEBUG_FS ))] +let data_adapted = { +// Mark used +let (_, _) = (data, f); + &0 + }; +// SAFETY: data outlives the file's accessibility, so data_adapted does too +unsafe { self.display_file_raw (name, data_adapted) } + } }impl Drop for Dir {
@@ -228,7 +297,9 @@ pub fn remove(self) {mod helpers {use crate ::seq_file::SeqFile ;use crate ::seq_print; -use core::fmt::Display ; +use core::fmt; +use core::fmt::{Display ,Formatter }; +use core::marker::PhantomData ;/// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`. ///
@@ -283,6 +354,51 @@ pub(crate) trait DisplayFile: Display + Sized { }impl <T :Display +Sized >DisplayFile for T {} + +/// Adapter to implement `Display` via a callback with the same representation as `T`. +/// +/// # Invariants +/// +/// If an instance for `FormatAdapter<_, F>` is constructed, `F` is inhabited. +#[repr(transparent)] +pub (crate )struct FormatAdapter <T ,F > { + inner:T , + _formatter:PhantomData <F >, + } + +impl <T ,F >FormatAdapter <T ,F > { +pub (crate )fn new <'a>(inner: &'aT , _f: &'staticF ) -> &'aSelf { +// SAFETY: FormatAdapater is a repr(transparent) wrapper around T, so +// casting a reference is legal +// INVARIANT: We were passed a reference to F, so it is inhabited. +unsafe { core::mem::transmute (inner) } + } + } + +impl <T ,F >Display for FormatAdapter <T ,F > +where +F :Fn (&T , &mut Formatter <'_>) -> fmt::Result + 'static, + { +fn fmt (&self, fmt: &mut Formatter <'_>) -> fmt::Result { +// SAFETY: FormatAdapter<_, F> can only be constructed if F is inhabited +let f: &F =unsafe {materialize_zst_fmt () }; +f (&self.inner, fmt) + } + } + +/// For types with a unique value, produce a static reference to it. +/// +/// # Safety +/// +/// The caller asserts that F is inhabited +unsafe fn materialize_zst_fmt <F >() -> &'staticF { +const {assert! (core::mem::size_of ::<F >() ==0 ) }; +let zst_dangle: core::ptr::NonNull <F > = core::ptr::NonNull ::dangling (); +// SAFETY: While the pointer is dangling, it is a dangling pointer to a ZST, based on the +// assertion above. The type is also inhabited, by the caller's assertion. This means +// we can materialize it. +unsafe { zst_dangle.as_ref () } + } }#[cfg( CONFIG_DEBUG_FS )]
Signature · 1 line
2.49.0.906.g1f30a19c02-goog
PATCH v3 4/4 rust: samples: Add debugfs sample
In reply to Matthew MaurerProvides an example of using the Rust DebugFS bindings.
--- MAINTAINERS | 1 + samples/rust/Kconfig | 11 +++++++++ samples/rust/Makefile | 1 + samples/rust/rust_debugfs.rs | 54 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+)
MAINTAINERS +1 -0
@@ -7278,6 +7278,7 @@ F: rust/kernel/devres.rs F: rust/kernel/driver.rs F: rust/kernel/faux.rs F: rust/kernel/platform.rs +F: samples/rust/rust_debugfs.rs F: samples/rust/rust_driver_platform.rs F: samples/rust/rust_driver_faux.rs
samples/rust/Kconfig +11 -0
@@ -51,6 +51,17 @@ config SAMPLE_RUST_DMA If unsure, say N. +config SAMPLE_RUST_DEBUGFS + tristate "DebugFS Test Driver" + depends on DEBUG_FS + help + This option builds the Rust DebugFS Test driver sample. + + To compile this as a module, choose M here: + the module will be called rust_debugfs. + + If unsure, say N. + config SAMPLE_RUST_DRIVER_PCI tristate "PCI Driver" depends on PCI
samples/rust/Makefile +1 -0
@@ -4,6 +4,7 @@ ccflags-y += -I$(src) # needed for trace events obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE) += rust_misc_device.o obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o +obj-$(CONFIG_SAMPLE_RUST_DEBUGFS) += rust_debugfs.o obj-$(CONFIG_SAMPLE_RUST_DMA) += rust_dma.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o
samples/rust/rust_debugfs.rs new file +54 -0
54 lines not on this page · raw message
Signature · 1 line
2.49.0.906.g1f30a19c02-goog
Danilo Krummrich
maintainerRe: [PATCH v3 4/4] rust: samples: Add debugfs sample
samples/rust/rust_debugfs.rs line 22
Quoted 3 lines · Matthew Maurer
samples/rust/rust_debugfs.rs new file +3
-0+struct RustDebugFs { +// As we only hold this for drop effect (to remove the directory) we have a leading underscore +// to indicate to the compiler that we don't expect to use this field directly.
This feels like an introduction to Rust in the kernel for Rust beginners, which is a great thing!
However, I wonder if, instead, we may want a dedicated (or even multiple) sample modules or sample drivers, where we go into such detail and leave those samples to focus only on the corresponding API?
Quoted 19 lines
samples/rust/rust_debugfs.rs new file +19
-0+ _debugfs: Dir, +} + +static EXAMPLE :AtomicU32 =AtomicU32 ::new (8 ); + +impl kernel::Module for RustDebugFs { +fn init (_this: &'staticThisModule ) ->Result <Self > { +// Create a debugfs directory in the root of the filesystem called "sample_debugfs". +let debugfs =Dir ::new (c_str! ("sample_debugfs" )); + +// Create a subdirectory, so "sample_debugfs/subdir" now exists. +let sub = debugfs.subdir (c_str! ("subdir" )); + +// Create a single file in the subdirectory called "example" that will read from the +// `EXAMPLE` atomic variable. + sub.fmt_file (c_str! ("example" ), &EXAMPLE , &|example, f| { +writeln! (f,"Reading atomic: {}" , example.load (Ordering ::Relaxed )) + }); +// Now, "sample_debugfs/subdir/example" will print "Reading atomic: 8\n" when read.
samples/rust/rust_debugfs.rs line 51
Quoted 10 lines
samples/rust/rust_debugfs.rs new file +10
-0+ +// Change the value in the variable displayed by the file. This is intended to demonstrate +// that the module can continue to change the value used by the file. +EXAMPLE .store (10 ,Ordering ::Relaxed ); +// Now, "sample_debugfs/subdir/example" will print "Reading atomic: 10\n" when read. + +// Save the root debugfs directory we created to our module object. It will be +// automatically cleaned up when our module is unloaded because dropping the module object +// will drop the `Dir` handle. The base directory, the subdirectory, and the file will all +// continue to exist until the module is unloaded.
Same with this comment.
@Greg: I know you proposed this one; for educational purposes I suppose. What's your take on the above?
Greg Kroah-Hartman
maintainerRe: [PATCH v3 4/4] rust: samples: Add debugfs sample
Quoted 11 lines · Danilo Krummrich
samples/rust/rust_debugfs.rs line 22
Quoted 3 lines · Matthew Maurer
samples/rust/rust_debugfs.rs new file +3
-0+struct RustDebugFs { +// As we only hold this for drop effect (to remove the directory) we have a leading underscore +// to indicate to the compiler that we don't expect to use this field directly. This feels like an introduction to Rust in the kernel for Rust beginners, which is a great thing!
However, I wonder if, instead, we may want a dedicated (or even multiple) sample modules or sample drivers, where we go into such detail and leave those samples to focus only on the corresponding API?
People search for the specific example of what they want to do, so I recommend just leaving all of these comments in for now as knowing to search for a different example is usually not going to happen :)
Quoted 33 lines
Quoted 20 lines
samples/rust/rust_debugfs.rs new file +19
-0+ _debugfs: Dir, +} + +static EXAMPLE :AtomicU32 =AtomicU32 ::new (8 ); + +impl kernel::Module for RustDebugFs { +fn init (_this: &'staticThisModule ) ->Result <Self > { +// Create a debugfs directory in the root of the filesystem called "sample_debugfs". +let debugfs =Dir ::new (c_str! ("sample_debugfs" )); + +// Create a subdirectory, so "sample_debugfs/subdir" now exists. +let sub = debugfs.subdir (c_str! ("subdir" )); + +// Create a single file in the subdirectory called "example" that will read from the +// `EXAMPLE` atomic variable. + sub.fmt_file (c_str! ("example" ), &EXAMPLE , &|example, f| { +writeln! (f,"Reading atomic: {}" , example.load (Ordering ::Relaxed )) + }); +// Now, "sample_debugfs/subdir/example" will print "Reading atomic: 8\n" when read. samples/rust/rust_debugfs.rs line 51
Quoted 10 lines
samples/rust/rust_debugfs.rs new file +10
-0+ +// Change the value in the variable displayed by the file. This is intended to demonstrate +// that the module can continue to change the value used by the file. +EXAMPLE .store (10 ,Ordering ::Relaxed ); +// Now, "sample_debugfs/subdir/example" will print "Reading atomic: 10\n" when read. + +// Save the root debugfs directory we created to our module object. It will be +// automatically cleaned up when our module is unloaded because dropping the module object +// will drop the `Dir` handle. The base directory, the subdirectory, and the file will all +// continue to exist until the module is unloaded. Same with this comment.
Quoted 2 lines
@Greg: I know you proposed this one; for educational purposes I suppose. What's your take on the above?
I like it and want to see this here as it helps explain both how to use this, AND how to have maintainers review code that uses the api that are not that familiar with how Rust does things.
thanks,
greg k-h
Danilo Krummrich
maintainerRe: [PATCH v3 4/4] rust: samples: Add debugfs sample
Quoted 2 lines · Greg Kroah-Hartman
AND how to have maintainers review code that uses the api that are not that familiar with how Rust does things.
That sounds very reasonable -- I like it!