Provides an example of using the Rust DebugFS bindings.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
MAINTAINERS | 1 +
samples/rust/Kconfig | 11 ++++++++
samples/rust/Makefile | 1 +
samples/rust/rust_debugfs.rs | 60 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 73 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index a3b835e427b083a4ddd690d9e7739851f0af47ae..426bcdac025134e20911de8e2cf5c9efb0591814 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 43cb72d72631bb2d6e06185e1d88778edff6ee13..6c42ed73f842cda26256039e6917bb443738d3f1 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -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
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 6a466afd2a21eba84a3b7b2be29f25dce44e9053..b1fc4677ed53fcf7d0f5a3dbf322f65851bc1784 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -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
diff --git a/samples/rust/rust_debugfs.rs b/samples/rust/rust_debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..a4b17c8241330b2f6caf8f17a5b2366138de6ced
--- /dev/null
+++ b/samples/rust/rust_debugfs.rs
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! Sample DebugFS exporting module
+
+use core::mem::{forget, ManuallyDrop};
+use core::sync::atomic::{AtomicU32, Ordering};
+use kernel::c_str;
+use kernel::debugfs::Dir;
+use kernel::prelude::*;
+
+module! {
+ type: RustDebugFs,
+ name: "rust_debugfs",
+ authors: ["Matthew Maurer"],
+ description: "Rust DebugFS usage sample",
+ license: "GPL",
+}
+
+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.
+ _debugfs: Dir<'static>,
+}
+
+static EXAMPLE: AtomicU32 = AtomicU32::new(8);
+
+impl kernel::Module for RustDebugFs {
+ fn init(_this: &'static ThisModule) -> 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.
+ // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
+ // at the end of the scope - it will be cleaned up when `debugfs` is.
+ let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
+
+ // Create a single file in the subdirectory called "example" that will read from the
+ // `EXAMPLE` atomic variable.
+ // We `forget` the result to avoid deleting the file at the end of the scope.
+ forget(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.
+ }
+
+ // 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.
+ Ok(Self { _debugfs: debugfs })
+ }
+}
--
2.49.0.967.g6a0df3ecc3-googOn Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> +impl kernel::Module for RustDebugFs {
> + fn init(_this: &'static ThisModule) -> 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.
> + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> + // at the end of the scope - it will be cleaned up when `debugfs` is.
> + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
I dislike the direct usage of `ManuallyDrop`. To me the usage of
`ManuallyDrop` signifies that one has to opt out of `Drop` without the
support of the wrapped type. But in this case, `Dir` is sometimes
intended to not be dropped, so I'd rather have a `.keep()` function I
saw mentioned somewhere.
> +
> + // Create a single file in the subdirectory called "example" that will read from the
> + // `EXAMPLE` atomic variable.
> + // We `forget` the result to avoid deleting the file at the end of the scope.
> + forget(sub.fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
> + writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
> + }));
Similarly here, I'd rather have a `.keep()` function than people start
using `forget` all over the place.
---
Cheers,
Benno
> + // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 8\n" when read.
> + }
> +
> + // 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.
> + Ok(Self { _debugfs: debugfs })
> + }
> +}
On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > +impl kernel::Module for RustDebugFs {
> > + fn init(_this: &'static ThisModule) -> 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.
> > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > + // at the end of the scope - it will be cleaned up when `debugfs` is.
> > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
>
> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> support of the wrapped type. But in this case, `Dir` is sometimes
> intended to not be dropped, so I'd rather have a `.keep()` function I
> saw mentioned somewhere.
I agree, if we really want to "officially" support to forget() (sub-)directories
and files in order to rely on the recursive cleanup of the "root" directory, it
should be covered explicitly by the API. I.e. (sub-)directories and files should
have some kind of keep() and / or forget() method, to make it clear that this is
supported and by design and won't lead to any leaks.
Consequently, this would mean that we'd need something like your proposed const
generic on the Dir type, such that keep() / forget() cannot be called on the
"root" directory.
However, I really think we should keep the code as it is in this version and
just don't provide an example that utilizes ManuallyDrop and forget().
I don't see how the idea of "manually dropping" (sub-)directories and files
provides any real value compared to just storing their instance in a driver
structure as long as they should stay alive, which is much more intuitive
anyways.
It either just adds complexity to the API (due to the need to distingish between
the "root" directory and sub-directories) or makes the API error prone by
providing a *valid looking* option to users to leak the "root" directory.
- Danilo
On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > > +impl kernel::Module for RustDebugFs {
> > > + fn init(_this: &'static ThisModule) -> 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.
> > > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > > + // at the end of the scope - it will be cleaned up when `debugfs` is.
> > > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> >
> > I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > support of the wrapped type. But in this case, `Dir` is sometimes
> > intended to not be dropped, so I'd rather have a `.keep()` function I
> > saw mentioned somewhere.
>
> I agree, if we really want to "officially" support to forget() (sub-)directories
> and files in order to rely on the recursive cleanup of the "root" directory, it
> should be covered explicitly by the API. I.e. (sub-)directories and files should
> have some kind of keep() and / or forget() method, to make it clear that this is
> supported and by design and won't lead to any leaks.
OK, I can do this with `.keep()` just being equivalent to
`ManuallyDrop::new`. Since we now only have "by default, things are
dropped", rather than two states, we shouldn't need a `.destroy()`
method.
>
> Consequently, this would mean that we'd need something like your proposed const
> generic on the Dir type, such that keep() / forget() cannot be called on the
> "root" directory.
Just to ensure I understand correctly, your opinion is that on
balance, additional complexity in the API types are worth making it
less ergonomic to suppress drop on a root directory, keeping in mind
that they still *can* suppress drop on that directory. You don't want
the extra API variants to do anything else special other than change
the ergonomics of `.keep()` (e.g. don't have subdirectories be default
pre-`.keep()`'d).
>
> However, I really think we should keep the code as it is in this version and
> just don't provide an example that utilizes ManuallyDrop and forget().
>
> I don't see how the idea of "manually dropping" (sub-)directories and files
> provides any real value compared to just storing their instance in a driver
> structure as long as they should stay alive, which is much more intuitive
> anyways.
We can't easily do this, because dropping a root directory recursively
drops everything underneath it. This means that if I have
foo/
- bar/
- baz/
Then my directory handle for `bar` have to be guaranteed to outlive my
directory handle for `foo` so that I know it's didn't get deleted
under me. This is why they have a borrow onto their parent directory.
This borrow means that you can't (without `unsafe`, or something like
`yoke`) keep handles to `foo` and `bar` in the same struct.
I could arguably make the subdir handle creator *take ownership* of
the root handle, which would mitigate this issue, but if we want to
allow creation of arbitrarily shaped trees, we'd either end up rapidly
reproducing something like my builder interface from the follow-up
series which I understand folks aren't enthused about, or we'd end up
with Rust having a mirror struct of some kind, e.g.
```
struct Tree {
name: CString,
// Order is load bearing, any children must be dropped before their parent
children: KVec<Tree>
root: Dir,
}
impl Tree {
fn subdir(&mut self, name: &CStr) -> &mut Tree {
if let Some(x) = children.iter_mut().find(|tree| tree.name == name) {
x
} else {
// This is an internal function, similar to Dir::create
// SAFETY: The structure promises all children will be released
before the parent directory is destroyed.
self.children.push(unsafe { Tree::sys_subdir(self.dir, name) })
self.children.last_mut().unwrap()
}
}
```
This structure has the downside that we now need to track the file
structure in Rust too, not just in the main kernel. This gets more
complex when we add Files, but follows the same approximate structure.
If we don't track the filenames, you have no way to reference
subdirectories again later. If we do track them, we're reproducing
dentry structure less efficiently in Rust. Additionally, because this
needs to track filenames, this now means that `CONFIG_DEBUG_FS=n` is
no longer free.
>
> It either just adds complexity to the API (due to the need to distingish between
> the "root" directory and sub-directories) or makes the API error prone by
> providing a *valid looking* option to users to leak the "root" directory.
>
> - Danilo
On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote: > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote: >> However, I really think we should keep the code as it is in this version and >> just don't provide an example that utilizes ManuallyDrop and forget(). >> >> I don't see how the idea of "manually dropping" (sub-)directories and files >> provides any real value compared to just storing their instance in a driver >> structure as long as they should stay alive, which is much more intuitive >> anyways. > > We can't easily do this, because dropping a root directory recursively > drops everything underneath it. This means that if I have > > foo/ > - bar/ > - baz/ > > Then my directory handle for `bar` have to be guaranteed to outlive my > directory handle for `foo` so that I know it's didn't get deleted > under me. This is why they have a borrow onto their parent directory. > This borrow means that you can't (without `unsafe`, or something like > `yoke`) keep handles to `foo` and `bar` in the same struct. Is there no refcount that we can use instead of borrowing? I guess not, since one can call `debugfs_remove`. What about a refcount on the rust side? or is debugfs not used for "debugging" and needs to have the performance of no refcount? --- Cheers, Benno
On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote: > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote: > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote: > >> However, I really think we should keep the code as it is in this version and > >> just don't provide an example that utilizes ManuallyDrop and forget(). > >> > >> I don't see how the idea of "manually dropping" (sub-)directories and files > >> provides any real value compared to just storing their instance in a driver > >> structure as long as they should stay alive, which is much more intuitive > >> anyways. > > > > We can't easily do this, because dropping a root directory recursively > > drops everything underneath it. This means that if I have > > > > foo/ > > - bar/ > > - baz/ > > > > Then my directory handle for `bar` have to be guaranteed to outlive my > > directory handle for `foo` so that I know it's didn't get deleted > > under me. This is why they have a borrow onto their parent directory. > > This borrow means that you can't (without `unsafe`, or something like > > `yoke`) keep handles to `foo` and `bar` in the same struct. > > Is there no refcount that we can use instead of borrowing? I guess not, > since one can call `debugfs_remove`. What about a refcount on the rust > side? or is debugfs not used for "debugging" and needs to have the > performance of no refcount? debugfs should never have any performance issues (i.e. you don't use it for performant things.) So refcount away! That should never be an issue. thanks, greg k-h
On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote: > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote: > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote: > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote: > > >> However, I really think we should keep the code as it is in this version and > > >> just don't provide an example that utilizes ManuallyDrop and forget(). > > >> > > >> I don't see how the idea of "manually dropping" (sub-)directories and files > > >> provides any real value compared to just storing their instance in a driver > > >> structure as long as they should stay alive, which is much more intuitive > > >> anyways. > > > > > > We can't easily do this, because dropping a root directory recursively > > > drops everything underneath it. This means that if I have > > > > > > foo/ > > > - bar/ > > > - baz/ > > > > > > Then my directory handle for `bar` have to be guaranteed to outlive my > > > directory handle for `foo` so that I know it's didn't get deleted > > > under me. This is why they have a borrow onto their parent directory. > > > This borrow means that you can't (without `unsafe`, or something like > > > `yoke`) keep handles to `foo` and `bar` in the same struct. > > > > Is there no refcount that we can use instead of borrowing? I guess not, > > since one can call `debugfs_remove`. What about a refcount on the rust > > side? or is debugfs not used for "debugging" and needs to have the > > performance of no refcount? > > debugfs should never have any performance issues (i.e. you don't use it > for performant things.) > > So refcount away! That should never be an issue. What I imagine would be the ideal API for Rust is the following: * For each file / dir you create, you get a Rust object that owns it. * When you destroy one of these Rust objects, it disappears from the file system. I.e., dropping a directory removes things recursively. * If you remove a directory before the removing objects inside it, then the Rust objects become "ghost" objects that are still usable, but not visible in the file system anywhere. I.e. calling methods on them succeed but are no-ops. * Possibly we have a way to drop a Rust object without removing it from the file system. In this case, it can never be accessed from Rust again, and the only way to remove it is to drop its parent directory. This way, you can drop foo/ before dropping bar/ and baz/ without that having to be unsafe. Whether that's best implemented by calling dget/dput on the dentry or by having Rust keep track of a separate Rust-only refcount, I don't know. But I think this is the API we want. Thoughts? Alice
On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote:
> > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >> However, I really think we should keep the code as it is in this version and
> > > >> just don't provide an example that utilizes ManuallyDrop and forget().
> > > >>
> > > >> I don't see how the idea of "manually dropping" (sub-)directories and files
> > > >> provides any real value compared to just storing their instance in a driver
> > > >> structure as long as they should stay alive, which is much more intuitive
> > > >> anyways.
> > > >
> > > > We can't easily do this, because dropping a root directory recursively
> > > > drops everything underneath it. This means that if I have
> > > >
> > > > foo/
> > > > - bar/
> > > > - baz/
> > > >
> > > > Then my directory handle for `bar` have to be guaranteed to outlive my
> > > > directory handle for `foo` so that I know it's didn't get deleted
> > > > under me. This is why they have a borrow onto their parent directory.
> > > > This borrow means that you can't (without `unsafe`, or something like
> > > > `yoke`) keep handles to `foo` and `bar` in the same struct.
> > >
> > > Is there no refcount that we can use instead of borrowing? I guess not,
> > > since one can call `debugfs_remove`. What about a refcount on the rust
> > > side? or is debugfs not used for "debugging" and needs to have the
> > > performance of no refcount?
> >
> > debugfs should never have any performance issues (i.e. you don't use it
> > for performant things.)
> >
> > So refcount away! That should never be an issue.
>
> What I imagine would be the ideal API for Rust is the following:
>
> * For each file / dir you create, you get a Rust object that owns it.
>
> * When you destroy one of these Rust objects, it disappears from the
> file system. I.e., dropping a directory removes things recursively.
>
> * If you remove a directory before the removing objects inside it, then
> the Rust objects become "ghost" objects that are still usable, but not
> visible in the file system anywhere. I.e. calling methods on them
> succeed but are no-ops.
If we would want to keep an entry alive as long as there are more leaves, we'd
obviously need to reference count things.
But what do we need reference counting for with this logic? Shouldn't this be
also possible without?
> * Possibly we have a way to drop a Rust object without removing it from
> the file system. In this case, it can never be accessed from Rust
> again, and the only way to remove it is to drop its parent directory.
I'm not sure about this one.
IIUC, this basically brings back the "keep() logic", which I still think is a
footgun and should be avoided.
Also, we only needed this, since due to the borrowing design we couldn't store
parent and child nodes in the same structure. With reference counting (or the
logic above) this goes away.
I wrote the following in a previous conversation [1].
--
What I see more likely to happen is a situation where the "root" directory
(almost) lives forever, and hence subsequent calls, such as
root.subdir("foo").keep()
effectively are leaks.
One specific example for that would be usb_debug_root, which is created in the
module scope of usb-common and is used by USB host / gadget / phy drivers.
The same is true for other cases where the debugfs "root" is created in the
module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively leak memory everytime a
device is unplugged (or unbound in general).
--
[1] https://lore.kernel.org/lkml/aCR9cD7OcSefeaUm@pollux/
> This way, you can drop foo/ before dropping bar/ and baz/ without that
> having to be unsafe.
>
> Whether that's best implemented by calling dget/dput on the dentry or by
> having Rust keep track of a separate Rust-only refcount, I don't know.
> But I think this is the API we want.
>
> Thoughts?
>
> Alice
On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote:
> > > > Is there no refcount that we can use instead of borrowing? I guess not,
> > > > since one can call `debugfs_remove`. What about a refcount on the rust
> > > > side? or is debugfs not used for "debugging" and needs to have the
> > > > performance of no refcount?
> > >
> > > debugfs should never have any performance issues (i.e. you don't use it
> > > for performant things.)
> > >
> > > So refcount away! That should never be an issue.
> >
> > What I imagine would be the ideal API for Rust is the following:
> >
> > * For each file / dir you create, you get a Rust object that owns it.
> >
> > * When you destroy one of these Rust objects, it disappears from the
> > file system. I.e., dropping a directory removes things recursively.
> >
> > * If you remove a directory before the removing objects inside it, then
> > the Rust objects become "ghost" objects that are still usable, but not
> > visible in the file system anywhere. I.e. calling methods on them
> > succeed but are no-ops.
>
> If we would want to keep an entry alive as long as there are more leaves, we'd
> obviously need to reference count things.
>
> But what do we need reference counting for with this logic? Shouldn't this be
> also possible without?
Well, my understanding is that when you remove the parent directory, the
dentries for child directories and files are freed, so trying to use the
Rust objects that correspond to those child dentries would be a UAF. I
want to refcount those child entries so that they at least remain valid
memory even if they're no longer visible in the file system.
> > * Possibly we have a way to drop a Rust object without removing it from
> > the file system. In this case, it can never be accessed from Rust
> > again, and the only way to remove it is to drop its parent directory.
>
> I'm not sure about this one.
>
> IIUC, this basically brings back the "keep() logic", which I still think is a
> footgun and should be avoided.
>
> Also, we only needed this, since due to the borrowing design we couldn't store
> parent and child nodes in the same structure. With reference counting (or the
> logic above) this goes away.
>
> I wrote the following in a previous conversation [1].
>
> --
>
> What I see more likely to happen is a situation where the "root" directory
> (almost) lives forever, and hence subsequent calls, such as
>
> root.subdir("foo").keep()
>
> effectively are leaks.
>
> One specific example for that would be usb_debug_root, which is created in the
> module scope of usb-common and is used by USB host / gadget / phy drivers.
>
> The same is true for other cases where the debugfs "root" is created in the
> module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively leak memory everytime a
> device is unplugged (or unbound in general).
Where is the leak? If the file is still visible in the file system, then
it's not a leak to still have the memory. If the file got removed by
removing its parent, then my intent is that this should free the memory
of the child object.
Alice
On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> > On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > > * If you remove a directory before the removing objects inside it, then
> > > the Rust objects become "ghost" objects that are still usable, but not
> > > visible in the file system anywhere. I.e. calling methods on them
> > > succeed but are no-ops.
> >
> > If we would want to keep an entry alive as long as there are more leaves, we'd
> > obviously need to reference count things.
> >
> > But what do we need reference counting for with this logic? Shouldn't this be
> > also possible without?
>
> Well, my understanding is that when you remove the parent directory, the
> dentries for child directories and files are freed, so trying to use the
> Rust objects that correspond to those child dentries would be a UAF. I
> want to refcount those child entries so that they at least remain valid
> memory even if they're no longer visible in the file system.
Yes, that makes sense.
Instead of using the dentry pointer as a handle, we could also use the entry's
path and do a lookup whenever the entry is used. Not saying this is better
though.
> > > * Possibly we have a way to drop a Rust object without removing it from
> > > the file system. In this case, it can never be accessed from Rust
> > > again, and the only way to remove it is to drop its parent directory.
> >
> > I'm not sure about this one.
> >
> > IIUC, this basically brings back the "keep() logic", which I still think is a
> > footgun and should be avoided.
> >
> > Also, we only needed this, since due to the borrowing design we couldn't store
> > parent and child nodes in the same structure. With reference counting (or the
> > logic above) this goes away.
> >
> > I wrote the following in a previous conversation [1].
> >
> > --
> >
> > What I see more likely to happen is a situation where the "root" directory
> > (almost) lives forever, and hence subsequent calls, such as
> >
> > root.subdir("foo").keep()
> >
> > effectively are leaks.
> >
> > One specific example for that would be usb_debug_root, which is created in the
> > module scope of usb-common and is used by USB host / gadget / phy drivers.
> >
> > The same is true for other cases where the debugfs "root" is created in the
> > module scope, but subsequent entries are created by driver instances. If a
> > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > device is unplugged (or unbound in general).
>
> Where is the leak? If the file is still visible in the file system, then
> it's not a leak to still have the memory. If the file got removed by
> removing its parent, then my intent is that this should free the memory
> of the child object.
Well, take the case I described above, where the debugfs "root" is created in
the module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively the file / directory
(and subsequently also the corresponding memory) everytime a device is unplugged
(or unbound in general)."
If the module is built-in the directory from the module scope is *never*
removed, but the entries the driver e.g. creates in probe() for a particular
device with keep() will pile up endlessly, especially for hot-pluggable devices.
(It's getting even worse when there's data bound to such a leaked file, that
might even contain a vtable that is entered from any of the fops of the file.)
That'd be clearly a bug, but for the driver author calling keep() seems like a
valid thing to do -- to me that's clearly a built-in footgun.
On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > the file system. In this case, it can never be accessed from Rust
> > > > again, and the only way to remove it is to drop its parent directory.
> > >
> > > I'm not sure about this one.
> > >
> > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > footgun and should be avoided.
> > >
> > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > parent and child nodes in the same structure. With reference counting (or the
> > > logic above) this goes away.
> > >
> > > I wrote the following in a previous conversation [1].
> > >
> > > --
> > >
> > > What I see more likely to happen is a situation where the "root" directory
> > > (almost) lives forever, and hence subsequent calls, such as
> > >
> > > root.subdir("foo").keep()
> > >
> > > effectively are leaks.
> > >
> > > One specific example for that would be usb_debug_root, which is created in the
> > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > >
> > > The same is true for other cases where the debugfs "root" is created in the
> > > module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > device is unplugged (or unbound in general).
> >
> > Where is the leak? If the file is still visible in the file system, then
> > it's not a leak to still have the memory. If the file got removed by
> > removing its parent, then my intent is that this should free the memory
> > of the child object.
>
> Well, take the case I described above, where the debugfs "root" is created in
> the module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively the file / directory
> (and subsequently also the corresponding memory) everytime a device is unplugged
> (or unbound in general)."
>
> If the module is built-in the directory from the module scope is *never*
> removed, but the entries the driver e.g. creates in probe() for a particular
> device with keep() will pile up endlessly, especially for hot-pluggable devices.
>
> (It's getting even worse when there's data bound to such a leaked file, that
> might even contain a vtable that is entered from any of the fops of the file.)
>
> That'd be clearly a bug, but for the driver author calling keep() seems like a
> valid thing to do -- to me that's clearly a built-in footgun.
I mean, for cases such as this, I could imagine that you use `keep()` on
the files stored inside of the driver directory, but don't use it on the
directory. That way, you only have to keep a single reference to an
entire directory around, which may be more convenient.
But I also see your point. I don't think keep() is a critical part of
what I think the approach should be.
Alice
On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > > the file system. In this case, it can never be accessed from Rust
> > > > > again, and the only way to remove it is to drop its parent directory.
> > > >
> > > > I'm not sure about this one.
> > > >
> > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > footgun and should be avoided.
> > > >
> > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > parent and child nodes in the same structure. With reference counting (or the
> > > > logic above) this goes away.
> > > >
> > > > I wrote the following in a previous conversation [1].
> > > >
> > > > --
> > > >
> > > > What I see more likely to happen is a situation where the "root" directory
> > > > (almost) lives forever, and hence subsequent calls, such as
> > > >
> > > > root.subdir("foo").keep()
> > > >
> > > > effectively are leaks.
> > > >
> > > > One specific example for that would be usb_debug_root, which is created in the
> > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > >
> > > > The same is true for other cases where the debugfs "root" is created in the
> > > > module scope, but subsequent entries are created by driver instances. If a
> > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > device is unplugged (or unbound in general).
> > >
> > > Where is the leak? If the file is still visible in the file system, then
> > > it's not a leak to still have the memory. If the file got removed by
> > > removing its parent, then my intent is that this should free the memory
> > > of the child object.
> >
> > Well, take the case I described above, where the debugfs "root" is created in
> > the module scope, but subsequent entries are created by driver instances. If a
> > driver would use keep() in such a case, we'd effectively the file / directory
> > (and subsequently also the corresponding memory) everytime a device is unplugged
> > (or unbound in general)."
> >
> > If the module is built-in the directory from the module scope is *never*
> > removed, but the entries the driver e.g. creates in probe() for a particular
> > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> >
> > (It's getting even worse when there's data bound to such a leaked file, that
> > might even contain a vtable that is entered from any of the fops of the file.)
> >
> > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > valid thing to do -- to me that's clearly a built-in footgun.
>
> I mean, for cases such as this, I could imagine that you use `keep()` on
> the files stored inside of the driver directory, but don't use it on the
> directory. That way, you only have to keep a single reference to an
> entire directory around, which may be more convenient.
No, sorry, but debugfs files are "create and forget" type of things.
The caller has NO reference back to the file at all in the C version,
let's not add that functionality back to the rust side after I spent a
long time removing it from the C code :)
If you really want to delete a debugfs file that you have created in the
past, then look it up and delete it with the call that is present for
that.
The only thing I think that might be worth "keeping" in some form, as an
object reference as discussed, is a debugfs directory.
thanks,
greg k-h
On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> > On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > > > the file system. In this case, it can never be accessed from Rust
> > > > > > again, and the only way to remove it is to drop its parent directory.
> > > > >
> > > > > I'm not sure about this one.
> > > > >
> > > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > > footgun and should be avoided.
> > > > >
> > > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > > parent and child nodes in the same structure. With reference counting (or the
> > > > > logic above) this goes away.
> > > > >
> > > > > I wrote the following in a previous conversation [1].
> > > > >
> > > > > --
> > > > >
> > > > > What I see more likely to happen is a situation where the "root" directory
> > > > > (almost) lives forever, and hence subsequent calls, such as
> > > > >
> > > > > root.subdir("foo").keep()
> > > > >
> > > > > effectively are leaks.
> > > > >
> > > > > One specific example for that would be usb_debug_root, which is created in the
> > > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > > >
> > > > > The same is true for other cases where the debugfs "root" is created in the
> > > > > module scope, but subsequent entries are created by driver instances. If a
> > > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > > device is unplugged (or unbound in general).
> > > >
> > > > Where is the leak? If the file is still visible in the file system, then
> > > > it's not a leak to still have the memory. If the file got removed by
> > > > removing its parent, then my intent is that this should free the memory
> > > > of the child object.
> > >
> > > Well, take the case I described above, where the debugfs "root" is created in
> > > the module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively the file / directory
> > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > (or unbound in general)."
> > >
> > > If the module is built-in the directory from the module scope is *never*
> > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > >
> > > (It's getting even worse when there's data bound to such a leaked file, that
> > > might even contain a vtable that is entered from any of the fops of the file.)
> > >
> > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > valid thing to do -- to me that's clearly a built-in footgun.
> >
> > I mean, for cases such as this, I could imagine that you use `keep()` on
> > the files stored inside of the driver directory, but don't use it on the
> > directory. That way, you only have to keep a single reference to an
> > entire directory around, which may be more convenient.
>
> No, sorry, but debugfs files are "create and forget" type of things.
> The caller has NO reference back to the file at all in the C version,
> let's not add that functionality back to the rust side after I spent a
> long time removing it from the C code :)
This response sounds like we may have a different understanding of what "keep"
means.
Earlier versions of this patch series introduced a keep() method for both
debugfs::File and debugfs::Dir, which would consume, and hence get rid of, the
instance of a debugfs::File or a debugfs::Dir object, while *keeping* the
corresponding directory / file in the filesystem.
This makes it easy for a driver to leak the directory / file in the filesystem,
as explained in [1], which you seemed to agree with in [2].
Was this a misunderstanding?
> If you really want to delete a debugfs file that you have created in the
> past, then look it up and delete it with the call that is present for
> that.
This is promoting a C pattern (which for C code obviously makes a lot of sense),
i.e. we have a function that creates a thing and another function that removes
the thing given a handle of the created thing. Whether the handle is valid is
the responsibility of the caller.
In this case the handle would be the filename. For instance:
debugfs_create_file("foo", parent, ...);
debugfs_remove(debugfs_lookup("foo", parent));
This leaves room to the caller for making mistakes, e.g. if the caller makes a
typo in the filename. In this case we wouldn't even recognize it from the return
value, since there is none.
In Rust, we do things differently, i.e. we wrap things into an object that
cleans up itself, once it goes out of scope. For instance:
let file = debugfs::File::new("foo", parent);
Subsequently we store file in a structure that represents the time we want this
file to exist. Once it goes out of scope, it's removed automatically.
This is better, since we can't make any mistakes anymore, i.e. we can't mess up
the filename or pass the wrong parent to clean things up.
Depending on whether the above was a misunderstanding and depending on how you
think about it with this rationale, I have quite some more reasons why we don't
want to have files / directories around in the filesystem without a
corresponding object representation in Rust.
But before I write up a lot more text, I'd like to see if we're not already on
the same page. :-)
> The only thing I think that might be worth "keeping" in some form, as an
> object reference as discussed, is a debugfs directory.
[1] https://lore.kernel.org/lkml/aC7DVewqqWIKetmk@pollux/
[2] https://lore.kernel.org/lkml/2025052205-thing-daylight-1115@gregkh/
On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> > > On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > > > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > > > > the file system. In this case, it can never be accessed from Rust
> > > > > > > again, and the only way to remove it is to drop its parent directory.
> > > > > >
> > > > > > I'm not sure about this one.
> > > > > >
> > > > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > > > footgun and should be avoided.
> > > > > >
> > > > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > > > parent and child nodes in the same structure. With reference counting (or the
> > > > > > logic above) this goes away.
> > > > > >
> > > > > > I wrote the following in a previous conversation [1].
> > > > > >
> > > > > > --
> > > > > >
> > > > > > What I see more likely to happen is a situation where the "root" directory
> > > > > > (almost) lives forever, and hence subsequent calls, such as
> > > > > >
> > > > > > root.subdir("foo").keep()
> > > > > >
> > > > > > effectively are leaks.
> > > > > >
> > > > > > One specific example for that would be usb_debug_root, which is created in the
> > > > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > > > >
> > > > > > The same is true for other cases where the debugfs "root" is created in the
> > > > > > module scope, but subsequent entries are created by driver instances. If a
> > > > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > > > device is unplugged (or unbound in general).
> > > > >
> > > > > Where is the leak? If the file is still visible in the file system, then
> > > > > it's not a leak to still have the memory. If the file got removed by
> > > > > removing its parent, then my intent is that this should free the memory
> > > > > of the child object.
> > > >
> > > > Well, take the case I described above, where the debugfs "root" is created in
> > > > the module scope, but subsequent entries are created by driver instances. If a
> > > > driver would use keep() in such a case, we'd effectively the file / directory
> > > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > > (or unbound in general)."
> > > >
> > > > If the module is built-in the directory from the module scope is *never*
> > > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > >
> > > > (It's getting even worse when there's data bound to such a leaked file, that
> > > > might even contain a vtable that is entered from any of the fops of the file.)
> > > >
> > > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > > valid thing to do -- to me that's clearly a built-in footgun.
> > >
> > > I mean, for cases such as this, I could imagine that you use `keep()` on
> > > the files stored inside of the driver directory, but don't use it on the
> > > directory. That way, you only have to keep a single reference to an
> > > entire directory around, which may be more convenient.
> >
> > No, sorry, but debugfs files are "create and forget" type of things.
> > The caller has NO reference back to the file at all in the C version,
> > let's not add that functionality back to the rust side after I spent a
> > long time removing it from the C code :)
>
> This response sounds like we may have a different understanding of what "keep"
> means.
>
> Earlier versions of this patch series introduced a keep() method for both
> debugfs::File and debugfs::Dir, which would consume, and hence get rid of, the
> instance of a debugfs::File or a debugfs::Dir object, while *keeping* the
> corresponding directory / file in the filesystem.
>
> This makes it easy for a driver to leak the directory / file in the filesystem,
> as explained in [1], which you seemed to agree with in [2].
>
> Was this a misunderstanding?
Kind of, see below:
> > If you really want to delete a debugfs file that you have created in the
> > past, then look it up and delete it with the call that is present for
> > that.
>
> This is promoting a C pattern (which for C code obviously makes a lot of sense),
> i.e. we have a function that creates a thing and another function that removes
> the thing given a handle of the created thing. Whether the handle is valid is
> the responsibility of the caller.
>
> In this case the handle would be the filename. For instance:
>
> debugfs_create_file("foo", parent, ...);
> debugfs_remove(debugfs_lookup("foo", parent));
>
> This leaves room to the caller for making mistakes, e.g. if the caller makes a
> typo in the filename. In this case we wouldn't even recognize it from the return
> value, since there is none.
>
> In Rust, we do things differently, i.e. we wrap things into an object that
> cleans up itself, once it goes out of scope. For instance:
>
> let file = debugfs::File::new("foo", parent);
>
> Subsequently we store file in a structure that represents the time we want this
> file to exist. Once it goes out of scope, it's removed automatically.
>
> This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> the filename or pass the wrong parent to clean things up.
>
> Depending on whether the above was a misunderstanding and depending on how you
> think about it with this rationale, I have quite some more reasons why we don't
> want to have files / directories around in the filesystem without a
> corresponding object representation in Rust.
>
> But before I write up a lot more text, I'd like to see if we're not already on
> the same page. :-)
Ok, let's knock up the api here first before worrying about the
implementation, as we seem to all be a bit confused as to what we want
to do.
Ideally, yes, I would like to NOT have any rust structure represent a
debugfs file, as that is the way the C api has been evolving. We are
one step away from debugfs_create_file() being a void function that
doesn't return anything, and I don't want to see us go "backwards" here.
But the main reason I did that work was to keep people from trying to
determine if a file creation worked or not and to do different logic
based on that. I think the Rust api CAN do both here, as you and Alice
have explained, so we should be ok.
So how about this for an example api, two structures, debugfs::Directory
and debugfs::File, which have only two types of operations that can be
done on them:
To create a debugfs directory, we do:
let my_dir = debugfs::Directory::new("foo_dir", parent);
There is no other Directory method, other than "drop", when the object
goes out of scope, which will clean up the directory and ALL child
objects at the same time (implementation details to be figured out
later.)
That will ALWAYS succeed no matter if the backend debugfs call failed or
not, and you have no way of knowing if it really did create something or
not. That directory you can then use to pass into debugfs file or
directory creation functions.
Same for creating a debugfs file, you would do something like:
let my_file = debugfs::File::new("foo_file", my_dir);
depending on the "file type" you want to create, there will be different
new_* methods (new_u32, new_u64, etc.).
which also ALWAYS succeeds. And again, as you want to keep objects
around, you have to have a reference on it at all times for it to exist
in the filesystem. If you drop it, then it too will be removed from
debugfs.
And again, that's the only operation you can do on this object, there
are no other methods for it other than "new" and "drop".
Now there are issues with implementation details here like:
- Do you want to keep the list of file and dir objects in the rust
structure to manually clean them up when we go to drop the
directory?
- Do we want to force all files to be dropped before the directory?
Or do we want to just let the C side clean it all up automagically
instead?
and other things like that, but we can argue about that once we settle
on the outward-facing api first.
I can live with this type of api. It's simple and seems hard to abuse
or get wrong from a user point-of-view, and should be pretty
straight-forward on the binding side as well. It will take a bit more
work on the user when creating debugfs files than the C api did, but
that's something that you want to impose on this api, not me :)
Sound reasonable? Or am I missing something else here?
thanks,
greg k-h
On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, May 22, 2025 at 02:01:26PM +0000, Alice Ryhl wrote:
> > > > On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> > > > > On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > > > > > > > * Possibly we have a way to drop a Rust object without removing it from
> > > > > > > > the file system. In this case, it can never be accessed from Rust
> > > > > > > > again, and the only way to remove it is to drop its parent directory.
> > > > > > >
> > > > > > > I'm not sure about this one.
> > > > > > >
> > > > > > > IIUC, this basically brings back the "keep() logic", which I still think is a
> > > > > > > footgun and should be avoided.
> > > > > > >
> > > > > > > Also, we only needed this, since due to the borrowing design we couldn't store
> > > > > > > parent and child nodes in the same structure. With reference counting (or the
> > > > > > > logic above) this goes away.
> > > > > > >
> > > > > > > I wrote the following in a previous conversation [1].
> > > > > > >
> > > > > > > --
> > > > > > >
> > > > > > > What I see more likely to happen is a situation where the "root" directory
> > > > > > > (almost) lives forever, and hence subsequent calls, such as
> > > > > > >
> > > > > > > root.subdir("foo").keep()
> > > > > > >
> > > > > > > effectively are leaks.
> > > > > > >
> > > > > > > One specific example for that would be usb_debug_root, which is created in the
> > > > > > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > > > > > >
> > > > > > > The same is true for other cases where the debugfs "root" is created in the
> > > > > > > module scope, but subsequent entries are created by driver instances. If a
> > > > > > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > > > > > device is unplugged (or unbound in general).
> > > > > >
> > > > > > Where is the leak? If the file is still visible in the file system, then
> > > > > > it's not a leak to still have the memory. If the file got removed by
> > > > > > removing its parent, then my intent is that this should free the memory
> > > > > > of the child object.
> > > > >
> > > > > Well, take the case I described above, where the debugfs "root" is created in
> > > > > the module scope, but subsequent entries are created by driver instances. If a
> > > > > driver would use keep() in such a case, we'd effectively the file / directory
> > > > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > > > (or unbound in general)."
> > > > >
> > > > > If the module is built-in the directory from the module scope is *never*
> > > > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > > >
> > > > > (It's getting even worse when there's data bound to such a leaked file, that
> > > > > might even contain a vtable that is entered from any of the fops of the file.)
> > > > >
> > > > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > > > valid thing to do -- to me that's clearly a built-in footgun.
> > > >
> > > > I mean, for cases such as this, I could imagine that you use `keep()` on
> > > > the files stored inside of the driver directory, but don't use it on the
> > > > directory. That way, you only have to keep a single reference to an
> > > > entire directory around, which may be more convenient.
> > >
> > > No, sorry, but debugfs files are "create and forget" type of things.
> > > The caller has NO reference back to the file at all in the C version,
> > > let's not add that functionality back to the rust side after I spent a
> > > long time removing it from the C code :)
> >
> > This response sounds like we may have a different understanding of what "keep"
> > means.
> >
> > Earlier versions of this patch series introduced a keep() method for both
> > debugfs::File and debugfs::Dir, which would consume, and hence get rid of, the
> > instance of a debugfs::File or a debugfs::Dir object, while *keeping* the
> > corresponding directory / file in the filesystem.
> >
> > This makes it easy for a driver to leak the directory / file in the filesystem,
> > as explained in [1], which you seemed to agree with in [2].
> >
> > Was this a misunderstanding?
>
> Kind of, see below:
>
> > > If you really want to delete a debugfs file that you have created in the
> > > past, then look it up and delete it with the call that is present for
> > > that.
> >
> > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > i.e. we have a function that creates a thing and another function that removes
> > the thing given a handle of the created thing. Whether the handle is valid is
> > the responsibility of the caller.
> >
> > In this case the handle would be the filename. For instance:
> >
> > debugfs_create_file("foo", parent, ...);
> > debugfs_remove(debugfs_lookup("foo", parent));
> >
> > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > typo in the filename. In this case we wouldn't even recognize it from the return
> > value, since there is none.
> >
> > In Rust, we do things differently, i.e. we wrap things into an object that
> > cleans up itself, once it goes out of scope. For instance:
> >
> > let file = debugfs::File::new("foo", parent);
> >
> > Subsequently we store file in a structure that represents the time we want this
> > file to exist. Once it goes out of scope, it's removed automatically.
> >
> > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > the filename or pass the wrong parent to clean things up.
> >
> > Depending on whether the above was a misunderstanding and depending on how you
> > think about it with this rationale, I have quite some more reasons why we don't
> > want to have files / directories around in the filesystem without a
> > corresponding object representation in Rust.
> >
> > But before I write up a lot more text, I'd like to see if we're not already on
> > the same page. :-)
>
> Ok, let's knock up the api here first before worrying about the
> implementation, as we seem to all be a bit confused as to what we want
> to do.
>
> Ideally, yes, I would like to NOT have any rust structure represent a
> debugfs file, as that is the way the C api has been evolving. We are
> one step away from debugfs_create_file() being a void function that
> doesn't return anything, and I don't want to see us go "backwards" here.
>
> But the main reason I did that work was to keep people from trying to
> determine if a file creation worked or not and to do different logic
> based on that. I think the Rust api CAN do both here, as you and Alice
> have explained, so we should be ok.
>
> So how about this for an example api, two structures, debugfs::Directory
> and debugfs::File, which have only two types of operations that can be
> done on them:
>
> To create a debugfs directory, we do:
>
> let my_dir = debugfs::Directory::new("foo_dir", parent);
>
> There is no other Directory method, other than "drop", when the object
> goes out of scope, which will clean up the directory and ALL child
> objects at the same time (implementation details to be figured out
> later.)
>
> That will ALWAYS succeed no matter if the backend debugfs call failed or
> not, and you have no way of knowing if it really did create something or
> not. That directory you can then use to pass into debugfs file or
> directory creation functions.
>
> Same for creating a debugfs file, you would do something like:
>
> let my_file = debugfs::File::new("foo_file", my_dir);
>
> depending on the "file type" you want to create, there will be different
> new_* methods (new_u32, new_u64, etc.).
>
> which also ALWAYS succeeds. And again, as you want to keep objects
> around, you have to have a reference on it at all times for it to exist
> in the filesystem. If you drop it, then it too will be removed from
> debugfs.
>
> And again, that's the only operation you can do on this object, there
> are no other methods for it other than "new" and "drop".
>
> Now there are issues with implementation details here like:
> - Do you want to keep the list of file and dir objects in the rust
> structure to manually clean them up when we go to drop the
> directory?
> - Do we want to force all files to be dropped before the directory?
> Or do we want to just let the C side clean it all up automagically
> instead?
> and other things like that, but we can argue about that once we settle
> on the outward-facing api first.
>
> I can live with this type of api. It's simple and seems hard to abuse
> or get wrong from a user point-of-view, and should be pretty
> straight-forward on the binding side as well. It will take a bit more
> work on the user when creating debugfs files than the C api did, but
> that's something that you want to impose on this api, not me :)
>
> Sound reasonable? Or am I missing something else here?
This does sound reasonable to me. I'm fine with always succeeding. I do
agree with Danilo that I would bias towards dir.file("foo_file"), but
it's not a super big deal to me.
I think the main thing I want to say here is that we should *not* force
files to be dropped before the directory, and we should also not force
child directories to be dropped before their parent directories. Trying
to impose that requirement on callers will make the API significantly
harder to use, and it's not worth it.
Alice
On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > If you really want to delete a debugfs file that you have created in the
> > > past, then look it up and delete it with the call that is present for
> > > that.
> >
> > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > i.e. we have a function that creates a thing and another function that removes
> > the thing given a handle of the created thing. Whether the handle is valid is
> > the responsibility of the caller.
> >
> > In this case the handle would be the filename. For instance:
> >
> > debugfs_create_file("foo", parent, ...);
> > debugfs_remove(debugfs_lookup("foo", parent));
> >
> > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > typo in the filename. In this case we wouldn't even recognize it from the return
> > value, since there is none.
> >
> > In Rust, we do things differently, i.e. we wrap things into an object that
> > cleans up itself, once it goes out of scope. For instance:
> >
> > let file = debugfs::File::new("foo", parent);
> >
> > Subsequently we store file in a structure that represents the time we want this
> > file to exist. Once it goes out of scope, it's removed automatically.
> >
> > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > the filename or pass the wrong parent to clean things up.
> >
> > Depending on whether the above was a misunderstanding and depending on how you
> > think about it with this rationale, I have quite some more reasons why we don't
> > want to have files / directories around in the filesystem without a
> > corresponding object representation in Rust.
> >
> > But before I write up a lot more text, I'd like to see if we're not already on
> > the same page. :-)
>
> Ok, let's knock up the api here first before worrying about the
> implementation, as we seem to all be a bit confused as to what we want
> to do.
>
> Ideally, yes, I would like to NOT have any rust structure represent a
> debugfs file, as that is the way the C api has been evolving. We are
> one step away from debugfs_create_file() being a void function that
> doesn't return anything, and I don't want to see us go "backwards" here.
>
> But the main reason I did that work was to keep people from trying to
> determine if a file creation worked or not and to do different logic
> based on that. I think the Rust api CAN do both here, as you and Alice
> have explained, so we should be ok.
>
> So how about this for an example api, two structures, debugfs::Directory
> and debugfs::File, which have only two types of operations that can be
> done on them:
>
> To create a debugfs directory, we do:
>
> let my_dir = debugfs::Directory::new("foo_dir", parent);
>
> There is no other Directory method, other than "drop", when the object
> goes out of scope, which will clean up the directory and ALL child
> objects at the same time (implementation details to be figured out
> later.)
>
> That will ALWAYS succeed no matter if the backend debugfs call failed or
> not, and you have no way of knowing if it really did create something or
> not. That directory you can then use to pass into debugfs file or
> directory creation functions.
>
> Same for creating a debugfs file, you would do something like:
>
> let my_file = debugfs::File::new("foo_file", my_dir);
>
> depending on the "file type" you want to create, there will be different
> new_* methods (new_u32, new_u64, etc.).
>
> which also ALWAYS succeeds. And again, as you want to keep objects
> around, you have to have a reference on it at all times for it to exist
> in the filesystem. If you drop it, then it too will be removed from
> debugfs.
Great! That's exactly what I think the API should look like as well. :-)
> And again, that's the only operation you can do on this object, there
> are no other methods for it other than "new" and "drop".
We probably still want
let foo_dir = my_dir.subdir("foo_dir")
foo_dir.file("foo_file")
for convinience, rather than having to call
let bar_dir = Dir::new("foo_dir", my_dir)
File::new("bar_file", bar_dir)
all the time. But otherwise, sounds reasonable to me.
> Now there are issues with implementation details here like:
> - Do you want to keep the list of file and dir objects in the rust
> structure to manually clean them up when we go to drop the
> directory?
> - Do we want to force all files to be dropped before the directory?
> Or do we want to just let the C side clean it all up automagically
> instead?
> and other things like that, but we can argue about that once we settle
> on the outward-facing api first.
The only thing I don't want to is to allow to leak files or directories, i.e.
File::new("bar_file", bar_dir).leak()
which keeps the file in the filesystem, but takes away the handle from the Rust
side, such that it won't be removed from the filesystem anymore when the handle
goes out of scope, which can cause nasty bugs. But I think there isn't any
benefit to allow this anyways and it isn't needed with reference counting.
I'm open on things like having "ghost" objects as proposed by Alice or force all
files to be dropped before the directory, etc. though.
> I can live with this type of api. It's simple and seems hard to abuse
> or get wrong from a user point-of-view, and should be pretty
> straight-forward on the binding side as well. It will take a bit more
> work on the user when creating debugfs files than the C api did, but
> that's something that you want to impose on this api, not me :)
Let's see if it actually does, I'm not sure it will be, at least not for all
use-cases. But even if, it's probably worth the extra robustness.
> Sound reasonable? Or am I missing something else here?
Yes, sounds good to me.
On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > > If you really want to delete a debugfs file that you have created in the
> > > > past, then look it up and delete it with the call that is present for
> > > > that.
> > >
> > > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > > i.e. we have a function that creates a thing and another function that removes
> > > the thing given a handle of the created thing. Whether the handle is valid is
> > > the responsibility of the caller.
> > >
> > > In this case the handle would be the filename. For instance:
> > >
> > > debugfs_create_file("foo", parent, ...);
> > > debugfs_remove(debugfs_lookup("foo", parent));
> > >
> > > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > > typo in the filename. In this case we wouldn't even recognize it from the return
> > > value, since there is none.
> > >
> > > In Rust, we do things differently, i.e. we wrap things into an object that
> > > cleans up itself, once it goes out of scope. For instance:
> > >
> > > let file = debugfs::File::new("foo", parent);
> > >
> > > Subsequently we store file in a structure that represents the time we want this
> > > file to exist. Once it goes out of scope, it's removed automatically.
> > >
> > > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > > the filename or pass the wrong parent to clean things up.
> > >
> > > Depending on whether the above was a misunderstanding and depending on how you
> > > think about it with this rationale, I have quite some more reasons why we don't
> > > want to have files / directories around in the filesystem without a
> > > corresponding object representation in Rust.
> > >
> > > But before I write up a lot more text, I'd like to see if we're not already on
> > > the same page. :-)
> >
> > Ok, let's knock up the api here first before worrying about the
> > implementation, as we seem to all be a bit confused as to what we want
> > to do.
> >
> > Ideally, yes, I would like to NOT have any rust structure represent a
> > debugfs file, as that is the way the C api has been evolving. We are
> > one step away from debugfs_create_file() being a void function that
> > doesn't return anything, and I don't want to see us go "backwards" here.
> >
> > But the main reason I did that work was to keep people from trying to
> > determine if a file creation worked or not and to do different logic
> > based on that. I think the Rust api CAN do both here, as you and Alice
> > have explained, so we should be ok.
> >
> > So how about this for an example api, two structures, debugfs::Directory
> > and debugfs::File, which have only two types of operations that can be
> > done on them:
> >
> > To create a debugfs directory, we do:
> >
> > let my_dir = debugfs::Directory::new("foo_dir", parent);
> >
> > There is no other Directory method, other than "drop", when the object
> > goes out of scope, which will clean up the directory and ALL child
> > objects at the same time (implementation details to be figured out
> > later.)
> >
> > That will ALWAYS succeed no matter if the backend debugfs call failed or
> > not, and you have no way of knowing if it really did create something or
> > not. That directory you can then use to pass into debugfs file or
> > directory creation functions.
> >
> > Same for creating a debugfs file, you would do something like:
> >
> > let my_file = debugfs::File::new("foo_file", my_dir);
> >
> > depending on the "file type" you want to create, there will be different
> > new_* methods (new_u32, new_u64, etc.).
> >
> > which also ALWAYS succeeds. And again, as you want to keep objects
> > around, you have to have a reference on it at all times for it to exist
> > in the filesystem. If you drop it, then it too will be removed from
> > debugfs.
>
> Great! That's exactly what I think the API should look like as well. :-)
>
> > And again, that's the only operation you can do on this object, there
> > are no other methods for it other than "new" and "drop".
>
> We probably still want
>
> let foo_dir = my_dir.subdir("foo_dir")
> foo_dir.file("foo_file")
>
> for convinience, rather than having to call
>
> let bar_dir = Dir::new("foo_dir", my_dir)
> File::new("bar_file", bar_dir)
>
> all the time. But otherwise, sounds reasonable to me.
>
> > Now there are issues with implementation details here like:
> > - Do you want to keep the list of file and dir objects in the rust
> > structure to manually clean them up when we go to drop the
> > directory?
> > - Do we want to force all files to be dropped before the directory?
> > Or do we want to just let the C side clean it all up automagically
> > instead?
> > and other things like that, but we can argue about that once we settle
> > on the outward-facing api first.
>
> The only thing I don't want to is to allow to leak files or directories, i.e.
>
> File::new("bar_file", bar_dir).leak()
>
> which keeps the file in the filesystem, but takes away the handle from the Rust
> side, such that it won't be removed from the filesystem anymore when the handle
> goes out of scope, which can cause nasty bugs. But I think there isn't any
> benefit to allow this anyways and it isn't needed with reference counting.
Why not have leak() for files only? That way, the files still go away
when you drop the directory, and you don't have to keep around a bunch
of File objects in your Rust structs.
Alice
On Fri, May 23, 2025 at 05:09:13PM +0000, Alice Ryhl wrote:
> On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> > The only thing I don't want to is to allow to leak files or directories, i.e.
> >
> > File::new("bar_file", bar_dir).leak()
> >
> > which keeps the file in the filesystem, but takes away the handle from the Rust
> > side, such that it won't be removed from the filesystem anymore when the handle
> > goes out of scope, which can cause nasty bugs. But I think there isn't any
> > benefit to allow this anyways and it isn't needed with reference counting.
>
> Why not have leak() for files only? That way, the files still go away
> when you drop the directory, and you don't have to keep around a bunch
> of File objects in your Rust structs.
In a previous mail I said that there are more reasons why we don't want to have
files (or directories) in the filesystem without an object representation in
Rust.
One of them is when attaching private data to a file, like we do with
debugfs_create_file().
When we do this, in most of the cases we want to share data between a driver
structure and the debugfs file (e.g. a GPU's VM to dump the virtual address
space layout).
And most likely we do this by passing an Arc<T> to dir.file(), to make the file
own a reference to the corresponding reference counted object (in C we just hope
that no one frees the object while the debugfs file holds a pointer to it).
The question is, what happens to this reference if we would subsequently call
file.leak()? We can't drop the Arc, because otherwise we risk a UAF in the
filesystem callback, but we can't keep it either because otherwise we *really*
leak this reference, even if the parent directory is removed eventually.
--
Now, one could say, what about attaching the file's private data to the directory
instead? (And I think this has been proposed already elsewhere.)
But I really don't like this approach, because it would mean that when creating
the directory we would necessarily need to know all the files we will ever
create in this directory *and* have all the corresponding private data
available at this point of time. But that would be an insane requirement.
For instance, let's assume a driver creates a directory 'clients', which for
every connected userspace "client" wants to provide a file with some metadata
for this client.
Sure, you can work around this somehow, e.g. by using a mutex protected Vec as
private data or by always creating a new directory first for dynamically created
debugfs entires. But this sounds pretty horrible to me.
On Sat, May 24, 2025 at 02:25:27PM +0200, Danilo Krummrich wrote:
> On Fri, May 23, 2025 at 05:09:13PM +0000, Alice Ryhl wrote:
> > On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> > > The only thing I don't want to is to allow to leak files or directories, i.e.
> > >
> > > File::new("bar_file", bar_dir).leak()
> > >
> > > which keeps the file in the filesystem, but takes away the handle from the Rust
> > > side, such that it won't be removed from the filesystem anymore when the handle
> > > goes out of scope, which can cause nasty bugs. But I think there isn't any
> > > benefit to allow this anyways and it isn't needed with reference counting.
> >
> > Why not have leak() for files only? That way, the files still go away
> > when you drop the directory, and you don't have to keep around a bunch
> > of File objects in your Rust structs.
>
> In a previous mail I said that there are more reasons why we don't want to have
> files (or directories) in the filesystem without an object representation in
> Rust.
>
> One of them is when attaching private data to a file, like we do with
> debugfs_create_file().
>
> When we do this, in most of the cases we want to share data between a driver
> structure and the debugfs file (e.g. a GPU's VM to dump the virtual address
> space layout).
>
> And most likely we do this by passing an Arc<T> to dir.file(), to make the file
> own a reference to the corresponding reference counted object (in C we just hope
> that no one frees the object while the debugfs file holds a pointer to it).
>
> The question is, what happens to this reference if we would subsequently call
> file.leak()? We can't drop the Arc, because otherwise we risk a UAF in the
> filesystem callback, but we can't keep it either because otherwise we *really*
> leak this reference, even if the parent directory is removed eventually.
>
> --
>
> Now, one could say, what about attaching the file's private data to the directory
> instead? (And I think this has been proposed already elsewhere.)
>
> But I really don't like this approach, because it would mean that when creating
> the directory we would necessarily need to know all the files we will ever
> create in this directory *and* have all the corresponding private data
> available at this point of time. But that would be an insane requirement.
>
> For instance, let's assume a driver creates a directory 'clients', which for
> every connected userspace "client" wants to provide a file with some metadata
> for this client.
>
> Sure, you can work around this somehow, e.g. by using a mutex protected Vec as
> private data or by always creating a new directory first for dynamically created
> debugfs entires. But this sounds pretty horrible to me.
I do agree that if we support file.leak(), then the private data should:
1. Be provided when creating the file, not the directory.
2. Be stored with the directory and be freed with it.
3. Not be encoded in the type of the directory.
Some sort of list with data to free when the directory is freed could
work. But it sounds like we should not consider file.leak() for the
initial API.
Alice
On Tue, May 27, 2025 at 11:38:02AM +0000, Alice Ryhl wrote:
> On Sat, May 24, 2025 at 02:25:27PM +0200, Danilo Krummrich wrote:
> > On Fri, May 23, 2025 at 05:09:13PM +0000, Alice Ryhl wrote:
> > > On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> > > > The only thing I don't want to is to allow to leak files or directories, i.e.
> > > >
> > > > File::new("bar_file", bar_dir).leak()
> > > >
> > > > which keeps the file in the filesystem, but takes away the handle from the Rust
> > > > side, such that it won't be removed from the filesystem anymore when the handle
> > > > goes out of scope, which can cause nasty bugs. But I think there isn't any
> > > > benefit to allow this anyways and it isn't needed with reference counting.
> > >
> > > Why not have leak() for files only? That way, the files still go away
> > > when you drop the directory, and you don't have to keep around a bunch
> > > of File objects in your Rust structs.
> >
> > In a previous mail I said that there are more reasons why we don't want to have
> > files (or directories) in the filesystem without an object representation in
> > Rust.
> >
> > One of them is when attaching private data to a file, like we do with
> > debugfs_create_file().
> >
> > When we do this, in most of the cases we want to share data between a driver
> > structure and the debugfs file (e.g. a GPU's VM to dump the virtual address
> > space layout).
> >
> > And most likely we do this by passing an Arc<T> to dir.file(), to make the file
> > own a reference to the corresponding reference counted object (in C we just hope
> > that no one frees the object while the debugfs file holds a pointer to it).
> >
> > The question is, what happens to this reference if we would subsequently call
> > file.leak()? We can't drop the Arc, because otherwise we risk a UAF in the
> > filesystem callback, but we can't keep it either because otherwise we *really*
> > leak this reference, even if the parent directory is removed eventually.
> >
> > --
> >
> > Now, one could say, what about attaching the file's private data to the directory
> > instead? (And I think this has been proposed already elsewhere.)
> >
> > But I really don't like this approach, because it would mean that when creating
> > the directory we would necessarily need to know all the files we will ever
> > create in this directory *and* have all the corresponding private data
> > available at this point of time. But that would be an insane requirement.
> >
> > For instance, let's assume a driver creates a directory 'clients', which for
> > every connected userspace "client" wants to provide a file with some metadata
> > for this client.
> >
> > Sure, you can work around this somehow, e.g. by using a mutex protected Vec as
> > private data or by always creating a new directory first for dynamically created
> > debugfs entires. But this sounds pretty horrible to me.
>
> I do agree that if we support file.leak(), then the private data should:
>
> 1. Be provided when creating the file, not the directory.
> 2. Be stored with the directory and be freed with it.
> 3. Not be encoded in the type of the directory.
>
> Some sort of list with data to free when the directory is freed could
> work.
I think this would be problematic too.
Think of the client example I gave above: Let's assume a driver creates a
'client' directory for every bound device and we create a single file per client
connecting to the corresponding device exposed to userspace.
Let's assume clients do connect and disconnect frequently. If we'd allow
file.leak() in this case, it would mean that all the private data of the clients
that have ever been connected piles up in the 'client' directories's private
data storage until the 'client' directory is removed, which is when the device
is unbound, which might be never.
> But it sounds like we should not consider file.leak() for the
> initial API.
Yeah, let's not have file.leak() either.
I'm going to try to rework the patch based on the feedback here, and have a few things I want to check on to reduce cycling: 1. Every approach described in this thread will result in the creation APIs (create a root dir, create a subdir, create a file) being fallible (returns `Result<>`) because they use `Arc` in the implementation which fundamentally allocates. I previously got the feedback that the debugfs API should have no possibility of handleable errors. This would *not* mean that we were signaling the errors out of the raw debugfs bindings into Rust, but Rust would be able to detect `Arc` allocation failures unless you really want me to be hiding allocation failures (I could theoretically do this by generating a dummy object on allocation failure, but this would add complexity and seems questionable). 2. If we're not using borrowing to statically prevent entries from outliving their parents, there are two possible semantics for what to do when an entry *does* outlive its parent: a. The parent doesn't actually get `debugfs_remove` called until all its child objects have left scope too. This can be accomplished by just having a `_parent: Option<Arc<Entry>>` in each `Entry` structure. b. We get ghost objects, e.g. the child object is still around, but it is somehow able to discover that its `dentry` pointer is no longer valid and avoids trying to use it. The best solution I have to this is to again maintain an optional parent `Arc`, but also have `Entry` hold an additional refcount member. When you want to create a subdir, a file in a directory, or delete any of those, you'd walk the spine of parents, trying to claim the extra refcount on everything. Once you've got it, you can safely delete yourself or create a subdirectory or file. Then, whoever is last out of the extra refcount (either the owning object or someone holding a reference while they perform an operation on themselves) can try to clean up, again by claiming the spine above them. 3. If we're not supporting `leak` or lifetime-based `File` anymore, then I don't think my method of attaching data is needed anymore. It was complex primarily in order to support statically enforced lifetimes on dirs/subdirs/files, and several of the tricks I was using don't work without a lifetime on the `File` type. Instead, I'll make it so that `File` objects can attach a `ForeignOwnable`, with the expected common cases being either `&'static MyData` or `Arc<MyData>`. Unless I get additional feedback, I'm currently implementing 2-a, using `ForeignOwnable` based attachments of data to files, and returning `Result` where the only actual failure possibility is allocation failure.
On Fri, May 23, 2025 at 11:42:23AM +0200, Danilo Krummrich wrote:
> On Fri, May 23, 2025 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 22, 2025 at 07:53:49PM +0200, Danilo Krummrich wrote:
> > > On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > > If you really want to delete a debugfs file that you have created in the
> > > > past, then look it up and delete it with the call that is present for
> > > > that.
> > >
> > > This is promoting a C pattern (which for C code obviously makes a lot of sense),
> > > i.e. we have a function that creates a thing and another function that removes
> > > the thing given a handle of the created thing. Whether the handle is valid is
> > > the responsibility of the caller.
> > >
> > > In this case the handle would be the filename. For instance:
> > >
> > > debugfs_create_file("foo", parent, ...);
> > > debugfs_remove(debugfs_lookup("foo", parent));
> > >
> > > This leaves room to the caller for making mistakes, e.g. if the caller makes a
> > > typo in the filename. In this case we wouldn't even recognize it from the return
> > > value, since there is none.
> > >
> > > In Rust, we do things differently, i.e. we wrap things into an object that
> > > cleans up itself, once it goes out of scope. For instance:
> > >
> > > let file = debugfs::File::new("foo", parent);
> > >
> > > Subsequently we store file in a structure that represents the time we want this
> > > file to exist. Once it goes out of scope, it's removed automatically.
> > >
> > > This is better, since we can't make any mistakes anymore, i.e. we can't mess up
> > > the filename or pass the wrong parent to clean things up.
> > >
> > > Depending on whether the above was a misunderstanding and depending on how you
> > > think about it with this rationale, I have quite some more reasons why we don't
> > > want to have files / directories around in the filesystem without a
> > > corresponding object representation in Rust.
> > >
> > > But before I write up a lot more text, I'd like to see if we're not already on
> > > the same page. :-)
> >
> > Ok, let's knock up the api here first before worrying about the
> > implementation, as we seem to all be a bit confused as to what we want
> > to do.
> >
> > Ideally, yes, I would like to NOT have any rust structure represent a
> > debugfs file, as that is the way the C api has been evolving. We are
> > one step away from debugfs_create_file() being a void function that
> > doesn't return anything, and I don't want to see us go "backwards" here.
> >
> > But the main reason I did that work was to keep people from trying to
> > determine if a file creation worked or not and to do different logic
> > based on that. I think the Rust api CAN do both here, as you and Alice
> > have explained, so we should be ok.
> >
> > So how about this for an example api, two structures, debugfs::Directory
> > and debugfs::File, which have only two types of operations that can be
> > done on them:
> >
> > To create a debugfs directory, we do:
> >
> > let my_dir = debugfs::Directory::new("foo_dir", parent);
> >
> > There is no other Directory method, other than "drop", when the object
> > goes out of scope, which will clean up the directory and ALL child
> > objects at the same time (implementation details to be figured out
> > later.)
> >
> > That will ALWAYS succeed no matter if the backend debugfs call failed or
> > not, and you have no way of knowing if it really did create something or
> > not. That directory you can then use to pass into debugfs file or
> > directory creation functions.
> >
> > Same for creating a debugfs file, you would do something like:
> >
> > let my_file = debugfs::File::new("foo_file", my_dir);
> >
> > depending on the "file type" you want to create, there will be different
> > new_* methods (new_u32, new_u64, etc.).
> >
> > which also ALWAYS succeeds. And again, as you want to keep objects
> > around, you have to have a reference on it at all times for it to exist
> > in the filesystem. If you drop it, then it too will be removed from
> > debugfs.
>
> Great! That's exactly what I think the API should look like as well. :-)
>
> > And again, that's the only operation you can do on this object, there
> > are no other methods for it other than "new" and "drop".
>
> We probably still want
>
> let foo_dir = my_dir.subdir("foo_dir")
> foo_dir.file("foo_file")
>
> for convinience, rather than having to call
>
> let bar_dir = Dir::new("foo_dir", my_dir)
> File::new("bar_file", bar_dir)
>
> all the time. But otherwise, sounds reasonable to me.
That would be very convenient, but given that we have 20+ different
types of debugfs files that we can create, that might get complex to
define that way. But that's just an implementation detail to work
out...
> > Now there are issues with implementation details here like:
> > - Do you want to keep the list of file and dir objects in the rust
> > structure to manually clean them up when we go to drop the
> > directory?
> > - Do we want to force all files to be dropped before the directory?
> > Or do we want to just let the C side clean it all up automagically
> > instead?
> > and other things like that, but we can argue about that once we settle
> > on the outward-facing api first.
>
> The only thing I don't want to is to allow to leak files or directories, i.e.
>
> File::new("bar_file", bar_dir).leak()
>
> which keeps the file in the filesystem, but takes away the handle from the Rust
> side, such that it won't be removed from the filesystem anymore when the handle
> goes out of scope, which can cause nasty bugs. But I think there isn't any
> benefit to allow this anyways and it isn't needed with reference counting.
I agree, I can live with that too.
> I'm open on things like having "ghost" objects as proposed by Alice or force all
> files to be dropped before the directory, etc. though.
"ghost" objects will get messy I think, and yes, cleaning up will be a
bit tricky here, which is why at the C level I am taking away the idea
of a file that can be controlled as an individual object entirely. If
you want the Rust code to have objects for the files, that's something
that the bindings are going to have to handle well. Somehow :)
thanks,
greg k-h
On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > Well, take the case I described above, where the debugfs "root" is created in
> > > the module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively the file / directory
> > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > (or unbound in general)."
> > >
> > > If the module is built-in the directory from the module scope is *never*
> > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > >
> > > (It's getting even worse when there's data bound to such a leaked file, that
> > > might even contain a vtable that is entered from any of the fops of the file.)
> > >
> > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > valid thing to do -- to me that's clearly a built-in footgun.
> >
> > I mean, for cases such as this, I could imagine that you use `keep()` on
> > the files stored inside of the driver directory, but don't use it on the
> > directory. That way, you only have to keep a single reference to an
> > entire directory around, which may be more convenient.
>
> No, sorry, but debugfs files are "create and forget" type of things.
> The caller has NO reference back to the file at all in the C version,
> let's not add that functionality back to the rust side after I spent a
> long time removing it from the C code :)
>
> If you really want to delete a debugfs file that you have created in the
> past, then look it up and delete it with the call that is present for
> that.
>
> The only thing I think that might be worth "keeping" in some form, as an
> object reference as discussed, is a debugfs directory.
That could work if we don't have any Rust value for files at all. The
problem is that if we do have such values, then code like this:
let my_file = dir.create_file("my_file_name");
dir.delete_file("my_file_name");
my_file.do_something();
would be a UAF on the last line. We have to design the Rust API to avoid
such UAF, which is why I suggested the ghost objects; the delete_file()
call leaves my_file in a valid but useless state. And as a ghost object,
the .do_something() call becomes a no-op since the file is now missing
from the filesystem.
Alice
On Thu, May 22, 2025 at 05:40:43PM +0000, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > > Well, take the case I described above, where the debugfs "root" is created in
> > > > the module scope, but subsequent entries are created by driver instances. If a
> > > > driver would use keep() in such a case, we'd effectively the file / directory
> > > > (and subsequently also the corresponding memory) everytime a device is unplugged
> > > > (or unbound in general)."
> > > >
> > > > If the module is built-in the directory from the module scope is *never*
> > > > removed, but the entries the driver e.g. creates in probe() for a particular
> > > > device with keep() will pile up endlessly, especially for hot-pluggable devices.
> > > >
> > > > (It's getting even worse when there's data bound to such a leaked file, that
> > > > might even contain a vtable that is entered from any of the fops of the file.)
> > > >
> > > > That'd be clearly a bug, but for the driver author calling keep() seems like a
> > > > valid thing to do -- to me that's clearly a built-in footgun.
> > >
> > > I mean, for cases such as this, I could imagine that you use `keep()` on
> > > the files stored inside of the driver directory, but don't use it on the
> > > directory. That way, you only have to keep a single reference to an
> > > entire directory around, which may be more convenient.
> >
> > No, sorry, but debugfs files are "create and forget" type of things.
> > The caller has NO reference back to the file at all in the C version,
> > let's not add that functionality back to the rust side after I spent a
> > long time removing it from the C code :)
> >
> > If you really want to delete a debugfs file that you have created in the
> > past, then look it up and delete it with the call that is present for
> > that.
> >
> > The only thing I think that might be worth "keeping" in some form, as an
> > object reference as discussed, is a debugfs directory.
>
> That could work if we don't have any Rust value for files at all. The
> problem is that if we do have such values, then code like this:
>
> let my_file = dir.create_file("my_file_name");
> dir.delete_file("my_file_name");
> my_file.do_something();
>
> would be a UAF on the last line. We have to design the Rust API to avoid
> such UAF, which is why I suggested the ghost objects; the delete_file()
> call leaves my_file in a valid but useless state. And as a ghost object,
> the .do_something() call becomes a no-op since the file is now missing
> from the filesystem.
See my other response now, but I don't want there to be any
.do_something() calls that are possible here at all. All a debugfs file
object can do is be created, or be destroyed.
thanks,
greg k-h
On Thu May 22, 2025 at 7:40 PM CEST, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
>> No, sorry, but debugfs files are "create and forget" type of things.
>> The caller has NO reference back to the file at all in the C version,
>> let's not add that functionality back to the rust side after I spent a
>> long time removing it from the C code :)
>>
>> If you really want to delete a debugfs file that you have created in the
>> past, then look it up and delete it with the call that is present for
>> that.
>>
>> The only thing I think that might be worth "keeping" in some form, as an
>> object reference as discussed, is a debugfs directory.
>
> That could work if we don't have any Rust value for files at all. The
> problem is that if we do have such values, then code like this:
>
> let my_file = dir.create_file("my_file_name");
> dir.delete_file("my_file_name");
> my_file.do_something();
I might have misunderstood something, but "deleting a debugfs file" is
not the same as freeing the representing object (is that a dentry?). So
you could still call `do_something`, it just wouldn't do anything.
Or did I misunderstand?
---
Cheers,
Benno
> would be a UAF on the last line. We have to design the Rust API to avoid
> such UAF, which is why I suggested the ghost objects; the delete_file()
> call leaves my_file in a valid but useless state. And as a ghost object,
> the .do_something() call becomes a no-op since the file is now missing
> from the filesystem.
>
> Alice
On Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> > On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> > > On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > > > * If you remove a directory before the removing objects inside it, then
> > > > the Rust objects become "ghost" objects that are still usable, but not
> > > > visible in the file system anywhere. I.e. calling methods on them
> > > > succeed but are no-ops.
> > >
> > > If we would want to keep an entry alive as long as there are more leaves, we'd
> > > obviously need to reference count things.
> > >
> > > But what do we need reference counting for with this logic? Shouldn't this be
> > > also possible without?
> >
> > Well, my understanding is that when you remove the parent directory, the
> > dentries for child directories and files are freed, so trying to use the
> > Rust objects that correspond to those child dentries would be a UAF. I
> > want to refcount those child entries so that they at least remain valid
> > memory even if they're no longer visible in the file system.
>
> Yes, that makes sense.
>
> Instead of using the dentry pointer as a handle, we could also use the entry's
> path and do a lookup whenever the entry is used. Not saying this is better
> though.
Either is fine, as long as that "handle" isn't exported out of the
internal binding for anyone to access directly.
> > > What I see more likely to happen is a situation where the "root" directory
> > > (almost) lives forever, and hence subsequent calls, such as
> > >
> > > root.subdir("foo").keep()
> > >
> > > effectively are leaks.
> > >
> > > One specific example for that would be usb_debug_root, which is created in the
> > > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > >
> > > The same is true for other cases where the debugfs "root" is created in the
> > > module scope, but subsequent entries are created by driver instances. If a
> > > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > > device is unplugged (or unbound in general).
> >
> > Where is the leak? If the file is still visible in the file system, then
> > it's not a leak to still have the memory. If the file got removed by
> > removing its parent, then my intent is that this should free the memory
> > of the child object.
>
> Well, take the case I described above, where the debugfs "root" is created in
> the module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively the file / directory
> (and subsequently also the corresponding memory) everytime a device is unplugged
> (or unbound in general)."
>
> If the module is built-in the directory from the module scope is *never*
> removed, but the entries the driver e.g. creates in probe() for a particular
> device with keep() will pile up endlessly, especially for hot-pluggable devices.
>
> (It's getting even worse when there's data bound to such a leaked file, that
> might even contain a vtable that is entered from any of the fops of the file.)
>
> That'd be clearly a bug, but for the driver author calling keep() seems like a
> valid thing to do -- to me that's clearly a built-in footgun.
Yeah, I like the keep() thing less and less and I think it can be done
without it entirely.
thanks,
greg k-h
On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote: > On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote: > > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote: > > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote: > > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote: > > > >> However, I really think we should keep the code as it is in this version and > > > >> just don't provide an example that utilizes ManuallyDrop and forget(). > > > >> > > > >> I don't see how the idea of "manually dropping" (sub-)directories and files > > > >> provides any real value compared to just storing their instance in a driver > > > >> structure as long as they should stay alive, which is much more intuitive > > > >> anyways. > > > > > > > > We can't easily do this, because dropping a root directory recursively > > > > drops everything underneath it. This means that if I have > > > > > > > > foo/ > > > > - bar/ > > > > - baz/ > > > > > > > > Then my directory handle for `bar` have to be guaranteed to outlive my > > > > directory handle for `foo` so that I know it's didn't get deleted > > > > under me. This is why they have a borrow onto their parent directory. > > > > This borrow means that you can't (without `unsafe`, or something like > > > > `yoke`) keep handles to `foo` and `bar` in the same struct. > > > > > > Is there no refcount that we can use instead of borrowing? I guess not, > > > since one can call `debugfs_remove`. What about a refcount on the rust > > > side? or is debugfs not used for "debugging" and needs to have the > > > performance of no refcount? > > > > debugfs should never have any performance issues (i.e. you don't use it > > for performant things.) > > > > So refcount away! That should never be an issue. > > What I imagine would be the ideal API for Rust is the following: > > * For each file / dir you create, you get a Rust object that owns it. > > * When you destroy one of these Rust objects, it disappears from the > file system. I.e., dropping a directory removes things recursively. > > * If you remove a directory before the removing objects inside it, then > the Rust objects become "ghost" objects that are still usable, but not > visible in the file system anywhere. I.e. calling methods on them > succeed but are no-ops. Why not just also remove those objects at the same time? That would be more like what the filesystem logic itself does today. > * Possibly we have a way to drop a Rust object without removing it from > the file system. In this case, it can never be accessed from Rust > again, and the only way to remove it is to drop its parent directory. This too would be nice as that's how the existing api works in C. > This way, you can drop foo/ before dropping bar/ and baz/ without that > having to be unsafe. > > Whether that's best implemented by calling dget/dput on the dentry or by > having Rust keep track of a separate Rust-only refcount, I don't know. > But I think this is the API we want. > > Thoughts? Sounds reasonable to me and should be easy to use, which is the key feature/requirement of debugfs. thanks, greg k-h
On Wed, May 21, 2025 at 06:47:40AM +0200, Greg Kroah-Hartman wrote: > On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote: > > On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote: > > > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote: > > > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote: > > > > >> However, I really think we should keep the code as it is in this version and > > > > >> just don't provide an example that utilizes ManuallyDrop and forget(). > > > > >> > > > > >> I don't see how the idea of "manually dropping" (sub-)directories and files > > > > >> provides any real value compared to just storing their instance in a driver > > > > >> structure as long as they should stay alive, which is much more intuitive > > > > >> anyways. > > > > > > > > > > We can't easily do this, because dropping a root directory recursively > > > > > drops everything underneath it. This means that if I have > > > > > > > > > > foo/ > > > > > - bar/ > > > > > - baz/ > > > > > > > > > > Then my directory handle for `bar` have to be guaranteed to outlive my > > > > > directory handle for `foo` so that I know it's didn't get deleted > > > > > under me. This is why they have a borrow onto their parent directory. > > > > > This borrow means that you can't (without `unsafe`, or something like > > > > > `yoke`) keep handles to `foo` and `bar` in the same struct. > > > > > > > > Is there no refcount that we can use instead of borrowing? I guess not, > > > > since one can call `debugfs_remove`. What about a refcount on the rust > > > > side? or is debugfs not used for "debugging" and needs to have the > > > > performance of no refcount? > > > > > > debugfs should never have any performance issues (i.e. you don't use it > > > for performant things.) > > > > > > So refcount away! That should never be an issue. > > > > What I imagine would be the ideal API for Rust is the following: > > > > * For each file / dir you create, you get a Rust object that owns it. > > > > * When you destroy one of these Rust objects, it disappears from the > > file system. I.e., dropping a directory removes things recursively. > > > > * If you remove a directory before the removing objects inside it, then > > the Rust objects become "ghost" objects that are still usable, but not > > visible in the file system anywhere. I.e. calling methods on them > > succeed but are no-ops. > > Why not just also remove those objects at the same time? That would be > more like what the filesystem logic itself does today. I mean, if I write a driver and I store a Rust object that holds a debugfs directory in some random struct of mine, how is debugfs going to go remove it from my struct? It can't. At most we could enforce that you destroy them in the right order, but actually designing an API which enforces that is difficult and results in something inconvenient to use. Of course, drivers probably shouldn't keep those ghost objects around, but I don't think the API should hard enforce that. Alice > > * Possibly we have a way to drop a Rust object without removing it from > > the file system. In this case, it can never be accessed from Rust > > again, and the only way to remove it is to drop its parent directory. > > This too would be nice as that's how the existing api works in C. > > > This way, you can drop foo/ before dropping bar/ and baz/ without that > > having to be unsafe. > > > > Whether that's best implemented by calling dget/dput on the dentry or by > > having Rust keep track of a separate Rust-only refcount, I don't know. > > But I think this is the API we want. > > > > Thoughts? > > Sounds reasonable to me and should be easy to use, which is the key > feature/requirement of debugfs. > > thanks, > > greg k-h
On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote: > On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote: > > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote: > > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote: > > >> However, I really think we should keep the code as it is in this version and > > >> just don't provide an example that utilizes ManuallyDrop and forget(). > > >> > > >> I don't see how the idea of "manually dropping" (sub-)directories and files > > >> provides any real value compared to just storing their instance in a driver > > >> structure as long as they should stay alive, which is much more intuitive > > >> anyways. > > > > > > We can't easily do this, because dropping a root directory recursively > > > drops everything underneath it. This means that if I have > > > > > > foo/ > > > - bar/ > > > - baz/ > > > > > > Then my directory handle for `bar` have to be guaranteed to outlive my > > > directory handle for `foo` so that I know it's didn't get deleted > > > under me. This is why they have a borrow onto their parent directory. > > > This borrow means that you can't (without `unsafe`, or something like > > > `yoke`) keep handles to `foo` and `bar` in the same struct. > > > > Is there no refcount that we can use instead of borrowing? I guess not, > > since one can call `debugfs_remove`. What about a refcount on the rust > > side? or is debugfs not used for "debugging" and needs to have the > > performance of no refcount? > > debugfs should never have any performance issues (i.e. you don't use it > for performant things.) > > So refcount away! That should never be an issue. Reference counting (parent) directories should lead to a much cleaner solution. I mentioned that previously, but also said in that context that it's a bit contrary to how the C API is utilized currently, which usually isn't desired. However, if we're fine with that I think it's superior to the borrowing solution, which requires keep(). IMHO keep() is a footgun in general, even if not callable for "root" directories.
On Thu May 15, 2025 at 2:37 PM CEST, Danilo Krummrich wrote: > On Thu, May 15, 2025 at 01:43:09PM +0200, Greg Kroah-Hartman wrote: >> On Thu, May 15, 2025 at 10:59:44AM +0200, Benno Lossin wrote: >> > On Wed May 14, 2025 at 11:55 PM CEST, Matthew Maurer wrote: >> > > On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote: >> > >> However, I really think we should keep the code as it is in this version and >> > >> just don't provide an example that utilizes ManuallyDrop and forget(). >> > >> >> > >> I don't see how the idea of "manually dropping" (sub-)directories and files >> > >> provides any real value compared to just storing their instance in a driver >> > >> structure as long as they should stay alive, which is much more intuitive >> > >> anyways. >> > > >> > > We can't easily do this, because dropping a root directory recursively >> > > drops everything underneath it. This means that if I have >> > > >> > > foo/ >> > > - bar/ >> > > - baz/ >> > > >> > > Then my directory handle for `bar` have to be guaranteed to outlive my >> > > directory handle for `foo` so that I know it's didn't get deleted >> > > under me. This is why they have a borrow onto their parent directory. >> > > This borrow means that you can't (without `unsafe`, or something like >> > > `yoke`) keep handles to `foo` and `bar` in the same struct. >> > >> > Is there no refcount that we can use instead of borrowing? I guess not, >> > since one can call `debugfs_remove`. What about a refcount on the rust >> > side? or is debugfs not used for "debugging" and needs to have the >> > performance of no refcount? >> >> debugfs should never have any performance issues (i.e. you don't use it >> for performant things.) >> >> So refcount away! That should never be an issue. > > Reference counting (parent) directories should lead to a much cleaner solution. > > I mentioned that previously, but also said in that context that it's a bit > contrary to how the C API is utilized currently, which usually isn't desired. We could also change the C side to use refcounting :) It is probably a bigger change (I have no idea how common the usage of debugfs is). In my mind, it would also allow the C side to benefit from the same "drop the dirs that you don't need anymore and all subdirs will be removed if they also aren't referenced any longer" thing. > However, if we're fine with that I think it's superior to the borrowing > solution, which requires keep(). IMHO keep() is a footgun in general, even if > not callable for "root" directories. I would prefer refcounting over forgetting, it much more clearly shows who owns the debugfs dirs. Also, in which cases would one not call `.keep()`? The USB example from the other thread comes to mind, but there you might be able to borrow a `Dir<'static` for `'static`, are there other cases? --- Cheers, Benno
On Wed, May 14, 2025 at 02:55:02PM -0700, Matthew Maurer wrote:
> On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > > On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > > > +impl kernel::Module for RustDebugFs {
> > > > + fn init(_this: &'static ThisModule) -> 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.
> > > > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > > > + // at the end of the scope - it will be cleaned up when `debugfs` is.
> > > > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> > >
> > > I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > > `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > > support of the wrapped type. But in this case, `Dir` is sometimes
> > > intended to not be dropped, so I'd rather have a `.keep()` function I
> > > saw mentioned somewhere.
> >
> > I agree, if we really want to "officially" support to forget() (sub-)directories
> > and files in order to rely on the recursive cleanup of the "root" directory, it
> > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > have some kind of keep() and / or forget() method, to make it clear that this is
> > supported and by design and won't lead to any leaks.
>
> OK, I can do this with `.keep()` just being equivalent to
> `ManuallyDrop::new`. Since we now only have "by default, things are
> dropped", rather than two states, we shouldn't need a `.destroy()`
> method.
>
> >
> > Consequently, this would mean that we'd need something like your proposed const
> > generic on the Dir type, such that keep() / forget() cannot be called on the
> > "root" directory.
>
> Just to ensure I understand correctly, your opinion is that on
> balance, additional complexity in the API types are worth making it
> less ergonomic to suppress drop on a root directory, keeping in mind
> that they still *can* suppress drop on that directory. You don't want
> the extra API variants to do anything else special other than change
> the ergonomics of `.keep()` (e.g. don't have subdirectories be default
> pre-`.keep()`'d).
Absolutely, yes! Having the keep() method available for "root" directories is
clearly a footgun, because it suggests that it is a valid thing to call for a
"root" directory, but actually is almost always a bug.
What do we have a powerful type system for if we don't take advantage of it to
prevent bugs? :-)
On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
>> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
>> > +impl kernel::Module for RustDebugFs {
>> > + fn init(_this: &'static ThisModule) -> 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.
>> > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
>> > + // at the end of the scope - it will be cleaned up when `debugfs` is.
>> > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
>>
>> I dislike the direct usage of `ManuallyDrop`. To me the usage of
>> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
>> support of the wrapped type. But in this case, `Dir` is sometimes
>> intended to not be dropped, so I'd rather have a `.keep()` function I
>> saw mentioned somewhere.
>
> I agree, if we really want to "officially" support to forget() (sub-)directories
> and files in order to rely on the recursive cleanup of the "root" directory, it
> should be covered explicitly by the API. I.e. (sub-)directories and files should
> have some kind of keep() and / or forget() method, to make it clear that this is
> supported and by design and won't lead to any leaks.
>
> Consequently, this would mean that we'd need something like your proposed const
> generic on the Dir type, such that keep() / forget() cannot be called on the
> "root" directory.
>
> However, I really think we should keep the code as it is in this version and
> just don't provide an example that utilizes ManuallyDrop and forget().
>
> I don't see how the idea of "manually dropping" (sub-)directories and files
> provides any real value compared to just storing their instance in a driver
> structure as long as they should stay alive, which is much more intuitive
> anyways.
Yeah that's whats normally done in Rust anyways. But I think that
lifetimes bite us in this case:
let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
// lifetime `'a` starts in the line above and `sub` borrows `debugfs`
/* code for creating the file etc */
Ok(Self { _debugfs: debugfs, _sub: sub })
// lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
This code won't compile, since we can't store the "root" dir in the same
struct that we want to store the subdir, because the subdir borrows from
the root dir.
Essentially this would require self-referential structs like the
`ouroboros` crate [1] from user-space Rust. We should rather have the
`.keep()` function in the API than use self-referential structs.
[1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
Another problem that only affects complicated debugfs structures is that
you would have to store all subdirs & files somewhere. If the structure
is dynamic and changes over the lifetime of the driver, then you'll need
a `Vec` or store the dirs in `Arc` or similar, leading to extra
allocations.
> It either just adds complexity to the API (due to the need to distingish between
> the "root" directory and sub-directories) or makes the API error prone by
> providing a *valid looking* option to users to leak the "root" directory.
I agree with this, I want that `ManuallyDrop` & `forget` are only used
rarely mostly for low-level operations.
---
Cheers,
Benno
On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote:
>
> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> >> > +impl kernel::Module for RustDebugFs {
> >> > + fn init(_this: &'static ThisModule) -> 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.
> >> > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> >> > + // at the end of the scope - it will be cleaned up when `debugfs` is.
> >> > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> >>
> >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> >> support of the wrapped type. But in this case, `Dir` is sometimes
> >> intended to not be dropped, so I'd rather have a `.keep()` function I
> >> saw mentioned somewhere.
> >
> > I agree, if we really want to "officially" support to forget() (sub-)directories
> > and files in order to rely on the recursive cleanup of the "root" directory, it
> > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > have some kind of keep() and / or forget() method, to make it clear that this is
> > supported and by design and won't lead to any leaks.
> >
> > Consequently, this would mean that we'd need something like your proposed const
> > generic on the Dir type, such that keep() / forget() cannot be called on the
> > "root" directory.
> >
> > However, I really think we should keep the code as it is in this version and
> > just don't provide an example that utilizes ManuallyDrop and forget().
> >
> > I don't see how the idea of "manually dropping" (sub-)directories and files
> > provides any real value compared to just storing their instance in a driver
> > structure as long as they should stay alive, which is much more intuitive
> > anyways.
>
> Yeah that's whats normally done in Rust anyways. But I think that
> lifetimes bite us in this case:
>
> let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
>
> let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
> // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
>
> /* code for creating the file etc */
>
> Ok(Self { _debugfs: debugfs, _sub: sub })
> // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
>
> This code won't compile, since we can't store the "root" dir in the same
> struct that we want to store the subdir, because the subdir borrows from
> the root dir.
>
> Essentially this would require self-referential structs like the
> `ouroboros` crate [1] from user-space Rust. We should rather have the
> `.keep()` function in the API than use self-referential structs.
>
> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
Yes, this is also an issue when it comes to attaching data to debugfs
if you look at the WIP patchset [1]. If we do end up deciding to try
to do self-referential structs for users (I'd rather not, but I
understand I don't have the deciding vote here), I'd strongly suggest
considering self_cell [2] as a model instead of ouroboros - there are
structural UB issues [3] with ouroboros that are unlikely to be
resolved, and the author of ouroboros encourages use of self_cell
instead [4]
[1]: https://lore.kernel.org/all/20250506-debugfs-rust-attach-v2-0-c6f88be3890a@google.com/
[2]: https://docs.rs/self_cell/latest/self_cell/
[3]: https://github.com/rust-lang/miri/issues/2844#issuecomment-1510577943
[4]: https://github.com/someguynamedjosh/ouroboros/issues/88
>
> Another problem that only affects complicated debugfs structures is that
> you would have to store all subdirs & files somewhere. If the structure
> is dynamic and changes over the lifetime of the driver, then you'll need
> a `Vec` or store the dirs in `Arc` or similar, leading to extra
> allocations.
Yep, this is part of why I tried to follow the "Build it, then hold
the needed handles to keep it alive" approach rather than keeping the
entire structure around.
>
> > It either just adds complexity to the API (due to the need to distingish between
> > the "root" directory and sub-directories) or makes the API error prone by
> > providing a *valid looking* option to users to leak the "root" directory.
>
> I agree with this, I want that `ManuallyDrop` & `forget` are only used
> rarely mostly for low-level operations.
Sure, I'll re-instate `Dir::keep` and `File::keep` as aliases for
`ManuallyDrop::new` in the next version. TBD whether we add the bit
that Danilo suggested to make `Dir::keep` not exist for root-level
directories.
>
> ---
> Cheers,
> Benno
On Thu May 15, 2025 at 12:08 AM CEST, Matthew Maurer wrote: > Yes, this is also an issue when it comes to attaching data to debugfs > if you look at the WIP patchset [1]. If we do end up deciding to try > to do self-referential structs for users (I'd rather not, but I > understand I don't have the deciding vote here), I'd strongly suggest > considering self_cell [2] as a model instead of ouroboros - there are > structural UB issues [3] with ouroboros that are unlikely to be > resolved, and the author of ouroboros encourages use of self_cell > instead [4] I do not think we should add something that allows us to have self-referential structs. I only mentioned ouroboros, because in theory it's possible to do in Rust, but we shouldn't do it. --- Cheers, Benno > [1]: https://lore.kernel.org/all/20250506-debugfs-rust-attach-v2-0-c6f88be3890a@google.com/ > [2]: https://docs.rs/self_cell/latest/self_cell/ > [3]: https://github.com/rust-lang/miri/issues/2844#issuecomment-1510577943 > [4]: https://github.com/someguynamedjosh/ouroboros/issues/88
On Wed, May 14, 2025 at 03:08:21PM -0700, Matthew Maurer wrote: > On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote: > > Another problem that only affects complicated debugfs structures is that > > you would have to store all subdirs & files somewhere. If the structure > > is dynamic and changes over the lifetime of the driver, then you'll need > > a `Vec` or store the dirs in `Arc` or similar, leading to extra > > allocations. > > Yep, this is part of why I tried to follow the "Build it, then hold > the needed handles to keep it alive" approach rather than keeping the > entire structure around. I already replied to that [1]: "If it changes dynamically then it's pretty likely that we do not only want to add entries dynamically, but also remove them, which implies that we need to be able to drop them. So, I don't think that's a problem." It is much more of a problem if we can't remove certain entries anymore without removing all of them. [1] https://lore.kernel.org/rust-for-linux/aCR9cD7OcSefeaUm@pollux/
On Wed, May 14, 2025 at 3:14 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, May 14, 2025 at 03:08:21PM -0700, Matthew Maurer wrote: > > On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote: > > > Another problem that only affects complicated debugfs structures is that > > > you would have to store all subdirs & files somewhere. If the structure > > > is dynamic and changes over the lifetime of the driver, then you'll need > > > a `Vec` or store the dirs in `Arc` or similar, leading to extra > > > allocations. > > > > Yep, this is part of why I tried to follow the "Build it, then hold > > the needed handles to keep it alive" approach rather than keeping the > > entire structure around. > > I already replied to that [1]: > > "If it changes dynamically then it's pretty likely that we do not only want to > add entries dynamically, but also remove them, which implies that we need to be > able to drop them. So, I don't think that's a problem." > > It is much more of a problem if we can't remove certain entries anymore without > removing all of them. > > [1] https://lore.kernel.org/rust-for-linux/aCR9cD7OcSefeaUm@pollux/ I think the main question here is this - is it OK to land an API that does static-layout or add-only-layout first, and we figure out how to do dynamic modification later, or do you think we need to solve the self-referential lifetimes issue in the first edition of DebugFS support? If you do think we need support for dynamic layout modification in the first patchset, do you think we want to allow static layouts that forget things, or mandate that we always manage all of the handles for the user?
On Wed, May 14, 2025 at 3:23 PM Matthew Maurer <mmaurer@google.com> wrote: > > On Wed, May 14, 2025 at 3:14 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Wed, May 14, 2025 at 03:08:21PM -0700, Matthew Maurer wrote: > > > On Wed, May 14, 2025 at 2:54 AM Benno Lossin <lossin@kernel.org> wrote: > > > > Another problem that only affects complicated debugfs structures is that > > > > you would have to store all subdirs & files somewhere. If the structure > > > > is dynamic and changes over the lifetime of the driver, then you'll need > > > > a `Vec` or store the dirs in `Arc` or similar, leading to extra > > > > allocations. > > > > > > Yep, this is part of why I tried to follow the "Build it, then hold > > > the needed handles to keep it alive" approach rather than keeping the > > > entire structure around. > > > > I already replied to that [1]: > > > > "If it changes dynamically then it's pretty likely that we do not only want to > > add entries dynamically, but also remove them, which implies that we need to be > > able to drop them. So, I don't think that's a problem." > > > > It is much more of a problem if we can't remove certain entries anymore without > > removing all of them. > > > > [1] https://lore.kernel.org/rust-for-linux/aCR9cD7OcSefeaUm@pollux/ > > I think the main question here is this - is it OK to land an API that > does static-layout or add-only-layout first, and we figure out how to > do dynamic modification later, or do you think we need to solve the > self-referential lifetimes issue in the first edition of DebugFS > support? > > If you do think we need support for dynamic layout modification in the > first patchset, do you think we want to allow static layouts that > forget things, or mandate that we always manage all of the handles for > the user? One further possibility here, which we'd need Greg to weigh in on - we could add a method to the debugfs API intended for Rust usage which specifically releases a directory or file *without* releasing any nested elements. This would mean we could get rid of all the lifetimes on directory and file handles. This variant would mean that if the user did something wrong (e.g. released a root directory before taking an action on a child or releasing it), they wouldn't get undefined behavior, they'd just get no effect. If they wrote this kind of buggy behavior, they might be *surprised* at the file being absent, but no UB would happen. If we wanted a variant that used the current recursive remove, it would still need the lifetimes, with all the difficulties that entails.
On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> >> > +impl kernel::Module for RustDebugFs {
> >> > + fn init(_this: &'static ThisModule) -> 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.
> >> > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> >> > + // at the end of the scope - it will be cleaned up when `debugfs` is.
> >> > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> >>
> >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> >> support of the wrapped type. But in this case, `Dir` is sometimes
> >> intended to not be dropped, so I'd rather have a `.keep()` function I
> >> saw mentioned somewhere.
> >
> > I agree, if we really want to "officially" support to forget() (sub-)directories
> > and files in order to rely on the recursive cleanup of the "root" directory, it
> > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > have some kind of keep() and / or forget() method, to make it clear that this is
> > supported and by design and won't lead to any leaks.
> >
> > Consequently, this would mean that we'd need something like your proposed const
> > generic on the Dir type, such that keep() / forget() cannot be called on the
> > "root" directory.
> >
> > However, I really think we should keep the code as it is in this version and
> > just don't provide an example that utilizes ManuallyDrop and forget().
> >
> > I don't see how the idea of "manually dropping" (sub-)directories and files
> > provides any real value compared to just storing their instance in a driver
> > structure as long as they should stay alive, which is much more intuitive
> > anyways.
>
> Yeah that's whats normally done in Rust anyways. But I think that
> lifetimes bite us in this case:
>
> let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
>
> let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
> // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
>
> /* code for creating the file etc */
>
> Ok(Self { _debugfs: debugfs, _sub: sub })
> // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
>
> This code won't compile, since we can't store the "root" dir in the same
> struct that we want to store the subdir, because the subdir borrows from
> the root dir.
>
> Essentially this would require self-referential structs like the
> `ouroboros` crate [1] from user-space Rust. We should rather have the
> `.keep()` function in the API than use self-referential structs.
Fair enough -- I think we should properly document those limitations, recommend
using keep() for those cases and ensure that we can't call keep() on the "root"
directory then.
Unless, we can find a better solution, which, unfortunately, I can't think of
one. The only thing I can think of is to reference count (parent) directories,
which would be contrary to how the C API works and not desirable.
> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
>
> Another problem that only affects complicated debugfs structures is that
> you would have to store all subdirs & files somewhere. If the structure
> is dynamic and changes over the lifetime of the driver, then you'll need
> a `Vec` or store the dirs in `Arc` or similar, leading to extra
> allocations.
If it changes dynamically then it's pretty likely that we do not only want to
add entries dynamically, but also remove them, which implies that we need to be
able to drop them. So, I don't think that's a problem.
What I see more likely to happen is a situation where the "root" directory
(almost) lives forever, and hence subsequent calls, such as
root.subdir("foo").keep()
effectively are leaks.
One specific example for that would be usb_debug_root, which is created in the
module scope of usb-common and is used by USB host / gadget / phy drivers.
The same is true for other cases where the debugfs "root" is created in the
module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively leak memory everytime a
device is unplugged (or unbound in general).
>
> > It either just adds complexity to the API (due to the need to distingish between
> > the "root" directory and sub-directories) or makes the API error prone by
> > providing a *valid looking* option to users to leak the "root" directory.
>
> I agree with this, I want that `ManuallyDrop` & `forget` are only used
> rarely mostly for low-level operations.
>
> ---
> Cheers,
> Benno
On Wed, May 14, 2025 at 4:24 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
> > On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
> > > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > >> > +impl kernel::Module for RustDebugFs {
> > >> > + fn init(_this: &'static ThisModule) -> 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.
> > >> > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > >> > + // at the end of the scope - it will be cleaned up when `debugfs` is.
> > >> > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> > >>
> > >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > >> support of the wrapped type. But in this case, `Dir` is sometimes
> > >> intended to not be dropped, so I'd rather have a `.keep()` function I
> > >> saw mentioned somewhere.
> > >
> > > I agree, if we really want to "officially" support to forget() (sub-)directories
> > > and files in order to rely on the recursive cleanup of the "root" directory, it
> > > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > > have some kind of keep() and / or forget() method, to make it clear that this is
> > > supported and by design and won't lead to any leaks.
> > >
> > > Consequently, this would mean that we'd need something like your proposed const
> > > generic on the Dir type, such that keep() / forget() cannot be called on the
> > > "root" directory.
> > >
> > > However, I really think we should keep the code as it is in this version and
> > > just don't provide an example that utilizes ManuallyDrop and forget().
> > >
> > > I don't see how the idea of "manually dropping" (sub-)directories and files
> > > provides any real value compared to just storing their instance in a driver
> > > structure as long as they should stay alive, which is much more intuitive
> > > anyways.
> >
> > Yeah that's whats normally done in Rust anyways. But I think that
> > lifetimes bite us in this case:
> >
> > let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
> >
> > let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
> > // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
> >
> > /* code for creating the file etc */
> >
> > Ok(Self { _debugfs: debugfs, _sub: sub })
> > // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
> >
> > This code won't compile, since we can't store the "root" dir in the same
> > struct that we want to store the subdir, because the subdir borrows from
> > the root dir.
> >
> > Essentially this would require self-referential structs like the
> > `ouroboros` crate [1] from user-space Rust. We should rather have the
> > `.keep()` function in the API than use self-referential structs.
>
> Fair enough -- I think we should properly document those limitations, recommend
> using keep() for those cases and ensure that we can't call keep() on the "root"
> directory then.
>
> Unless, we can find a better solution, which, unfortunately, I can't think of
> one. The only thing I can think of is to reference count (parent) directories,
> which would be contrary to how the C API works and not desirable.
>
> > [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
> >
> > Another problem that only affects complicated debugfs structures is that
> > you would have to store all subdirs & files somewhere. If the structure
> > is dynamic and changes over the lifetime of the driver, then you'll need
> > a `Vec` or store the dirs in `Arc` or similar, leading to extra
> > allocations.
>
> If it changes dynamically then it's pretty likely that we do not only want to
> add entries dynamically, but also remove them, which implies that we need to be
> able to drop them. So, I don't think that's a problem.
>
> What I see more likely to happen is a situation where the "root" directory
> (almost) lives forever, and hence subsequent calls, such as
>
> root.subdir("foo").keep()
>
> effectively are leaks.
>
> One specific example for that would be usb_debug_root, which is created in the
> module scope of usb-common and is used by USB host / gadget / phy drivers.
>
> The same is true for other cases where the debugfs "root" is created in the
> module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively leak memory everytime a
> device is unplugged (or unbound in general).
Yes, this is one of the things I don't currently have a good safe
solution for without introducing something with similar power to
`self_cell` or a bespoke type implemented unsafely like the `Tree` I
mentioned earlier in the chain.
>
> >
> > > It either just adds complexity to the API (due to the need to distingish between
> > > the "root" directory and sub-directories) or makes the API error prone by
> > > providing a *valid looking* option to users to leak the "root" directory.
> >
> > I agree with this, I want that `ManuallyDrop` & `forget` are only used
> > rarely mostly for low-level operations.
> >
> > ---
> > Cheers,
> > Benno
On Wed May 14, 2025 at 1:24 PM CEST, Danilo Krummrich wrote:
> On Wed, May 14, 2025 at 11:54:39AM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 11:07 AM CEST, Danilo Krummrich wrote:
>> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
>> >> On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
>> >> > +impl kernel::Module for RustDebugFs {
>> >> > + fn init(_this: &'static ThisModule) -> 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.
>> >> > + // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
>> >> > + // at the end of the scope - it will be cleaned up when `debugfs` is.
>> >> > + let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
>> >>
>> >> I dislike the direct usage of `ManuallyDrop`. To me the usage of
>> >> `ManuallyDrop` signifies that one has to opt out of `Drop` without the
>> >> support of the wrapped type. But in this case, `Dir` is sometimes
>> >> intended to not be dropped, so I'd rather have a `.keep()` function I
>> >> saw mentioned somewhere.
>> >
>> > I agree, if we really want to "officially" support to forget() (sub-)directories
>> > and files in order to rely on the recursive cleanup of the "root" directory, it
>> > should be covered explicitly by the API. I.e. (sub-)directories and files should
>> > have some kind of keep() and / or forget() method, to make it clear that this is
>> > supported and by design and won't lead to any leaks.
>> >
>> > Consequently, this would mean that we'd need something like your proposed const
>> > generic on the Dir type, such that keep() / forget() cannot be called on the
>> > "root" directory.
>> >
>> > However, I really think we should keep the code as it is in this version and
>> > just don't provide an example that utilizes ManuallyDrop and forget().
>> >
>> > I don't see how the idea of "manually dropping" (sub-)directories and files
>> > provides any real value compared to just storing their instance in a driver
>> > structure as long as they should stay alive, which is much more intuitive
>> > anyways.
>>
>> Yeah that's whats normally done in Rust anyways. But I think that
>> lifetimes bite us in this case:
>>
>> let debugfs: Dir<'static> = Dir::new(cstr!("sample_debugfs"));
>>
>> let sub: Dir<'a> = debugfs.subdir(cstr!("subdir"));
>> // lifetime `'a` starts in the line above and `sub` borrows `debugfs`
>>
>> /* code for creating the file etc */
>>
>> Ok(Self { _debugfs: debugfs, _sub: sub })
>> // lifetime `'a` has to end in the line above, since debugfs is moved but `sub` still borrows from it!
>>
>> This code won't compile, since we can't store the "root" dir in the same
>> struct that we want to store the subdir, because the subdir borrows from
>> the root dir.
>>
>> Essentially this would require self-referential structs like the
>> `ouroboros` crate [1] from user-space Rust. We should rather have the
>> `.keep()` function in the API than use self-referential structs.
>
> Fair enough -- I think we should properly document those limitations, recommend
> using keep() for those cases and ensure that we can't call keep() on the "root"
> directory then.
>
> Unless, we can find a better solution, which, unfortunately, I can't think of
> one. The only thing I can think of is to reference count (parent) directories,
> which would be contrary to how the C API works and not desirable.
Yeah, I also don't have an idea, but if I find something, I'll let you
know.
>> [1]: https://docs.rs/ouroboros/latest/ouroboros/attr.self_referencing.html
>>
>> Another problem that only affects complicated debugfs structures is that
>> you would have to store all subdirs & files somewhere. If the structure
>> is dynamic and changes over the lifetime of the driver, then you'll need
>> a `Vec` or store the dirs in `Arc` or similar, leading to extra
>> allocations.
>
> If it changes dynamically then it's pretty likely that we do not only want to
> add entries dynamically, but also remove them, which implies that we need to be
> able to drop them. So, I don't think that's a problem.
Yeah that's true.
> What I see more likely to happen is a situation where the "root" directory
> (almost) lives forever, and hence subsequent calls, such as
>
> root.subdir("foo").keep()
>
> effectively are leaks.
>
> One specific example for that would be usb_debug_root, which is created in the
> module scope of usb-common and is used by USB host / gadget / phy drivers.
>
> The same is true for other cases where the debugfs "root" is created in the
> module scope, but subsequent entries are created by driver instances. If a
> driver would use keep() in such a case, we'd effectively leak memory everytime a
> device is unplugged (or unbound in general).
Hmm that is unfortunate. But I don't see a problem with having:
static USB_DEBUGFS: Dir<'static> = ...; // or some on-demand init process
Then users can store subdir that also is `Dir<'static>` and just borrow
the USB_DEBUGFS for `'static`.
The docs on `keep` should definitely warn about leaks.
---
Cheers,
Benno
On Wed, May 14, 2025 at 02:21:41PM +0200, Benno Lossin wrote: > The docs on `keep` should definitely warn about leaks. Sounds like it could be a good candidate for `FORGET` comments as discussed in [1]. :-) [1] https://hackmd.io/@rust-for-linux-/HyJipr_3Jl
© 2016 - 2025 Red Hat, Inc.