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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGSQo00Kg2QV56LYFg6nRY+yS2KtiVAPaggKaKFCdprjBfXCcA@mail.gmail.com>
Date: Wed, 30 Apr 2025 08:10:44 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Sami Tolvanen <samitolvanen@...gle.com>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.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@...gle.com>
>
> First off, many thanks for doing this.  I like this in general, but I
> have loads of specific questions/comments.  Don't take that as a
> criticism of this feature, I really want these bindings to be in the
> tree and work hopefully better/cleaner than the userspace ones do.
>
> First off, the main "rule" of debugfs is that you should NEVER care
> about the return value of any debugfs function.  So much so that the C
> side hides errors almost entirely where possible.  I'd like to see this
> carried through to the Rust side as well, but I think you didn't do that
> for various "traditional" reasons.

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

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

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

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

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