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: <CAH5fLgiQcJcR+DYr7g3AfRPufAmM_4ZqHraGsYz7k1vU7=TZ-g@mail.gmail.com>
Date: Mon, 27 Jan 2025 14:25:03 +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 1:14 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > 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.
>
> Undefined as in the sense of anything is allowed to happen?

Yes.

> I thought undefined
> as in you might still see the old value on two consecutive reads.

That is the optimization that motivates this being UB, but it's
defined as full UB.

> Do you have a pointer to the corresponding docs?

Sure, it's on the "behavior considered undefined" page:
Moreover, the bytes pointed to by a shared reference, including
transitively through other references (both shared and mutable) and
Boxes, are immutable; transitivity includes those references stored in
fields of compound types.

https://doc.rust-lang.org/reference/behavior-considered-undefined.html#r-undefined.immutable

> > 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.
>
> From read_volatile() [1]: "In particular, a race between a read_volatile and any
> write operation to the same location is undefined behavior."

I mean, ultimately we are a bit on our own here. In C code you just
use an ordinary read / write, so we could use the ordinary
core::ptr::copy_nonoverlapping method to mirror that. We've been told
from the Rust project that we should just do these kinds of things
like we do in C - technically these things aren't okay in C either,
but because LLVM will try to avoid breaking patterns used in the
kernel, they shouldn't break in Rust either.

But using an immutable reference should be avoided because that gives
LLVM optimization hints that we are not giving to LLVM in C code.

> Also, what if the hardware put a value that is invalid for the type?

The trait bounds require that T is FromBytes, which has a safety
requirement that all bit-patterns must be valid for the type.

Alice

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ