linux-kernel - Re: [PATCH 5/8] rust: debugfs: Support format hooks

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: <CANiq72=vM9Zr-q=BWvE258=9BV1Q4S_9sYo+gfCnaUUj_E09ow@mail.gmail.com>
Date: Wed, 30 Apr 2025 05:17:33 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Matthew Maurer <mmaurer@...gle.com>
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>, Greg Kroah-Hartman <gregkh@...uxfoundation.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 5/8] rust: debugfs: Support format hooks

On Wed, Apr 30, 2025 at 1:16 AM Matthew Maurer <mmaurer@...gle.com> wrote:
>
> +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking `f`
> +    /// on the provided reference. `f` must be a function item or a non-capturing closure, or this
> +    /// will fail to compile.

The first paragraph of docs are the "title" ("short description"),
which renders differently. Typically it should be kept short, e.g. a
sentence. So I would probably move the second sentence to a second
paragraph.

In any case, is it true that it will fail to compile? Please see below.

> +    /// # Example

We use plurals for these sections.

> +    ) -> Result<()> {

Can be `Result`.

> +        // We forget the reference because its reference count is implicitly "owned" by the root
> +        // builder, which we know will use `debugfs_remove` to clean this up. If we release the
> +        // file here, it will be immediately deleted.
> +        // SAFETY:
> +        // Because `Builder`'s invariant says that our lifetime is how long the directory will

In our usual style, this would be e.g.:

    // file here, it will be immediately deleted.
    //
    // SAFETY: Because `Builder`'s ...

> +// INVARIANT: F is inhabited
> +#[repr(transparent)]
> +struct FormatAdapter<T, F> {

For invariants, we put them in the docs, as a section, e.g.

    /// # Invariants
    ///
    /// `F` is inhabited.

> +        // SAFETY: FormatAdapater is a repr(transparent) wrapper around T, so
> +        // casting a reference is legal

Please use Markdown in comments too.

> +/// # Safety
> +/// The caller asserts that F is inhabited
> +unsafe fn materialize_zst_fmt<F>() -> &'static F {

    /// # Safety
    ///
    /// `F` must be inhabited.

(I simplified the wording)

> +    // We don't have generic_const_exprs, and const items inside the function get promoted out and
> +    // lose type variables, so we need to do the old-style assert to check for ZSTness
> +    [(); 1][core::mem::size_of::<F>()];

Shouldn't this be:

    const { assert!(core::mem::size_of::<F>() == 0) };

to prevent a runtime panic?

Or `build_assert!` otherwise, but the line above catches the issue if
I try to pass a capturing closure from the sample module.

Thanks!

Cheers,
Miguel