linux-kernel - Re: [PATCH v2 3/5] 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] [thread-next>] [day] [month] [year] [list]

Message-Id: <DC8XLP6C3E5I.10QJQVI4LORSF@kernel.org>
Date: Fri, 22 Aug 2025 13:48:47 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.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>,
 <tmgross@...ch.edu>, <abdiel.janulgue@...il.com>, <acourbot@...dia.com>,
 <jgg@...pe.ca>, <lyude@...hat.com>, <robin.murphy@....com>,
 <daniel.almeida@...labora.com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction
 for sg_table

On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
>> +#[pinned_drop]
>> +impl PinnedDrop for RawSGTable {
>> +    #[inline]
>> +    fn drop(self: Pin<&mut Self>) {
>> +        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
>> +        unsafe { bindings::sg_free_table(self.sgt.get()) };
>
> It's weird that this is called free when the sg_table isn't freed by
> this call.

It frees the entries contained in the sg_table.

>> +    }
>> +}
>> +
>> +/// The [`Owned`] type state of an [`SGTable`].
>> +///
>> +/// A [`SGTable<Owned>`] signifies that the [`SGTable`] owns all associated resources:
>> +///
>> +/// - The backing memory pages.
>> +/// - The `struct sg_table` allocation (`sgt`).
>> +/// - The DMA mapping, managed through a [`Devres`]-managed `DmaMapSgt`.
>> +///
>> +/// Users interact with this type through the [`SGTable`] handle and do not need to manage
>> +/// [`Owned`] directly.
>> +#[pin_data]
>> +pub struct Owned<P> {
>> +    // Note: The drop order is relevant; we first have to unmap the `struct sg_table`, then free the
>> +    // `struct sg_table` and finally free the backing pages.
>> +    #[pin]
>> +    dma: Devres<DmaMapSgt>,
>> +    #[pin]
>> +    sgt: RawSGTable,
>> +    _pages: P,
>> +}
>> +
>> +// SAFETY: `Owned` can be send to any task if `P` can be send to any task.
>> +unsafe impl<P: Send> Send for Owned<P> {}
>> +
>> +// SAFETY: `Owned` can be accessed concurrently if `P` can be accessed concurrently.
>> +unsafe impl<P: Sync> Sync for Owned<P> {}
>> +
>> +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> + '_> {
>> +        let page_iter = pages.page_iter();
>> +        let size = page_iter.size();
>
> Variable naming here is confusing. There's another variable called size
> in an inner scope, and then afterwards in RawSGTable you use *this* size
> variable again.

I can change the size in the assignment block of max_segment to max_size, or do
you have other suggestions?