linux-kernel - Re: [PATCH 1/2] rust: add initial scatterlist bindings

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: <DA9KQF9CY77R.77PBMU8Y1FPY@nvidia.com>
Date: Fri, 30 May 2025 23:44:26 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Jason Gunthorpe" <jgg@...pe.ca>
Cc: "Abdiel Janulgue" <abdiel.janulgue@...il.com>, <dakr@...nel.org>,
 <lyude@...hat.com>, "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>, "Valentin Obst"
 <kernel@...entinobst.de>, "open list" <linux-kernel@...r.kernel.org>,
 "Marek Szyprowski" <m.szyprowski@...sung.com>, "Robin Murphy"
 <robin.murphy@....com>, <airlied@...hat.com>,
 <rust-for-linux@...r.kernel.org>, "open list:DMA MAPPING HELPERS"
 <iommu@...ts.linux.dev>, "Petr Tesarik" <petr@...arici.cz>, "Andrew Morton"
 <akpm@...ux-foundation.org>, "Herbert Xu" <herbert@...dor.apana.org.au>,
 "Sui Jingfeng" <sui.jingfeng@...ux.dev>, "Randy Dunlap"
 <rdunlap@...radead.org>, "Michael Kelley" <mhklinux@...look.com>
Subject: Re: [PATCH 1/2] rust: add initial scatterlist bindings

On Fri May 30, 2025 at 11:14 PM JST, Jason Gunthorpe wrote:
> On Fri, May 30, 2025 at 11:02:02PM +0900, Alexandre Courbot wrote:
>> You would have a trait for providing the pages and their range:
>> 
>>     /// Provides a list of pages that can be used to build a `SGTable`.
>>     trait SGTablePages {
>>         /// Returns an iterator to the pages providing the backing memory of `self`.
>>         fn pages_iter<'a>(&'a self) -> impl Iterator<Item = &'a bindings::page>;
>>         /// Returns the effective range of the mapping.
>>         fn range(&self) -> Range<usize>;
>>     }
>>
>> The `SGTable` becomes something like:
>> 
>>     struct SGTable<P: SGTablePages, T: MapState>
>>     {
>>         table: Opaque<bindings::sg_table>,
>>         pages: P,
>>         _s: PhantomData<T>,
>>     }
>
> At this point it isn't exactly a sgtable anymore, it is some rust
> specific way to get a dma mapped scatterlist. Most of the actual ways
> to use a sgtable's cpu side would become unavailable for safety
> reasons.
>
> That seems fine to me, and is what I was suggesting when I said not to
> expose set_page at all.
>
> But I would maybe lean into it a bit more, why have the type state at
> all anymore if the flow is SGTablePages -> SgTable -> Dma Mapped?
> There isn't really a reason to expose the CPU populated but not yet
> mapped state to the user at all. They can't do anything with it. Just
> directly create the DMA mapped scatterlist and only expose the DMA
> list through the rust API in a single step.
>
> So much simpler to understand and doesn't leak the bad decisions of
> the scatterlist design.

I would be fully on board with a simpler design, definitely. The reason
why I've tried to keep some doors open is that as you mentioned
scatterlist is used in many different ways, and I am not familiar enough
with all these uses to draw a line and say "we will never ever need to
do that".

Like unmapping a buffer and remapping it later sounds like a plausible
use (say, if the device's own DMA space is limited), so preserving at
least 2 states sounds sensible.

> Certainly the initial uses of scatterlist don't need to ever know
> about or touch the CPU side of the scatterlist, and it would be great
> if Rust could stay that way..

Yeah I am also more and more convinced we don't need to expose that part
and should just write it at initialization time and never touch it
afterwards.