The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
is delayed until the more full-featured API, because we need to avoid
the user having an reference to a dir that is recursively removed.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
MAINTAINERS | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/helpers/dcache.c | 12 +++++
rust/helpers/helpers.c | 1 +
rust/kernel/debugfs.rs | 100 ++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
6 files changed, 116 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 906881b6c5cb6ff743e13b251873b89138c69a1c..a3b835e427b083a4ddd690d9e7739851f0af47ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8a2add69e5d66d1c2ebed9d2c950380e61c48842..787f928467faabd02a7f3cf041378fac856c4f89 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -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>
diff --git a/rust/helpers/dcache.c b/rust/helpers/dcache.c
new file mode 100644
index 0000000000000000000000000000000000000000..2396cdaa89a95a2be69fd84ec205e0f5f1b63f0c
--- /dev/null
+++ b/rust/helpers/dcache.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2025 Google LLC.
+ */
+
+#include <linux/dcache.h>
+
+struct dentry *rust_helper_dget(struct dentry *d)
+{
+ return dget(d);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index f34320e6d1f2fb56cc151ee2ffe5d331713fd36a..95f486c1175191483297b7140b99f1aa364c081c 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -15,6 +15,7 @@
#include "cpumask.c"
#include "cred.c"
#include "device.c"
+#include "dcache.c"
#include "dma.c"
#include "err.c"
#include "fs.c"
diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4d06cce7099607f95b684bad329f791a815d3e86
--- /dev/null
+++ b/rust/kernel/debugfs.rs
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! DebugFS Abstraction
+//!
+//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
+
+use crate::error::{from_err_ptr, Result};
+use crate::str::CStr;
+use crate::types::{ARef, AlwaysRefCounted, Opaque};
+use core::ptr::NonNull;
+
+/// Handle to a DebugFS directory.
+pub struct Dir {
+ inner: Opaque<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 {}
+
+// SAFETY: Dir is actually `dentry`, and dget/dput are the reference counting functions
+// for it.
+unsafe impl AlwaysRefCounted for Dir {
+ #[inline]
+ fn inc_ref(&self) {
+ // SAFETY: Since we have a reference to the directory,
+ // it's live, so it's safe to call dget on it.
+ unsafe {
+ kernel::bindings::dget(self.as_ptr());
+ }
+ }
+ #[inline]
+ unsafe fn dec_ref(obj: NonNull<Self>) {
+ // SAFETY: By the caller precondition on the trait, we know that the caller has a reference
+ // count to the object.
+ unsafe {
+ kernel::bindings::dput(obj.cast().as_ptr());
+ }
+ }
+}
+
+impl Dir {
+ /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// # use kernel::c_str;
+ /// # use kernel::debugfs::Dir;
+ /// {
+ /// let dir = Dir::new(c_str!("my_debug_dir"), None)?;
+ /// // The directory will exist in DebugFS here.
+ /// }
+ /// // The directory will no longer exist in DebugFS here.
+ /// # Ok::<(), Error>(())
+ /// ```
+ ///
+ /// ```
+ /// # use kernel::c_str;
+ /// # use kernel::debugfs::Dir;
+ /// let parent = Dir::new(c_str!("parent"), None)?;
+ /// let child = Dir::new(c_str!("child"), Some(&parent))?;
+ /// // parent/child exists in DebugFS here.
+ /// drop(parent);
+ /// // The child dentry is still valid here, but DebugFS will have neither directory.
+ /// # Ok::<(), Error>(())
+ /// ```
+ pub fn new(name: &CStr, parent: Option<&Self>) -> Result<ARef<Self>> {
+ let parent_ptr = match parent {
+ Some(parent) => parent.as_ptr(),
+ None => core::ptr::null_mut(),
+ };
+ // SAFETY:
+ // * name argument points to a null 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 `NonNull::new_unchecked`.
+ let dir = unsafe {
+ NonNull::new_unchecked(from_err_ptr(kernel::bindings::debugfs_create_dir(
+ name.as_char_ptr(),
+ parent_ptr,
+ ))?)
+ };
+ // SAFETY: Dir is a transparent wrapper for an Opaque<dentry>, and we received a live
+ // owning dentry from `debugfs_create_dir`, so we can wrap it in an ARef
+ Ok(unsafe { ARef::from_raw(dir.cast()) })
+ }
+
+ fn as_ptr(&self) -> *mut bindings::dentry {
+ self.inner.get()
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c3762e80b314316b4b0cee3bfd9442f8f0510b91..86f6055b828d5f711578293d8916a517f2436977 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -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;
--
2.49.0.901.g37484f566f-googOn Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> is delayed until the more full-featured API, because we need to avoid
> the user having an reference to a dir that is recursively removed.
>
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
First off, many thanks for doing this. I like this in general, but I
have loads of specific questions/comments. Don't take that as a
criticism of this feature, I really want these bindings to be in the
tree and work hopefully better/cleaner than the userspace ones do.
First off, the main "rule" of debugfs is that you should NEVER care
about the return value of any debugfs function. So much so that the C
side hides errors almost entirely where possible. I'd like to see this
carried through to the Rust side as well, but I think you didn't do that
for various "traditional" reasons.
Please try to unwind all of that in your next version, I'll point this
out below in one place:
> ---
> MAINTAINERS | 1 +
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/dcache.c | 12 +++++
> rust/helpers/helpers.c | 1 +
> rust/kernel/debugfs.rs | 100 ++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 6 files changed, 116 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 906881b6c5cb6ff743e13b251873b89138c69a1c..a3b835e427b083a4ddd690d9e7739851f0af47ae 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 8a2add69e5d66d1c2ebed9d2c950380e61c48842..787f928467faabd02a7f3cf041378fac856c4f89 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -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>
> diff --git a/rust/helpers/dcache.c b/rust/helpers/dcache.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..2396cdaa89a95a2be69fd84ec205e0f5f1b63f0c
> --- /dev/null
> +++ b/rust/helpers/dcache.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (C) 2025 Google LLC.
> + */
> +
> +#include <linux/dcache.h>
> +
> +struct dentry *rust_helper_dget(struct dentry *d)
> +{
> + return dget(d);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index f34320e6d1f2fb56cc151ee2ffe5d331713fd36a..95f486c1175191483297b7140b99f1aa364c081c 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -15,6 +15,7 @@
> #include "cpumask.c"
> #include "cred.c"
> #include "device.c"
> +#include "dcache.c"
> #include "dma.c"
> #include "err.c"
> #include "fs.c"
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4d06cce7099607f95b684bad329f791a815d3e86
> --- /dev/null
> +++ b/rust/kernel/debugfs.rs
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Google LLC.
> +
> +//! DebugFS Abstraction
> +//!
> +//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
> +
> +use crate::error::{from_err_ptr, Result};
> +use crate::str::CStr;
> +use crate::types::{ARef, AlwaysRefCounted, Opaque};
> +use core::ptr::NonNull;
> +
> +/// Handle to a DebugFS directory.
> +pub struct Dir {
> + inner: Opaque<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 {}
> +
> +// SAFETY: Dir is actually `dentry`, and dget/dput are the reference counting functions
> +// for it.
> +unsafe impl AlwaysRefCounted for Dir {
> + #[inline]
> + fn inc_ref(&self) {
> + // SAFETY: Since we have a reference to the directory,
> + // it's live, so it's safe to call dget on it.
> + unsafe {
> + kernel::bindings::dget(self.as_ptr());
> + }
> + }
> + #[inline]
> + unsafe fn dec_ref(obj: NonNull<Self>) {
> + // SAFETY: By the caller precondition on the trait, we know that the caller has a reference
> + // count to the object.
> + unsafe {
> + kernel::bindings::dput(obj.cast().as_ptr());
> + }
> + }
> +}
> +
> +impl Dir {
> + /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use kernel::c_str;
> + /// # use kernel::debugfs::Dir;
> + /// {
> + /// let dir = Dir::new(c_str!("my_debug_dir"), None)?;
> + /// // The directory will exist in DebugFS here.
> + /// }
> + /// // The directory will no longer exist in DebugFS here.
> + /// # Ok::<(), Error>(())
> + /// ```
> + ///
> + /// ```
> + /// # use kernel::c_str;
> + /// # use kernel::debugfs::Dir;
> + /// let parent = Dir::new(c_str!("parent"), None)?;
Why are you checking this? You shouldn't, it should never return an
error. What it returns you can ALWAYS pass back into any call that
expects to get a "debugfs::Dir" object. You should never even be able
to know if it succeeded or not as your code should never do anything
different depending on the result.
> + /// let child = Dir::new(c_str!("child"), Some(&parent))?;
> + /// // parent/child exists in DebugFS here.
> + /// drop(parent);
Wait, what? Why would an explicit drop(parent) be required here? That
feels wrong, and way too complex. Why can't you rely on the child
creation to properly manage this if needed (hint, it shouldn't be.)
> + /// // The child dentry is still valid here, but DebugFS will have neither directory.
> + /// # Ok::<(), Error>(())
Again, no error checking please.
> + /// ```
> + pub fn new(name: &CStr, parent: Option<&Self>) -> Result<ARef<Self>> {
> + let parent_ptr = match parent {
> + Some(parent) => parent.as_ptr(),
> + None => core::ptr::null_mut(),
> + };
> + // SAFETY:
> + // * name argument points to a null 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 `NonNull::new_unchecked`.
> + let dir = unsafe {
> + NonNull::new_unchecked(from_err_ptr(kernel::bindings::debugfs_create_dir(
> + name.as_char_ptr(),
> + parent_ptr,
> + ))?)
> + };
> + // SAFETY: Dir is a transparent wrapper for an Opaque<dentry>, and we received a live
> + // owning dentry from `debugfs_create_dir`, so we can wrap it in an ARef
> + Ok(unsafe { ARef::from_raw(dir.cast()) })
Why do you want this wrapped? And I think you "wrapped" it wrong here.
When the object is "gone" it should have called
debugfs_remove_recursive(), not dput(), in order to properly drop
everything. Where is that call happening?
The debugfs core already handles the reference counting of this object,
please don't add some extra reference count calls with dput/get, that's
just going to be a mess.
You should NEVER be encoding the fact that the return value of a
debugfs_*() call is a dentry. Just treat that as an opaque pointer that
just happens to have a common name that might refer to something else.
That pointer can be passed back into other debugfs functions, and THAT
IS IT. No checking for it, no passing it to any vfs functions, or
anything else.
So the dput() stuff here should not be present at all (hint, no C code
that used debugfs ever calls that, so the rust bindings shouldn't
either.)
thanks,
greg k-h
On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote: > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove` > > is delayed until the more full-featured API, because we need to avoid > > the user having an reference to a dir that is recursively removed. > > > > Signed-off-by: Matthew Maurer <mmaurer@google.com> > > First off, many thanks for doing this. I like this in general, but I > have loads of specific questions/comments. Don't take that as a > criticism of this feature, I really want these bindings to be in the > tree and work hopefully better/cleaner than the userspace ones do. > > First off, the main "rule" of debugfs is that you should NEVER care > about the return value of any debugfs function. So much so that the C > side hides errors almost entirely where possible. I'd like to see this > carried through to the Rust side as well, but I think you didn't do that > for various "traditional" reasons. Sure, I mostly had to do error checking because I was using an `ARef<Dir>` to represent a directory, which meant that the underlying directory needed to be a valid pointer. Given that you've said that the returned `dentry *` should never be used as an actual `dentry`, only as an abstract handle to a DebugFS object, that requirement goes away, and I can remove the error handling code and always successfully return a `Dir`, even in cases where an error has occurred. > > Wait, what? Why would an explicit drop(parent) be required here? That > feels wrong, and way too complex. Why can't you rely on the child > creation to properly manage this if needed (hint, it shouldn't be.) The explicit `drop` is not required for normal usage, it was intended to be illustrative - I was trying to explain what the semantics would be if the parent `dentry` was released before the child was. As before, now that the child will not be an `ARef<Dir>`, and so not assumed to be a valid `dentry` pointer, this example won't be relevant anymore. > > Why do you want this wrapped? And I think you "wrapped" it wrong here. > When the object is "gone" it should have called > debugfs_remove_recursive(), not dput(), in order to properly drop > everything. Where is that call happening? > > The debugfs core already handles the reference counting of this object, > please don't add some extra reference count calls with dput/get, that's > just going to be a mess. > > You should NEVER be encoding the fact that the return value of a > debugfs_*() call is a dentry. Just treat that as an opaque pointer that > just happens to have a common name that might refer to something else. > That pointer can be passed back into other debugfs functions, and THAT > IS IT. No checking for it, no passing it to any vfs functions, or > anything else. > > So the dput() stuff here should not be present at all (hint, no C code > that used debugfs ever calls that, so the rust bindings shouldn't > either.) > I incorrectly assumed that if a `dentry *` was being returned, then at some point it would be used as a `dentry *` rather than as a debugfs-only handle. I bound it into an `ARef` to avoid backing myself into a corner later if someone needed to use the `dentry *` for something, but if that's against the API contract of DebugFS, I can avoid it entirely.
On Wed, Apr 30, 2025 at 08:10:44AM -0700, Matthew Maurer wrote: > On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote: > > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove` > > > is delayed until the more full-featured API, because we need to avoid > > > the user having an reference to a dir that is recursively removed. > > > > > > Signed-off-by: Matthew Maurer <mmaurer@google.com> > > > > First off, many thanks for doing this. I like this in general, but I > > have loads of specific questions/comments. Don't take that as a > > criticism of this feature, I really want these bindings to be in the > > tree and work hopefully better/cleaner than the userspace ones do. > > > > First off, the main "rule" of debugfs is that you should NEVER care > > about the return value of any debugfs function. So much so that the C > > side hides errors almost entirely where possible. I'd like to see this > > carried through to the Rust side as well, but I think you didn't do that > > for various "traditional" reasons. > > Sure, I mostly had to do error checking because I was using an > `ARef<Dir>` to represent a directory, which meant that the underlying > directory needed to be a valid pointer. Given that you've said that > the returned `dentry *` should never be used as an actual `dentry`, > only as an abstract handle to a DebugFS object, that requirement goes > away, and I can remove the error handling code and always successfully > return a `Dir`, even in cases where an error has occurred. Great! Except when debugfs is not enabled, then what are you going to return? The same structure, or an error? I'd vote for the same pointer to the structure, just to make it more obvious that this is a totally opaque thing and that no caller should ever know or care if debugfs is working or even present in the system. Note that some drivers will want to save a bit of space if debugfs is not enabled in the build, so be prepared to make the binding work somehow that way too. Can you have an "empty" object that takes no memory? Or is this too overthinking things? > > Wait, what? Why would an explicit drop(parent) be required here? That > > feels wrong, and way too complex. Why can't you rely on the child > > creation to properly manage this if needed (hint, it shouldn't be.) > > The explicit `drop` is not required for normal usage, it was intended > to be illustrative - I was trying to explain what the semantics would > be if the parent `dentry` was released before the child was. As > before, now that the child will not be an `ARef<Dir>`, and so not > assumed to be a valid `dentry` pointer, this example won't be relevant > anymore. Great! thanks, hopefully this should make things simpler. greg k-h
On Wed, Apr 30, 2025 at 8:23 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Apr 30, 2025 at 08:10:44AM -0700, Matthew Maurer wrote: > > On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote: > > > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove` > > > > is delayed until the more full-featured API, because we need to avoid > > > > the user having an reference to a dir that is recursively removed. > > > > > > > > Signed-off-by: Matthew Maurer <mmaurer@google.com> > > > > > > First off, many thanks for doing this. I like this in general, but I > > > have loads of specific questions/comments. Don't take that as a > > > criticism of this feature, I really want these bindings to be in the > > > tree and work hopefully better/cleaner than the userspace ones do. > > > > > > First off, the main "rule" of debugfs is that you should NEVER care > > > about the return value of any debugfs function. So much so that the C > > > side hides errors almost entirely where possible. I'd like to see this > > > carried through to the Rust side as well, but I think you didn't do that > > > for various "traditional" reasons. > > > > Sure, I mostly had to do error checking because I was using an > > `ARef<Dir>` to represent a directory, which meant that the underlying > > directory needed to be a valid pointer. Given that you've said that > > the returned `dentry *` should never be used as an actual `dentry`, > > only as an abstract handle to a DebugFS object, that requirement goes > > away, and I can remove the error handling code and always successfully > > return a `Dir`, even in cases where an error has occurred. > > Great! > > Except when debugfs is not enabled, then what are you going to return? > The same structure, or an error? > > I'd vote for the same pointer to the structure, just to make it more > obvious that this is a totally opaque thing and that no caller should > ever know or care if debugfs is working or even present in the system. > > Note that some drivers will want to save a bit of space if debugfs is > not enabled in the build, so be prepared to make the binding work > somehow that way too. Can you have an "empty" object that takes no > memory? Or is this too overthinking things? Based on what you've expressed, I think what makes sense is: * Initial patch will always return the same `Dir`, just sometimes it will be a wrapper around a pointer that is an error code, and sometimes it will be a useful `dentry` pointer. This will match the current behavior of C code to my understanding. * Follow-up (probably still in this series) will check `CONFIG_DEBUG_FS`, and if it's off, will just make `Dir` into a ZST, and just discard the pointer. This would be an improvement upon the C interface, because drivers would get the shrinkage without needing to add conditionals on `CONFIG_DEBUG_FS` in their own driver. > > > > Wait, what? Why would an explicit drop(parent) be required here? That > > > feels wrong, and way too complex. Why can't you rely on the child > > > creation to properly manage this if needed (hint, it shouldn't be.) > > > > The explicit `drop` is not required for normal usage, it was intended > > to be illustrative - I was trying to explain what the semantics would > > be if the parent `dentry` was released before the child was. As > > before, now that the child will not be an `ARef<Dir>`, and so not > > assumed to be a valid `dentry` pointer, this example won't be relevant > > anymore. > > Great! > > thanks, hopefully this should make things simpler. > > greg k-h
On Wed, Apr 30, 2025 at 08:31:29AM -0700, Matthew Maurer wrote: > On Wed, Apr 30, 2025 at 8:23 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Apr 30, 2025 at 08:10:44AM -0700, Matthew Maurer wrote: > > > On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote: > > > > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove` > > > > > is delayed until the more full-featured API, because we need to avoid > > > > > the user having an reference to a dir that is recursively removed. > > > > > > > > > > Signed-off-by: Matthew Maurer <mmaurer@google.com> > > > > > > > > First off, many thanks for doing this. I like this in general, but I > > > > have loads of specific questions/comments. Don't take that as a > > > > criticism of this feature, I really want these bindings to be in the > > > > tree and work hopefully better/cleaner than the userspace ones do. > > > > > > > > First off, the main "rule" of debugfs is that you should NEVER care > > > > about the return value of any debugfs function. So much so that the C > > > > side hides errors almost entirely where possible. I'd like to see this > > > > carried through to the Rust side as well, but I think you didn't do that > > > > for various "traditional" reasons. > > > > > > Sure, I mostly had to do error checking because I was using an > > > `ARef<Dir>` to represent a directory, which meant that the underlying > > > directory needed to be a valid pointer. Given that you've said that > > > the returned `dentry *` should never be used as an actual `dentry`, > > > only as an abstract handle to a DebugFS object, that requirement goes > > > away, and I can remove the error handling code and always successfully > > > return a `Dir`, even in cases where an error has occurred. > > > > Great! > > > > Except when debugfs is not enabled, then what are you going to return? > > The same structure, or an error? > > > > I'd vote for the same pointer to the structure, just to make it more > > obvious that this is a totally opaque thing and that no caller should > > ever know or care if debugfs is working or even present in the system. > > > > Note that some drivers will want to save a bit of space if debugfs is > > not enabled in the build, so be prepared to make the binding work > > somehow that way too. Can you have an "empty" object that takes no > > memory? Or is this too overthinking things? > > Based on what you've expressed, I think what makes sense is: > > * Initial patch will always return the same `Dir`, just sometimes it > will be a wrapper around a pointer that is an error code, and > sometimes it will be a useful `dentry` pointer. This will match the > current behavior of C code to my understanding. Great. > * Follow-up (probably still in this series) will check > `CONFIG_DEBUG_FS`, and if it's off, will just make `Dir` into a ZST, > and just discard the pointer. This would be an improvement upon the C > interface, because drivers would get the shrinkage without needing to > add conditionals on `CONFIG_DEBUG_FS` in their own driver. You're going to have to check CONFIG_DEBUG_FS anyway for this series, otherwise things aren't going to build properly, so this sounds like a great way to handle that. thanks, greg k-h
© 2016 - 2026 Red Hat, Inc.