Re: [PATCH v10 5/8] rust: clist: Add support to interface with C linked lists

[Joel Fernandes <joelagnelf@nvidia.com>]

On Thu, Feb 19, 2026 at 12:21:56PM +0100, Danilo Krummrich wrote:
> On Wed Feb 18, 2026 at 9:55 PM CET, Joel Fernandes wrote:
> > +RUST TO C LIST INTERFACES
>
> Maybe this should just be "RUST [FFI]" instead (in case Alex and you want to
> sign up for looking after FFI helper infrastructure in general)?

Good idea, done.

> > +F:	rust/kernel/ffi/clist.rs
>
> <snip>
>
> > +//! This module provides Rust abstractions for iterating over C `list_head`-based
> > +//! linked lists. It is intended for FFI use-cases where a C subsystem manages a
> > +//! circular linked list that Rust code needs to read. This is generally required
> > +//! only for special cases and should be avoided by drivers.
>
> Maybe generalize the statement a bit and say that this should only be used for
> cases where C and Rust code share direct access to the same linked list through
> an FFI interface.
>
> Additionally, add a separate note that this *must not* be used by Rust
> components that just aim for a linked list primitive and instead refer to the
> Rust linked list implementation with an intra-doc link.

Done. Updated the module doc to say "It should only be used for cases
where C and Rust code share direct access to the same linked list
through an FFI interface" and added a separate note:

  Note: This *must not* be used by Rust components that just need a
  linked list primitive. Use [`kernel::list::List`] instead.

> > +//!     types::Opaque, //
>
> Examples don't necessarily need '//' at the end, as they are not automatically
> formatted anyways.

Removed from the example. Non-example imports keep the '//' as a
rustfmt guard.

> > +//! #     // SAFETY: pointers are to allocated test objects with a list_head field.
> > +//! #     unsafe {
>
> I understand that this is just setup code for a doc-test, but I still think we
> should hold it to the same standards, i.e. let's separate the different unsafe
> calls into their own unsafe blocks and add proper safety comments.

Done. Split into three separate unsafe blocks with individual SAFETY
comments for the value write, INIT_LIST_HEAD, and list_add_tail calls.

> > +    PinInit //
>
> Should be 'PinInit, //'.

Fixed.

> > +/// - [`CListHead`] represents an allocated and valid `list_head` structure.
>
> What does "allocated" mean in this context? (Dynamic allocations, stack, .data
> section of the binary, any of those?)
>
> In case of the latter, I'd just remove "allocated".

Removed "allocated". The invariant now reads:

  The underlying `list_head` has been initialized (e.g. via
  `INIT_LIST_HEAD()`) and its `next`/`prev` pointers are valid and
  non-NULL.

> > +    /// - `ptr` must be a valid pointer to an allocated and initialized `list_head` structure.
>
> Same here, what exactly is meant by "allocated"?

Removed "allocated" from from_raw() safety docs as well. Updated to:

  `ptr` must be a valid pointer to an initialized `list_head` (e.g.
  via `INIT_LIST_HEAD()`), with valid non-NULL `next`/`prev` pointers.

> > +    /// - The list and all linked `list_head` nodes must not be modified by non-Rust code
> > +    ///   for the lifetime `'a`.
>
> This is a bit vague I think, concurrent modifications of (other) Rust code are
> not OK either.

Fixed. Changed to "must not be concurrently modified for the lifetime
`'a`" which covers both Rust and C code.

> > +        // SAFETY:
> > +        // - `self.as_raw()` is valid per type invariants.
> > +        // - The `next` pointer is guaranteed to be non-NULL.
>
> I'm not sure whether "valid" in the type invariant implies that the struct
> list_head is initialized. From a language point of view it is also valid if the
> pointers are NULL.
>
> So, I think the invariant (and the safety requirements of from_raw()) have to
> ensure that the struct list_head is initialized in the sense of
> INIT_LIST_HEAD().

Agreed. The invariant and from_raw() safety requirements now explicitly
require INIT_LIST_HEAD() initialization with valid non-NULL next/prev
pointers. The next() SAFETY comment now reads:

  - `self.as_raw()` is valid and initialized per type invariants.
  - The `next` pointer is valid and non-NULL per type invariants
    (initialized via `INIT_LIST_HEAD()` or equivalent).

> > +/// - The [`CListHead`] is an allocated and valid sentinel C `list_head` structure.
> > +/// - `OFFSET` is the byte offset of the `list_head` field within the struct that `T` wraps.
> > +/// - All the list's `list_head` nodes are allocated and have valid next/prev pointers.
>
> Comments from CListHead also apply here.

Updated CList invariants and from_raw() safety docs to match the
CListHead pattern (removed "allocated", added INIT_LIST_HEAD, non-NULL
pointers, "concurrently modified").

> > +/// This is an unsafe macro. The caller must ensure:
>
> Given that, we should probably use the same (or a similar) trick as in [1].
>
> [1] https://rust.docs.kernel.org/src/kernel/device.rs.html#665-688

Done. Applied the device.rs pattern - the macro now requires
`clist_create!(unsafe { ... })` syntax, which forces callers to
acknowledge the safety requirements at the call site. The macro
internally wraps the `CList::from_raw` call in an unsafe block.

Thanks for the review!

Joel