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: <487bb34a-e304-488a-80d5-97cf55ea25d0@gmail.com>
Date: Mon, 24 Feb 2025 18:27:21 +0200
From: Abdiel Janulgue <abdiel.janulgue@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: dakr@...nel.org, robin.murphy@....com, daniel.almeida@...labora.com,
 rust-for-linux@...r.kernel.org, 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 v12 2/3] rust: add dma coherent allocator abstraction.

On 24/02/2025 15:21, Alice Ryhl wrote:
> On Mon, Feb 24, 2025 at 12:50 PM 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.
>>
>> Nacked-by: Christoph Hellwig <hch@....de>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
> 
>> +    /// Create a duplicate of the `CoherentAllocation` object but prevent it from being dropped.
>> +    pub fn skip_drop(self) -> CoherentAllocation<T> {
>> +        let me = core::mem::ManuallyDrop::new(self);
>> +        Self {
>> +            // SAFETY: The refcount of `dev` will not be decremented because this doesn't actually
>> +            // duplicafe `ARef` and the use of `ManuallyDrop` forgets the originals.
>> +            dev: unsafe { core::ptr::read(&me.dev) },
>> +            dma_handle: me.dma_handle,
>> +            count: me.count,
>> +            cpu_addr: me.cpu_addr,
>> +            dma_attrs: me.dma_attrs,
>> +        }
>> +    }
> 
> The skip_drop pattern requires the return value to use a different
> struct with the same fields, because otherwise you don't really skip
> the destructor. But I don't think you have the user for this method
> anymore so maybe just drop it.

Ah, yep. I agree.

> 
>> +    /// Retrieve a single entry from the region with bounds checking. `offset` is in units of `T`,
>> +    /// not the number of bytes.
>> +    pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
>> +        if offset >= 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.
>> +        Ok(unsafe { &mut *self.cpu_addr.add(offset) })
> 
> The point of the dma_read/dma_write macros is to avoid creating
> references to the dma memory, so don't create a reference here.
> 

This is embarrassing, I thought I had changed this already in the patch. 
My local tree had this already fixed. Anyways thanks for catching this!

/Abdiel