Message-ID: <20250512164247.GF138689@ziepe.ca>
Date: Mon, 12 May 2025 13:42:47 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: 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: [RFC PATCH 1/2] rust: add initial scatterlist bindings

On Mon, May 12, 2025 at 12:53:27PM +0300, Abdiel Janulgue wrote:
> +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
> +    /// of the returned reference.
> +    pub unsafe fn as_ref<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
> +        // SAFETY: The pointer is valid and guaranteed by the safety requirements of the function.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Convert a raw `struct scatterlist *` to a `&'a mut SGEntry`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// See safety requirements of [`SGEntry::as_ref`]. In addition, callers must ensure that only
> +    /// a single mutable reference can be taken from the same raw pointer, i.e. for the lifetime of the
> +    /// returned reference, no other call to this function on the same `struct scatterlist *` should
> +    /// be permitted.
> +    unsafe fn as_mut<'a>(ptr: *mut bindings::scatterlist) -> &'a mut Self {
> +        // SAFETY: The pointer is valid and guaranteed by the safety requirements of the function.
> +        unsafe { &mut *ptr.cast() }
> +    }
> +
> +    /// Returns the DMA address of this SG entry.
> +    pub fn dma_address(&self) -> bindings::dma_addr_t {
> +        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
> +        unsafe { bindings::sg_dma_address(self.0.get()) }
> +    }
> +
> +    /// Returns the length of this SG entry.
> +    pub fn dma_len(&self) -> u32 {
> +        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
> +        unsafe { bindings::sg_dma_len(self.0.get()) }
> +    }
> +
> +    /// Set this entry to point at a given page.
> +    pub fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
> +        let c: *mut bindings::scatterlist = self.0.get();
> +        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
> +        // `Page` invariant also ensure the pointer is valid.
> +        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
> +    }

I think this is repeating the main API mistake of scatterlist here in
Rust.

Scatterlist is actually two different lists of value pairs stored in
overlapping memory.

It's lifetime starts out with a list of CPU pages using struct page *

Then it is DMA mapped and you get a list of DMA ranges using
dma_len/dma_address. At this point the CPU list becoms immutable

Iterators work over one list or the other, never ever both at once. So
you should never have a user facing API where they get a type with
both a set_page and a dma_len.

Arguably set_page probably shouldn't exist in the rust bindings, it is
better to use one of the append functions to build up the initial
list.

> +/// DMA mapping direction.
> +///
> +/// Corresponds to the kernel's [`enum dma_data_direction`].
> +///
> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> +#[derive(Copy, Clone, PartialEq, Eq)]
> +#[repr(u32)]
> +pub enum DmaDataDirection {

Shouldn't this in the DMA API headers?

> +impl DeviceSGTable {
> +    /// Allocate and construct the scatter-gather table.
> +    pub fn alloc_table(dev: &Device, nents: usize, flags: kernel::alloc::Flags) -> Result<Self> {
> +        let sgt: Opaque<bindings::sg_table> = Opaque::uninit();
> +
> +        // SAFETY: The sgt pointer is from the Opaque-wrapped `sg_table` object hence is valid.
> +        let ret = unsafe { bindings::sg_alloc_table(sgt.get(), nents as _, flags.as_raw()) };
> +        if ret != 0 {
> +            return Err(Error::from_errno(ret));
> +        }
> +
> +        Ok(Self {
> +            sg: SGTable(sgt),
> +            dir: DmaDataDirection::DmaNone,
> +            dev: dev.into(),
> +        })
> +    }
> +
> +    /// Map this scatter-gather table describing a buffer for DMA.
> +    pub fn dma_map(&mut self, dir: DmaDataDirection) -> Result {
> +        // SAFETY: Invariants on `Device` and `SGTable` ensures that the `self.dev` and `self.sg`
> +        // pointers are valid.
> +        let ret = unsafe {
> +            bindings::dma_map_sgtable(
> +                self.dev.as_raw(),
> +                self.sg.as_raw(),

Can't call this function on an unbound driver, didn't someone add
special types for this recently?

> +impl Drop for DeviceSGTable {
> +    fn drop(&mut self) {
> +        if self.dir != DmaDataDirection::DmaNone {
> +            // SAFETY: Invariants on `Device` and `SGTable` ensures that the `self.dev` and `self.sg`
> +            // pointers are valid.

Wrong safety statement, Device must be on a bound driver to call this
function, a valid pointer (eg device refcount) is not enough.

Jason

Powered by blists - more mailing lists