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: <CAH5fLggMSiHZwHJ3md0eH7smyDNVL3L=Sre7F0SXSSnPrJiO-A@mail.gmail.com>
Date: Mon, 27 Jan 2025 11:43:39 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Abdiel Janulgue <abdiel.janulgue@...il.com>, rust-for-linux@...r.kernel.org, 
	daniel.almeida@...labora.com, 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 Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > <abdiel.janulgue@...il.com> wrote:
> > > +    /// 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?
>
> Why do we consider the existing one unsafe?
>
> Surely, it's not desirable that the contents of the buffer are modified by the
> HW unexpectedly, but is this a concern in terms of Rust safety requirements?
>
> And if so, how does this go away with the proposed approach?

In Rust, it is undefined behavior if the value behind an immutable
reference changes (unless the type uses UnsafeCell / Opaque or
similar). That is, any two consecutive reads of the same immutable
reference must return the same byte value no matter what happened in
between those reads.

If we manually perform the read as a volatile read, then it is
arguably allowed for the value to be modified by the hardware while we
read the value.

> > 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);
>
> I really like how this turns out.

Yes, I think it would be a nice API.

Alice