linux-kernel - Re: [PATCH v3 2/2] samples: rust: add sample code for scatterlist abstraction

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: <F13AD3DF-6045-45FB-B3A8-9F4BA99BC485@collabora.com>
Date: Tue, 22 Jul 2025 21:54:23 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: acourbot@...dia.com,
 dakr@...nel.org,
 jgg@...pe.ca,
 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 <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Tamir Duberstein <tamird@...il.com>,
 FUJITA Tomonori <fujita.tomonori@...il.com>,
 open list <linux-kernel@...r.kernel.org>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Randy Dunlap <rdunlap@...radead.org>,
 Herbert Xu <herbert@...dor.apana.org.au>,
 Caleb Sander Mateos <csander@...estorage.com>,
 Petr Tesarik <petr@...arici.cz>,
 Sui Jingfeng <sui.jingfeng@...ux.dev>,
 Marek Szyprowski <m.szyprowski@...sung.com>,
 Robin Murphy <robin.murphy@....com>,
 airlied@...hat.com,
 "open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 2/2] samples: rust: add sample code for scatterlist
 abstraction

Hi Abdiel, Alex,

> On 18 Jul 2025, at 07:33, Abdiel Janulgue <abdiel.janulgue@...il.com> wrote:
> 
> Add simple excercises to test the scatterlist abstraction.
> 
> Co-developed-by: Alexandre Courbot <acourbot@...dia.com>
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> ---
> samples/rust/rust_dma.rs | 49 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 9e05d5c0cdae..1fa278e8e29a 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -4,11 +4,33 @@
> //!
> //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
> 
> -use kernel::{bindings, device::Core, dma::CoherentAllocation, pci, prelude::*, types::ARef};
> +use kernel::{
> +    bindings, device::Core, dma::CoherentAllocation, page::*, pci, prelude::*, scatterlist::*,
> +    sync::Arc, types::ARef,
> +};
> 
> struct DmaSampleDriver {
>     pdev: ARef<pci::Device>,
>     ca: CoherentAllocation<MyStruct>,
> +    _sgt: SGTable<OwnedSgt<PagesArray>, ManagedMapping>,
> +}
> +
> +struct PagesArray(KVec<Page>);
> +impl SGTablePages for PagesArray {
> +    fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)> {
> +        self.0.iter().map(|page| (page, kernel::page::PAGE_SIZE, 0))

The order seems to also be inverted here (see comment on the previous patch)

> +    }
> +
> +    fn entries(&self) -> usize {
> +        self.0.len()
> +    }
> +}
> +
> +struct WrappedArc(Arc<kernel::bindings::sg_table>);
> +impl core::borrow::Borrow<kernel::bindings::sg_table> for WrappedArc {
> +    fn borrow(&self) -> &kernel::bindings::sg_table {
> +        &self.0
> +    }
> }

I assume there is no way to get around this without compromising somewhere
else, right?

> 
> const TEST_VALUES: [(u32, u32); 5] = [
> @@ -58,10 +80,35 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
>             kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
>         }
> 
> +        let mut pages = KVec::new();
> +        for _ in TEST_VALUES.into_iter() {
> +            let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
> +        }
> +
> +        // Let's pretend this is valid...

I’d reword this.

> +        // SAFETY: `sg_table` is not a reference.
> +        let sg_table: bindings::sg_table = unsafe { core::mem::zeroed() };
> +
> +        // `borrowed_sgt` cannot outlive `sg_table`.
> +        // SAFETY: From above, we assume that `sg_table` is initialized and valid.
> +        let _borrowed_sgt = unsafe { SGTable::new_unmapped(&sg_table) };

Wait, zero-initialization is considered “initialized and valid” here? i.e.:

struct sg_table {
	struct scatterlist *sgl;	/* the list */
	unsigned int nents;		/* number of mapped entries */
	unsigned int orig_nents;	/* original size of list */
};

> +
> +        let sg_table = WrappedArc(Arc::new(sg_table, GFP_KERNEL)?);
> +        // `refcounted_sgt` keeps a refcounted reference to the `sg_table` and is thus not
> +        // tied by a compile-time lifetime.
> +        // SAFETY: From above, we assume that `sg_table` is initialized and valid.
> +        let _refcounted_sgt = unsafe { SGTable::new_unmapped(sg_table) };

Ah, this is cool, though the Borrow implementation is a bit of a downside :/

> +
> +        // `owned_sgt` carries and owns the data it represents.
> +        let owned_sgt = SGTable::new_owned(PagesArray(pages), GFP_KERNEL)?;
> +        let sgt = owned_sgt.dma_map(pdev.as_ref(), kernel::dma::DmaDataDirection::DmaToDevice)?;
> +
>         let drvdata = KBox::new(
>             Self {
>                 pdev: pdev.into(),
>                 ca,
> +                // excercise the destructor
> +                _sgt: sgt,
>             },
>             GFP_KERNEL,
>         )?;
> -- 
> 2.43.0
> 

— Daniel