linux-kernel - Re: [PATCH v3 3/5] rust: scatterlist: Add abstraction for sg

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: <DCCFOGR3BPVC.3OW6B74N372MB@nvidia.com>
Date: Tue, 26 Aug 2025 23:36:44 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>, <akpm@...ux-foundation.org>,
 <ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
 <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <abdiel.janulgue@...il.com>, <jgg@...pe.ca>, <lyude@...hat.com>,
 <robin.murphy@....com>, <daniel.almeida@...labora.com>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/5] rust: scatterlist: Add abstraction for sg_table

On Mon Aug 25, 2025 at 10:24 PM JST, Danilo Krummrich wrote:
> Add a safe Rust abstraction for the kernel's scatter-gather list
> facilities (`struct scatterlist` and `struct sg_table`).
>
> This commit introduces `SGTable<T>`, a wrapper that uses a generic
> parameter to provide compile-time guarantees about ownership and lifetime.
>
> The abstraction provides two primary states:
> - `SGTable<Owned<P>>`: Represents a table whose resources are fully
>   managed by Rust. It takes ownership of a page provider `P`, allocates
>   the underlying `struct sg_table`, maps it for DMA, and handles all
>   cleanup automatically upon drop. The DMA mapping's lifetime is tied to
>   the associated device using `Devres`, ensuring it is correctly unmapped
>   before the device is unbound.
> - `SGTable<Borrowed>` (or just `SGTable`): A zero-cost representation of
>   an externally managed `struct sg_table`. It is created from a raw
>   pointer using `SGTable::as_ref()` and provides a lifetime-bound
>   reference (`&'a SGTable`) for operations like iteration.
>
> The API exposes a safe iterator that yields `&SGEntry` references,
> allowing drivers to easily access the DMA address and length of each
> segment in the list.
>
> Co-developed-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>

A few minor things below, but:

Reviewed-by: Alexandre Courbot <acourbot@...dia.com>

Used successfully on nova-core:

Tested-by: Alexandre Courbot <acourbot@...dia.com>

I still see mentions of "type state" in the code (and the commit
message), is this on purpose? I still think this is a misleading use of
the term, but your call.

<snip>
> +impl SGEntry {
> +    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the
> +    /// lifetime `'a`.

Shouldn't the scatterlist also have valid a dma_address and dma_len?

<snip>
> +#[repr(transparent)]
> +#[pin_data(PinnedDrop)]
> +struct RawSGTable {

Even if this is for internal use, I think a short comment explaining
what this is for, and why it needs to be pinned (pointed to by devres)
would be helpful to people looking at this code.