Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

Matthew Maurer
2025-04-30T15:10:58Z

On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> > is delayed until the more full-featured API, because we need to avoid
> > the user having an reference to a dir that is recursively removed.
> >
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
>
> First off, many thanks for doing this. I like this in general, but I
> have loads of specific questions/comments. Don't take that as a
> criticism of this feature, I really want these bindings to be in the
> tree and work hopefully better/cleaner than the userspace ones do.
>
> First off, the main "rule" of debugfs is that you should NEVER care
> about the return value of any debugfs function. So much so that the C
> side hides errors almost entirely where possible. I'd like to see this
> carried through to the Rust side as well, but I think you didn't do that
> for various "traditional" reasons.

Sure, I mostly had to do error checking because I was using an
`ARef ` to represent a directory, which meant that the underlying
directory needed to be a valid pointer. Given that you've said that
the returned `dentry *` should never be used as an actual `dentry`,
only as an abstract handle to a DebugFS object, that requirement goes
away, and I can remove the error handling code and always successfully
return a `Dir`, even in cases where an error has occurred.

>
> Wait, what? Why would an explicit drop(parent) be required here? That
> feels wrong, and way too complex. Why can't you rely on the child
> creation to properly manage this if needed (hint, it shouldn't be.)

The explicit `drop` is not required for normal usage, it was intended
to be illustrative - I was trying to explain what the semantics would
be if the parent `dentry` was released before the child was. As
before, now that the child will not be an `ARef `, and so not
assumed to be a valid `dentry` pointer, this example won't be relevant
anymore.

>
> Why do you want this wrapped? And I think you "wrapped" it wrong here.
> When the object is "gone" it should have called
> debugfs_remove_recursive(), not dput(), in order to properly drop
> everything. Where is that call happening?
>
> The debugfs core already handles the reference counting of this object,
> please don't add some extra reference count calls with dput/get, that's
> just going to be a mess.
>
> You should NEVER be encoding the fact that the return value of a
> debugfs_*() call is a dentry. Just treat that as an opaque pointer that
> just happens to have a common name that might refer to something else.
> That pointer can be passed back into other debugfs functions, and THAT
> IS IT. No checking for it, no passing it to any vfs functions, or
> anything else.
>
> So the dput() stuff here should not be present at all (hint, no C code
> that used debugfs ever calls that, so the rust bindings shouldn't
> either.)
>

I incorrectly assumed that if a `dentry *` was being returned, then at
some point it would be used as a `dentry *` rather than as a
debugfs-only handle. I bound it into an `ARef` to avoid backing myself
into a corner later if someone needed to use the `dentry *` for
something, but if that's against the API contract of DebugFS, I can
avoid it entirely.