linux-kernel - Re: [PATCH 2/4] rust: scatterlist: Add type-state 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] [day] [month] [year] [list]

Message-Id: <DC7HI4RCMJWG.12GHJUGAYZ3TA@kernel.org>
Date: Wed, 20 Aug 2025 20:59:10 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: <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>,
 <acourbot@...dia.com>, <jgg@...pe.ca>, <lyude@...hat.com>,
 <robin.murphy@....com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for
 sg_table

On Wed Aug 20, 2025 at 7:08 PM CEST, Daniel Almeida wrote:
>> +/// A scatter-gather table.
>> +///
>> +/// This struct is a wrapper around the kernel's `struct sg_table`. It manages a list of DMA-mapped
>> +/// memory segments that can be passed to a device for I/O operations.
>> +///
>> +/// The generic parameter `T` is used as a type state to distinguish between owned and borrowed
>> +/// tables.
>> +///
>> +///  - [`SGTable<Owned>`]: An owned table created and managed entirely by Rust code. It handles
>> +///    allocation, DMA mapping, and cleanup of all associated resources. See [`SGTable::new`].
>> +///  - [`SGTable<Borrowed>`} (or simply [`SGTable`]): Represents a table whose lifetime is managed
>> +///    externally. It can be used safely via a borrowed reference `&'a SGTable`, where `'a` is the
>> +///    external lifetime.
>> +///
>> +/// All [`SGTable`] variants can be iterated over the individual [`SGEntry`]s.
>> +#[repr(transparent)]
>> +#[pin_data]
>> +pub struct SGTable<T: private::Sealed = Borrowed> {
>
> Am I the only one that think we should have an actual trait here instead of
> using private::Sealed directly?

I don't know. :)

I think this case perfectly fits the Sealed pattern. There isn't any semantics
behind that, other than "we want that only the sealed ones work".

>
>> +    #[pin]
>> +    inner: T,
>> +}
>> +
>> +impl SGTable {
>> +    /// Creates a borrowed `&'a SGTable` from a raw `struct sg_table` pointer.
>> +    ///
>> +    /// This allows safe access to an `sg_table` that is managed elsewhere (for example, in C code).
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is valid for the entire
>> +    /// lifetime of `'a`.
>> +    pub unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
>> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
>> +        // to a `struct sg_table` for the duration of `'a`.
>> +        unsafe { &*ptr.cast() }
>> +    }
>> +
>> +    fn as_raw(&self) -> *mut bindings::sg_table {
>> +        self.inner.0.get()
>> +    }
>> +
>> +    fn as_iter(&self) -> SGTableIter<'_> {
>
> Perhaps just "iter()” ?

No strong opinition, I'm fine with either.

>
>> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct sg_table`.
>> +        let ptr = unsafe { (*self.as_raw()).sgl };
>> +
>> +        // SAFETY: `ptr` is guaranteed to be a valid pointer to a `struct scatterlist`.
>> +        let pos = Some(unsafe { SGEntry::as_ref(ptr) });
>> +
>> +        SGTableIter { pos }
>> +    }
>> +}
>> +
>> +/// # Invariants
>> +///
>> +/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>> +struct DmaMapSgt {
>
> This is private, but some extra docs here wouldn’t hurt?

The type just represents the DMA mapping state of the SGT, i.e. exists
corresponds to is mapped, is dropped corresponds to is unmapped.

I can probably add a few lines if it helps.

>> +impl<P> Owned<P>
>> +where
>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>> +{
>> +    fn new(
>> +        dev: &Device<Bound>,
>> +        mut pages: P,
>> +        dir: dma::DataDirection,
>> +        flags: alloc::Flags,
>> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {
>
> I confess I have no idea what “use<‘_, P>” is.

It's a feature called #![feature(precise_capturing)], which, unfortunately, does
not exist in 1.78, so I removed this syntax in v2. Luckly, we don't actually
need it in this case.