linux-kernel - Re: [PATCH v6 1/3] rust: configfs: introduce rust support for configfs

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: <CANiq72mS_HV5rDjP+t+k0jX9QeAgF2SB9_xX54iEBTH-GoPuEg@mail.gmail.com>
Date: Thu, 1 May 2025 12:52:10 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Danilo Krummrich <dakr@...nel.org>, 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>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Joel Becker <jlbec@...lplan.org>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>, 
	Waiman Long <longman@...hat.com>, Fiona Behrens <me@...enk.dev>, 
	Charalampos Mitrodimas <charmitro@...teo.net>, Daniel Almeida <daniel.almeida@...labora.com>, 
	Breno Leitao <leitao@...ian.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/3] rust: configfs: introduce rust support for configfs

On Thu, May 1, 2025 at 12:15 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>
> ---
>

Spurious newlines.

> This patch is a direct dependency for `rnull`, the rust null block driver.
> ---

By the way, you don't need this `---`.

> +//! `configfs` interface.

I don't know if configfs is supposed to be written in code spans, but
I appreciate you are trying to be throughout in your Markdown use ;)
It may be easier to read to not have it in code spans, since we have
many already and it is not code anyway.

By the way, you may want to mention somehow the title they use in
their docs: "Userspace-driven Kernel Object Configuration".

> +//! See the [rust_configfs.rs] sample for a full example use of this module.

Files are, though, like the C header below, so: [`rust_configfs.rs`]

> +/// with configfs, embed a field of this type into your kernel module struct.

Either with or without a code span, i.e. being consistent is best.

> +    /// Return the address of the `bindings::config_group` embedded in `Self`.

I think you may be able to use intra-doc links for [`Self`].

> +        let c_group: *mut bindings::config_group =
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +            unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();

It doesn't work to put the safety comment on top? (We had issues
similar to this in the past, so if it is intentional, that is fine).

> +/// This type is constructed statically at compile time and is by the
> +/// [`kernel::configfs_attrs`] macro.

Sentence is missing something. Also, we never used `# Note` yet, but I
guess it is fine.

> +    /// Null terminated Array of pointers to `Attribute`. The type is `c_void`

Intar-doc link(s)?

> +        // We need a space at the end of our list for a null terminator.
> +        if I >= N - 1 {
> +            kernel::build_error!("Invalid attribute index");
> +        }

Would the following work instead?

    const { assert!(I < N - 1, "Invalid attribute index") };

(Please double-check it actually catches the cases you need)

Thanks!

Cheers,
Miguel