linux-kernel - Re: [PATCH v11 2/3] rust: add dma coherent allocator 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] [day] [month] [year] [list]

Message-ID: <CAH5fLgjL0aGGwkjEfXH08Jopwin1NfkLzp9vQWKcHPEum8jV4g@mail.gmail.com>
Date: Fri, 24 Jan 2025 08:27:36 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Abdiel Janulgue <abdiel.janulgue@...il.com>
Cc: rust-for-linux@...r.kernel.org, daniel.almeida@...labora.com, 
	dakr@...nel.org, robin.murphy@....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>, 
	Trevor Gross <tmgross@...ch.edu>, Valentin Obst <kernel@...entinobst.de>, 
	open list <linux-kernel@...r.kernel.org>, Christoph Hellwig <hch@....de>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, airlied@...hat.com, 
	"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>
Subject: Re: [PATCH v11 2/3] rust: add dma coherent allocator abstraction.

On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
<abdiel.janulgue@...il.com> wrote:
>
> Add a simple dma coherent allocator rust abstraction. Based on
> Andreas Hindborg's dma abstractions from the rnvme driver, which
> was also based on earlier work by Wedson Almeida Filho.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>

Overall I think it looks pretty good, but a follow-up from our
discussion the other day.

> +    /// Returns the device, base address, dma handle, attributes and the size of the
> +    /// allocated region.
> +    ///
> +    /// The caller takes ownership of the returned resources, i.e., will have the responsibility
> +    /// in calling `bindings::dma_free_attrs`. The allocated region is valid as long as
> +    /// the returned device exists.
> +    pub fn into_parts(
> +        self,
> +    ) -> (
> +        ARef<Device>,
> +        *mut T,
> +        bindings::dma_addr_t,
> +        crate::ffi::c_ulong,
> +        usize,
> +    ) {
> +        let size = self.count * core::mem::size_of::<T>();
> +        let ret = (
> +            // SAFETY: `&self.dev` is valid for reads.
> +            unsafe { core::ptr::read(&self.dev) },

This safety comment should explain why this will not lead to
decrementing the refcount too many times. Which is because this
doesn't actually duplicate the ARef due to the use of mem::forget.

That said, I think wrapping self in ManuallyDrop is slightly easier to
read than using mem::forget. I refer you to this pattern from Tokio
that solves a similar problem:
https://github.com/tokio-rs/tokio/blob/ee19b0ed7371b069112b9c9ef9280b81f3438d26/tokio/src/sync/rwlock/owned_write_guard.rs#L38-L51

> +            self.cpu_addr,
> +            self.dma_handle,
> +            self.dma_attrs.as_raw(),

Why not return the Attrs type instead of an integer?

> +            size,
> +        );
> +        core::mem::forget(self);
> +        ret
> +    }

> +    /// Reads data from the region starting from `offset` as a slice.
> +    /// `offset` and `count` are in units of `T`, not the number of bytes.
> +    ///
> +    /// Due to the safety requirements of slice, the data returned should be regarded by the
> +    /// caller as a snapshot of the region when this function is called, as the region could
> +    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases
> +    /// where the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()`
> +    /// could be used instead.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that no hardware operations that involve the buffer are currently
> +    /// taking place while the returned slice is live.
> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {

You were asked to rename this function because it returns a slice, but
I wonder if it's better to take an `&mut [T]` argument and to have
this function copy data into that argument. That way, we could make
the function itself safe. Perhaps the actual copy needs to be
volatile?

Well ... I understand that we did this previously and that we want to
avoid it because it causes too much reading if T is a struct and we
just want to read one of its fields. How about an API like this?

dma_read!(my_alloc[7].foo)

which expands to something that reads the value of the foo field of
the 7th element, and

dma_write!(my_alloc[7].foo = 13);

That expands to something that writes 13 to field foo of the 7th element.

Thoughts? I'm proposing this to avoid going in circles between the
same solutions.

Alice