Joel Fernandes
2025-Dec-01 20:32 UTC
[PATCH v3] rust: clist: Add support to interface with C linked lists
Hi John, On 11/30/2025 7:34 PM, John Hubbard wrote:> On 11/29/25 1:30 PM, Joel Fernandes wrote: >> Add a new module `clist` for working with C's doubly circular linked >> lists. Provide low-level iteration over list_head nodes and high-level >> iteration over typed list items. > ... >> >> MAINTAINERS | 7 + >> rust/helpers/helpers.c | 1 + >> rust/helpers/list.c | 12 ++ >> rust/kernel/clist.rs | 349 +++++++++++++++++++++++++++++++++++++++++ >> rust/kernel/lib.rs | 1 + >> 5 files changed, 370 insertions(+) >> create mode 100644 rust/helpers/list.c >> create mode 100644 rust/kernel/clist.rs > > Hi Joel, > > This is sufficiently tricky that I think it needs some code to exercise it. > > Lately I'm not sure what to recommend, there are several choices, each with > trade-offs: kunit, samples/rust, or even new DRM Rust code. Maybe the last > one is especially nice, because it doesn't really have many downsides. > > Rather than wait for any of that, I wrote a quick samples/rust/rust_clist.rs > and used it to sanity check my review findings, which are below.In v1, I had a samples/rust/ patch, but everyone's opinion almost unanimously was this does not belong in a sample, but rather in doctests. What in the sample is not supported by the current doctest? If something is missing, I think I can add it in. Plus yes, DRM_BUDDY is going to be a consumer shortly.>> diff --git a/MAINTAINERS b/MAINTAINERS >> index 5f7aa6a8a9a1..fb2ff877b6d1 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -22666,6 +22666,13 @@ F: rust/kernel/init.rs >> F: rust/pin-init/ >> K: \bpin-init\b|pin_init\b|PinInit >> >> +RUST TO C LIST INTERFACES >> +M: Joel Fernandes <joelagnelf at nvidia.com> >> +M: Alexandre Courbot <acourbot at nvidia.com> >> +L: rust-for-linux at vger.kernel.org >> +S: Maintained >> +F: rust/kernel/clist.rs >> + >> RXRPC SOCKETS (AF_RXRPC) >> M: David Howells <dhowells at redhat.com> >> M: Marc Dionne <marc.dionne at auristor.com> >> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c >> index 79c72762ad9c..634fa2386bbb 100644 >> --- a/rust/helpers/helpers.c >> +++ b/rust/helpers/helpers.c >> @@ -32,6 +32,7 @@ >> #include "io.c" >> #include "jump_label.c" >> #include "kunit.c" >> +#include "list.c" >> #include "maple_tree.c" >> #include "mm.c" >> #include "mutex.c" >> diff --git a/rust/helpers/list.c b/rust/helpers/list.c >> new file mode 100644 >> index 000000000000..6044979c7a2e >> --- /dev/null >> +++ b/rust/helpers/list.c >> @@ -0,0 +1,12 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +/* >> + * Helpers for C Circular doubly linked list implementation. > > s/Circular/circular/ > > ...but: > >> + */ >> + >> +#include <linux/list.h> >> + >> +void rust_helper_list_add_tail(struct list_head *new, struct list_head *head) >> +{ >> + list_add_tail(new, head); >> +} > > This is, so far, not used. Let's remove it, until/unless you have some > code in this patch(set) to use it, please.Did you try to remove it and build the doctest? :) CC rust/doctests_kernel_generated_kunit.o error[E0425]: cannot find function `list_add_tail` in crate `bindings` --> rust/doctests_kernel_generated.rs:3619:19 | 3619 | bindings::list_add_tail(&mut (*ptr).link, head);>> diff --git a/rust/kernel/clist.rs b/rust/kernel/clist.rs >> new file mode 100644 >> index 000000000000..361a6132299b >> --- /dev/null >> +++ b/rust/kernel/clist.rs >> @@ -0,0 +1,349 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +//! A C doubly circular intrusive linked list interface for rust code. >> +//! >> +//! # Examples >> +//! >> +//! ``` >> +//! use kernel::{ >> +//! bindings, >> +//! clist::init_list_head, >> +//! clist_create, >> +//! types::Opaque, // >> +//! }; >> +//! # // Create test list with values (0, 10, 20) - normally done by C code but it is >> +//! # // emulated here for doctests using the C bindings. >> +//! # use core::mem::MaybeUninit; >> +//! # >> +//! # /// C struct with embedded `list_head` (typically will be allocated by C code). >> +//! # #[repr(C)] >> +//! # pub(crate) struct SampleItemC { >> +//! # pub value: i32, >> +//! # pub link: bindings::list_head, >> +//! # } >> +//! # >> +//! # let mut head = MaybeUninit::<bindings::list_head>::uninit(); >> +//! # >> +//! # // SAFETY: head and all the items are test objects allocated in this scope. >> +//! # unsafe { init_list_head(head.as_mut_ptr()) }; >> +//! # // SAFETY: head is a test object allocated in this scope. >> +//! # let mut head = unsafe { head.assume_init() }; > > This is a bug that leads to a corrupted list. I have the test code (and > the kernel hang/crash) to prove it.Good find, actually it is a bug only in the example (the list construction in your sample should be coming from C code, not rust - this code here is just for doctest setup). That said, see below for fix.> The problem is that any move after init_list_head() invalidates the > list: the next/prev pointers still point to the old address. > > The fix requires two-step initialization, like this, for example:It has nothing to do with 2-step initialization. The issue is only related to the HEAD (and not the items) right? The issue is `assume_init()` should not be used on self-referential structures, the fix just one line: -//! # unsafe { init_list_head(head.as_mut_ptr()) }; -//! # let mut head = unsafe { head.assume_init() }; +//! # let head = head.as_mut_ptr(); +//! # unsafe { init_list_head(head) }; Does that fix the issue in your private sample test too? Or did I miss what you're suggesting?> > //! # // Two-step init: create uninit first (can be moved), then init after. > //! # let mut head = MaybeUninit::<bindings::list_head>::uninit(); > //! # let mut items = [ > //! # MaybeUninit::<SampleItemC>::uninit(), > //! # MaybeUninit::<SampleItemC>::uninit(), > //! # MaybeUninit::<SampleItemC>::uninit(), > //! # ]; > //! # > //! # // Step 2: Now init after they're in their final location > //! # // SAFETY: head is in its final stack location. > //! # unsafe { init_list_head(head.as_mut_ptr()) };Until the items are added, the items have nothing to do with the head. So I am not sure why you have to order it this way.> >> +//! # let mut items = [ >> +//! # MaybeUninit::<SampleItemC>::uninit(), >> +//! # MaybeUninit::<SampleItemC>::uninit(), >> +//! # MaybeUninit::<SampleItemC>::uninit(), >> +//! # ]; >> +//! # >> +//! # for (i, item) in items.iter_mut().enumerate() { >> +//! # let ptr = item.as_mut_ptr(); >> +//! # // SAFETY: pointers are to allocated test objects with a list_head field. >> +//! # unsafe { >> +//! # (*ptr).value = i as i32 * 10; >> +//! # // addr_of_mut!() computes address of link directly as link is uninitialized. >> +//! # init_list_head(core::ptr::addr_of_mut!((*ptr).link)); >> +//! # bindings::list_add_tail(&mut (*ptr).link, &mut head); >> +//! # } >> +//! # } >> +//! >> +//! // Rust wrapper for the C struct. >> +//! // The list item struct in this example is defined in C code as: >> +//! // struct SampleItemC { >> +//! // int value; >> +//! // struct list_head link; >> +//! // }; >> +//! // >> +//! #[repr(transparent)] >> +//! pub(crate) struct Item(Opaque<SampleItemC>); >> +//! >> +//! impl Item { >> +//! pub(crate) fn value(&self) -> i32 { >> +//! // SAFETY: `Item` has same layout as `SampleItemC`. >> +//! unsafe { (*self.0.get()).value } >> +//! } >> +//! } >> +//! >> +//! // Create typed Clist from sentinel head. >> +//! // SAFETY: head is valid, items are `SampleItemC` with embedded `link` field. >> +//! let list = unsafe { clist_create!(&mut head, Item, SampleItemC, link) }; >> +//! >> +//! // Iterate directly over typed items. >> +//! let mut found_0 = false; >> +//! let mut found_10 = false; >> +//! let mut found_20 = false; >> +//! >> +//! for item in list.iter() { >> +//! let val = item.value(); >> +//! if val == 0 { found_0 = true; } >> +//! if val == 10 { found_10 = true; } >> +//! if val == 20 { found_20 = true; } >> +//! } >> +//! >> +//! assert!(found_0 && found_10 && found_20); >> +//! ``` >> +[...]>> +impl<'a> Iterator for ClistHeadIter<'a> { >> + type Item = &'a ClistHead; >> + >> + #[inline] >> + fn next(&mut self) -> Option<Self::Item> { >> + if self.exhausted { >> + return None; >> + } >> + >> + // Advance to next node. >> + self.current_head = self.current_head.next(); >> + >> + // Check if we've circled back to the sentinel head. >> + if self.current_head == self.list_head { >> + self.exhausted = true; >> + return None; >> + } >> + >> + Some(self.current_head) >> + } >> +} >> + >> +impl<'a> FusedIterator for ClistHeadIter<'a> {} >> + >> +/// A typed C linked list with a sentinel head. >> +/// >> +/// A sentinel head represents the entire linked list and can be used for >> +/// iteration over items of type `T`, it is not associated with a specific item. >> +/// >> +/// # Invariants >> +/// >> +/// - `head` is an allocated and valid C `list_head` structure that is the list's sentinel. >> +/// - `offset` is the byte offset of the `list_head` field within the C struct that `T` wraps. >> +/// >> +/// # Safety >> +/// >> +/// - All the list's `list_head` nodes must be allocated and have valid next/prev pointers. >> +/// - The underlying `list_head` (and entire list) must not be modified by C for the >> +/// lifetime 'a of `Clist`. >> +pub struct Clist<'a, T> { >> + head: &'a ClistHead, >> + offset: usize, >> + _phantom: PhantomData<&'a T>, >> +} > > This discards build-time (const generic) information, and demotes it to > runtime (.offset), without any real benefit. I believe it's better to keep > it as a const generic, like this: > > pub struct Clist<'a, T, const OFFSET: usize> { > head: &'a ClistHead, > _phantom: PhantomData<&'a T>, > } > >> + >> +impl<'a, T> Clist<'a, T> { > > And here, the above becomes: > > impl<'a, T, const OFFSET: usize> Clist<'a, T, OFFSET> { > > ...etc.It is not ignored, the const-generic part only applies to the constructor method in my patch. I didn't want to add another argument to the diamond brackets, the type name looks really ugly then. The only advantage I think of doing this (inspite of the obvious aesthetic disadvantage) is that a mutable `Clist` cannot have its offset modified. Let me see if I can get Alice's suggestion to make it a const in the struct work to solve that. [..]>> +impl<'a, T> FusedIterator for ClistIter<'a, T> {} >> + >> +/// Create a C doubly-circular linked list interface `Clist` from a raw `list_head` pointer. >> +/// >> +/// This macro creates a `Clist<T>` that can iterate over items of type `$rust_type` linked >> +/// via the `$field` field in the underlying C struct `$c_type`. >> +/// >> +/// # Arguments >> +/// >> +/// - `$head`: Raw pointer to the sentinel `list_head` object (`*mut bindings::list_head`). >> +/// - `$rust_type`: Each item's rust wrapper type. >> +/// - `$c_type`: Each item's C struct type that contains the embedded `list_head`. >> +/// - `$field`: The name of the `list_head` field within the C struct. >> +/// >> +/// # Safety >> +/// >> +/// The caller must ensure: >> +/// - `$head` is a valid, initialized sentinel `list_head` pointing to a list that remains >> +/// unmodified for the lifetime of the rust `Clist`. >> +/// - The list contains items of type `$c_type` linked via an embedded `$field`. >> +/// - `$rust_type` is `#[repr(transparent)]` over `$c_type` or has compatible layout. >> +/// - The macro is called from an unsafe block. >> +/// >> +/// # Examples >> +/// >> +/// Refer to the examples in the [crate::clist] module documentation. >> +#[macro_export] >> +macro_rules! clist_create { >> + ($head:expr, $rust_type:ty, $c_type:ty, $field:ident) => { >> + $crate::clist::Clist::<$rust_type>::from_raw_and_offset::< >> + { ::core::mem::offset_of!($c_type, $field) }, >> + >($head) >> + }; >> +} > > Unlike the corresponding C container_of() macro, this one here has no > compile-time verification that the field is actually a list_head. > > How about this, to add that check: > > --- a/rust/kernel/clist.rs > +++ b/rust/kernel/clist.rs > macro_rules! clist_create { > - ($head:expr, $rust_type:ty, $c_type:ty, $field:ident) => { > - $crate::clist::Clist::<$rust_type>::from_raw_and_offset::< > + ($head:expr, $rust_type:ty, $c_type:ty, $field:ident) => {{ > + // Compile-time check: $field must be a list_head. > + const _: () = { > + let _check: fn(*const $c_type) -> *const $crate::bindings::list_head > + |p| unsafe { ::core::ptr::addr_of!((*p).$field) }; > + }; > + $crate::clist::Clist::<$rust_type, { ::core::mem::offset_of!($c_type, $field) }>::from_raw( > $head, > ) > - }; > + }};Sure I will play with your suggested snippet and add that, thanks. - Joel
Joel Fernandes
2025-Dec-01 20:57 UTC
[PATCH v3] rust: clist: Add support to interface with C linked lists
On 12/1/2025 3:32 PM, Joel Fernandes wrote:> On 11/30/2025 7:34 PM, John Hubbard wrote:[...]>>> +/// Refer to the examples in the [crate::clist] module documentation. >>> +#[macro_export] >>> +macro_rules! clist_create { >>> + ($head:expr, $rust_type:ty, $c_type:ty, $field:ident) => { >>> + $crate::clist::Clist::<$rust_type>::from_raw_and_offset::< >>> + { ::core::mem::offset_of!($c_type, $field) }, >>> + >($head) >>> + }; >>> +} >> >> Unlike the corresponding C container_of() macro, this one here has no >> compile-time verification that the field is actually a list_head. >> >> How about this, to add that check: >> >> --- a/rust/kernel/clist.rs >> +++ b/rust/kernel/clist.rs >> macro_rules! clist_create { >> - ($head:expr, $rust_type:ty, $c_type:ty, $field:ident) => { >> - $crate::clist::Clist::<$rust_type>::from_raw_and_offset::< >> + ($head:expr, $rust_type:ty, $c_type:ty, $field:ident) => {{ >> + // Compile-time check: $field must be a list_head. >> + const _: () = { >> + let _check: fn(*const $c_type) -> *const $crate::bindings::list_head >> + |p| unsafe { ::core::ptr::addr_of!((*p).$field) }; >> + }; >> + $crate::clist::Clist::<$rust_type, { ::core::mem::offset_of!($c_type, $field) }>::from_raw( >> $head, >> ) >> - }; >> + }}; > > Sure I will play with your suggested snippet and add that, thanks.I think it works also without the 'const _:' block: let _: fn(*const $c_type) -> *const $crate::bindings::list_head |p| unsafe { ::core::ptr::addr_of!((*p).$field) }; I will roll that in, thanks John. - Joel
John Hubbard
2025-Dec-01 22:17 UTC
[PATCH v3] rust: clist: Add support to interface with C linked lists
On 12/1/25 12:32 PM, Joel Fernandes wrote:> On 11/30/2025 7:34 PM, John Hubbard wrote: >> On 11/29/25 1:30 PM, Joel Fernandes wrote:...>> This is sufficiently tricky that I think it needs some code to exercise it. >> >> Lately I'm not sure what to recommend, there are several choices, each with >> trade-offs: kunit, samples/rust, or even new DRM Rust code. Maybe the last >> one is especially nice, because it doesn't really have many downsides. >> >> Rather than wait for any of that, I wrote a quick samples/rust/rust_clist.rs >> and used it to sanity check my review findings, which are below. > > In v1, I had a samples/rust/ patch, but everyone's opinion almost unanimously > was this does not belong in a sample, but rather in doctests. What in the sample > is not supported by the current doctest? If something is missing, I think I can > add it in. Plus yes, DRM_BUDDY is going to be a consumer shortly.Well, I won't contest the choice of doctests, since wiser heads than mine have already worked through the tradeoffs. But for API developers, the problem with doctests is that no one has ever actually *run* the code. It's just a build test. And so critical bugs, such as the kernel crash/hang below, are missed. I would humbly suggest that you build and *run* your own samples code, for new code that has no users yet. Because if you are skipping steps like this (posting the code before there is an actual caller), then the documentation of how to use it is not "just documentation" anymore--it really needs to run correctly. And actually, after writing the above...I still think it would be better to post this with its first caller (DRM_BUDDY, or BUDDY_DRM_ALUMNI, or however it ends up), so that we can see how it looks and behaves in practice. What's the rush? ...>> The fix requires two-step initialization, like this, for example: > > It has nothing to do with 2-step initialization. The issue is only related to > the HEAD (and not the items) right? The issue is `assume_init()` should not be > used on self-referential structures, the fix just one line: > > -//! # unsafe { init_list_head(head.as_mut_ptr()) }; > -//! # let mut head = unsafe { head.assume_init() }; > > +//! # let head = head.as_mut_ptr(); > +//! # unsafe { init_list_head(head) }; > > Does that fix the issue in your private sample test too? > > Or did I miss what you're suggesting? >Yes, you are correct: the main point is to avoid moving a struct that contains self-referential fields. So your version is a more accurate and better fix. ...>>> +pub struct Clist<'a, T> { >>> + head: &'a ClistHead, >>> + offset: usize, >>> + _phantom: PhantomData<&'a T>, >>> +} >> >> This discards build-time (const generic) information, and demotes it to >> runtime (.offset), without any real benefit. I believe it's better to keep >> it as a const generic, like this: >> >> pub struct Clist<'a, T, const OFFSET: usize> { >> head: &'a ClistHead, >> _phantom: PhantomData<&'a T>, >> } >> >>> + >>> +impl<'a, T> Clist<'a, T> { >> >> And here, the above becomes: >> >> impl<'a, T, const OFFSET: usize> Clist<'a, T, OFFSET> { >> >> ...etc. > > It is not ignored, the const-generic part only applies to the constructor method > in my patch. I didn't want to add another argument to the diamond brackets, the > type name looks really ugly then. >The macro hides it, though. Users never have to write the full type. Increasing const-ness is worth something. The messy syntax is unfortunate, but I don't really know what to say there.> The only advantage I think of doing this (inspite of the obvious aesthetic > disadvantage) is that a mutable `Clist` cannot have its offset modified. Let me > see if I can get Alice's suggestion to make it a const in the struct work to > solve that.Yes. I have it working locally, so I'm confident that you will prevail. :) thanks, -- John Hubbard