linux-kernel - Re: [PATCH v12 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: <a65125ce-524f-4226-9e59-32b731ad97f0@gmail.com>
Date: Tue, 25 Feb 2025 10:15:33 +0200
From: Abdiel Janulgue <abdiel.janulgue@...il.com>
To: Benno Lossin <benno.lossin@...ton.me>, aliceryhl@...gle.com,
 dakr@...nel.org, robin.murphy@....com, daniel.almeida@...labora.com,
 rust-for-linux@...r.kernel.org
Cc: 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>,
 Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
 Valentin Obst <kernel@...entinobst.de>, linux-kernel@...r.kernel.org,
 Christoph Hellwig <hch@....de>, Marek Szyprowski <m.szyprowski@...sung.com>,
 airlied@...hat.com, iommu@...ts.linux.dev
Subject: Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

Hi,

On 24/02/2025 22:07, Benno Lossin wrote:
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// It is the callers responsibility to avoid separate read and write accesses to the region
>> +    /// while the returned slice is live.
> This safety requirement is worded quite differently compared to the one
> on `as_slice`, why?
> 
>> +    pub unsafe fn as_slice_mut(&self, offset: usize, count: usize) -> Result<&mut [T]> {
>> +        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
>> +        if end >= 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.
>> +        // - `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.
>> +        Ok(unsafe {core::slice::from_raw_parts_mut(self.cpu_addr.add(offset), count) })
>> +    }
>> +
>> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
>> +    /// number of bytes.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// # fn test(alloc: &mutkernel::dma::CoherentAllocation<u8>) -> Result {
>> +    /// let somedata: [u8; 4] = [0xf; 4];
>> +    /// let buf: &[u8] = &somedata;
>> +    /// alloc.write(buf, 0)?;
>> +    /// # Ok::<(), Error>(()) }
>> +    /// ```
>> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
>> +        let end = offset.checked_add(src.len()).ok_or(EOVERFLOW)?;
>> +        if end >= 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())
> Why are there no concurrent write or read operations on `cpu_addr`?

Thanks for the feedback! I noticed an additional safety requirement in 
slice::from_raw_parts_mut:

"The memory referenced by the returned slice must not be accessed 
through any other pointer (not derived from the return value) for the 
duration of lifetime 'a. Both read and write accesses are forbidden."

I can see now though why both as_slice and as_slice_mut docs needs more 
clarity. i.e., they could be worded similarly and add the additional 
safety requirement of slice::from_raw_parts_mut of having no other r/w 
access while the slice is live?

Regards,
Abdiel