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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLgg+Su4FWXB23Zd=oQw_xEzBKRqpTujQiiOKDOYqqXheCQ@mail.gmail.com>
Date: Tue, 28 Jan 2025 12:28:51 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Abdiel Janulgue <abdiel.janulgue@...il.com>, rust-for-linux@...r.kernel.org, 
	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 Tue, Jan 28, 2025 at 11:37 AM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> >
> > I mean, I imagine that you could make the syntax
> > `dma_read!(my_alloc[7])` read the entire struct. I'm not sure which
> > safe methods you are referring to, because right now there is only the
> > unsafe as_slice().
> >
> > Alice
>
> I mean this:
>
> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
> +    /// number of bytes.
> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
> +        if offset + src.len() >= self.count {
> +            return Err(EINVAL);
> +        }
> +        // SAFETY:
> +        // - The pointer is valid due to type invariant on `CoherentAllocation`
> +        // and we've just checked that the range and index is within bounds.
> +        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
> +        // that `self.count` won't overflow early in the constructor.
> +        unsafe {
> +            core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu_addr.add(offset), src.len())
> +        };
> +        Ok(())
> +    }
> +}
>
> …and the similar read() method that was apparently removed, i.e.:
>
> +    /// 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 read(&self, offset: usize, count: usize) -> Result<&[T]> {
> +        if offset + count >= self.count {
> +            return Err(EINVAL);
> +        }
> +        // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation`,
> +        // we've just checked that the range and index is within bounds. The immutability of the
> +        // of data is also guaranteed by the safety requirements of the function.
> +        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.wrapping_add(offset), count) })
> +    }
>
> I don’t think there’s anything wrong with these, so maybe your solution can come in as a convenience
> when one wants to save on the amount of copying? IMHO the two methods above co-exist with what
> you propose.
>
> The unsafe `as_slice()` and `as_slice_mut()` should also stay, in my opinion. They can be used, for example,
> in codec drivers.

Agreed, there is nothing wrong with the safe write that was here
previously, or the current unsafe functions.

Alice