New design based on `*mut T` #22

phil-opp · 2021-05-18T12:19:40Z

TODO:

update tests
update docs

cc #13 (comment)

bjorn3

Apart from from_ref and from_mut_ref I think the new interface doesn't invoke any UB.

src/lib.rs

bjorn3 · 2021-05-18T12:43:40Z

@RalfJung volatile accesses to &UnsafeCell<T> are fine, right?

RalfJung · 2021-05-20T15:57:28Z

volatile accesses to &UnsafeCell are fine, right?

I mean, the access itself is fine, but if you are doing MMIO then you don't want to use a reference. References are considered "dereferencable" and the compiler may insert spurious reads. See e.g. here for more details.

…ncable

phil-opp · 2021-05-21T14:12:40Z

@RalfJung Thanks for taking a look! I pushed a new commit that switches from &UnsafeCell<T> to *mut T as the base type. Thus it should no longer be considered as dereferencable, right?

See e.g. here for more details.

The problem with pushing the volatile to the lowest level (e.g. struct fields) is that it doesn't work well for slices/arrays. For example, we don't want to write each byte of a [u8] slice through a separate volatile instruction since this would harm performance.

What I'm trying with this crate is to allow putting the volatile wrapper around on the outer level and then providing methods to narrow it down to specific slice ranges (through Index/IndexMut impls) or struct fields (through the addr_of_mut! macro). I'm also trying to provide volatile versions of slice fill/copy methods using intrinsics such as volatile_copy_nonoverlapping_memory and volatile_set_memory.

RalfJung · 2021-05-21T14:57:00Z

I pushed a new commit that switches from &UnsafeCell to *mut T as the base type. Thus it should no longer be considered as dereferencable, right?

Yes. Basically, "use raw pointers and only raw pointers" is currently the only option for spec-compliant MMIO in Rust.

But I'm really not the expert here, it'd be great if @Lokathor could take a look. :)

Lokathor · 2021-05-21T15:01:24Z

I will have a look later today if I can get the chance, but if you want a comparison point you can look at yourself the voladdress crate is currently what I use on the GBA. It provides abstraction types for single addresses, compact block of addresses, and strided series of addresses.

Edit: oh, you've probably already read my crate though XD

src/lib.rs

josephlr · 2021-05-27T10:56:11Z

It seems like you could simplify the marker traits/structs to avoid duplication (and making things like ReadWrite just be an alias for a more generic type):

pub struct NoAccess;
pub struct UnsafeAccess;
pub struct SafeAccess;

pub trait Unsafe {}
pub trait Safe: Unsafe {}

impl Unsafe for UnsafeAccess {}
impl Unsafe for SafeAccess {}
impl Safe for SafeAccess {}

pub struct Access<R, W> {
    pub read: R,
    pub write: W,
}

pub type ReadOnly = Access<SafeAccess, NoAccess>;
pub type WriteOnly = Access<NoAccess, SafeAccess>;
pub type ReadWrite = Access<SafeAccess, SafeAccess>;

Then the implementation would be fairly straightforward:

impl<T: ?Sized> Volatile<T> {
    pub const unsafe fn new<A>(pointer: *mut T) -> Volatile<T, A> {
        Volatile { pointer, access: PhantomData }
    }
    pub const unsafe fn from_ptr(pointer: *const T) -> Volatile<T, ReadOnly> {
        Volatile::new(pointer as *mut T)
    }
    pub const unsafe fn from_mut_ptr(pointer: *mut T) -> Volatile<T, ReadWrite> {
        Volatile::new(pointer)
    }
}

impl<T, Read, Write> Volatile<T, Access<Read, Write>> {
    pub unsafe fn read_unsafe(&self) -> T
    where
        Read: Unsafe,
    {
        ptr::read_volatile(self.pointer)
    }

    pub fn read(&self) -> T
    where
        Read: Safe,
    {
        unsafe { self.read_unsafe() }
    }

    pub unsafe fn write_unsafe(&self, val: T)
    where
        Write: Unsafe,
    {
        ptr::write_volatile(self.pointer, val)
    }

    pub fn write(&self, val: T)
    where
        Write: Safe,
    {
        unsafe { self.write_unsafe(val) }
    }

    pub unsafe fn update_unsafe(&mut self, f: impl FnOnce(&mut T))
    where
        Read: Unsafe,
        Write: Unsafe,
    {
        let mut value = self.read_unsafe();
        f(&mut value);
        self.write_unsafe(value);
    }

    pub fn update(&mut self, f: impl FnOnce(&mut T))
    where
        Read: Safe,
        Write: Safe,
    {
        unsafe { self.update_unsafe(f) }
    }
}

src/lib.rs

phil-opp · 2021-06-13T11:29:34Z

@josephlr

I thought about removing the ReadWrite etc types too, but unfortunately, type aliases cannot be used as type constructors. But methods like the following could work:

pub fn read_only() -> Access<SafeAccess, NoAccess> {
    Access {
        read: SafeAccess,
        write: NoAccess,
    }
}

The lifetime parameter is required to make `map_mut` sound (otherwise we might get mutable aliasing). Also: - add new `from_ref` and `from_mut_ref` contructor methods again. - add a `new_generic` constructor method to construct a `VolatilePtr` with arbitrary lifetime and access parameters.

Lokathor · 2021-06-14T19:15:04Z

unfortunately, type aliases cannot be used as type constructors

I'm not sure what this means. Do you mean if you use an alias like

pub type MyPhantom = PhantomData<u8>;

then you can't use MyPhantom to make the value? That's easy enough to fix, just add a const.

#[allow(non_upper_case_globals)]
pub const MyPhantom: MyPhantom = PhantomData;

josephlr

This is really looking good, mostly just questions about new_generic vs. new_with_access I'm not sure it makes sense to have both.

I put together a commit that shows what the crate would look like without new_with_access:
josephlr@952b7cd

josephlr · 2021-06-22T08:22:33Z

src/tests.rs

+    // Todo: is there a way to check that a compile error occurs when trying to use
+    // unavailable methods (e.g. `write` when write permission is `UnsafeAccess`)?


I think you can have a failing doctest? That might be the easiest way to do it.

See: https://doc.rust-lang.org/edition-guide/rust-2018/rustdoc/documentation-tests-can-now-compile-fail.html

josephlr · 2021-06-22T08:25:09Z

src/tests.rs

+    volatile.index_mut(..4);
+}
+
+#[cfg(feature = "unstable")]


Are there plans to have tests for the "very_unstable" features?" Maybe just doc-tests are enough?

josephlr · 2021-06-22T08:32:38Z

src/lib.rs

-            reference,
-            access: PhantomData,
-        }
+    pub const unsafe fn new_with_access<A>(


If we were to just delete this function and only have VolatilePtr::new_generic, would we really lose much? It seems like you would end up writing about the same amount of code.

let access = Access { read: UnsafeAccess, write: SafeAccess, }; let mut volatile = unsafe { VolatilePtr::new_with_access(NonNull::from(&mut val), access) };

vs

type Custom = Access<UnsafeAccess, SafeAccess>; let mut volatile = unsafe { VolatilePtr::new_generic::<Custom>(NonNull::from(&mut val)) }; // Or just define inline let mut volatile: VolatilePtr<_, Access<UnsafeAccess, SafeAccess>> = unsafe { VolatilePtr::new_generic(NonNull::from(&mut val)) };

It seems like having to pass the marker type by value isn't really necessary.

josephlr · 2021-06-22T08:34:38Z

src/access.rs

+pub struct Access<R, W> {
+    pub read: R,
+    pub write: W,
+}
+
+impl Access<SafeAccess, NoAccess> {
+    pub const fn read_only() -> ReadOnly {
+        Access {
+            read: SafeAccess,
+            write: NoAccess,
+        }
+    }
+
+    pub const fn write_only() -> WriteOnly {
+        Access {
+            read: NoAccess,
+            write: SafeAccess,
+        }
+    }
+
+    pub const fn read_write() -> ReadWrite {
+        Access {
+            read: SafeAccess,
+            write: SafeAccess,
+        }
+    }
+}


If we got rid of the need to pass Access by value and instead just referred to the type, we could delete all these type methods, and also just define Access like:

pub struct Access<R, W>(R, W);

josephlr · 2021-06-22T08:51:21Z

That's easy enough to fix, just add a const.
#[allow(non_upper_case_globals)]
pub const MyPhantom: MyPhantom = PhantomData;

I think we should either:

Remove new_with_access (and thus the need to even have Access values)
- Then Access can just be pub struct Access<R, W>(R, W);

Define associated constants for Access instead of const fn methods.

impl Access<SafeAccess, NoAccess> {
    pub const READ_ONLY: ReadOnly = Access {
        read: SafeAccess,
        write: NoAccess,
    };
    pub const WRITE_ONLY: WriteOnly = Access {
        read: NoAccess,
        write: SafeAccess,
    };
    pub const READ_WRITE: ReadWrite = Access {
        read: SafeAccess,
        write: SafeAccess,
    };
}

Define a single Access::new() method:

pub struct Access<R, W>(PhantomData<(R, W)>);

impl<R, W> Access<R, W> {
    const fn new() -> Self {
        Self(PhantomData)
    }
}

Then you just can use ReadOnly::new() when you need a value.

josephlr

Some comments (I don't currently have time for a full review)

josephlr · 2022-02-26T03:03:32Z

src/lib.rs

+#![cfg_attr(feature = "very_unstable", feature(const_slice_ptr_len))]
+#![cfg_attr(feature = "very_unstable", feature(const_panic))]
+#![cfg_attr(feature = "very_unstable", feature(const_fn_trait_bound))]
+#![cfg_attr(feature = "very_unstable", feature(const_fn_fn_ptr_basics))]
+#![cfg_attr(feature = "very_unstable", feature(const_trait_impl))]
+#![cfg_attr(feature = "very_unstable", feature(const_mut_refs))]
+#![cfg_attr(feature = "very_unstable", allow(incomplete_features))]


Could we drop the const_fn stuff and "very_unstable" feature for this review. That stuff might be easier to review if we do it in a followup PR. We would probably want to make certain functions const fn if a certain feature is enabled, rather than having additional functions. This is what x86_64 does.

josephlr · 2022-02-26T03:08:38Z

src/lib.rs

+        unsafe { self.split_at_mut_unchecked(mid) }
+    }
+
+    unsafe fn split_at_unchecked(


Could we add the various "unchecked" methods in a followup PR? This will allow us to focus on having the "right" API for the checked methods. Once we have the "base" PR merged, adding the unchecked methods shouldn't be too bad.

josephlr · 2022-02-26T03:23:13Z

src/lib.rs

-                value,
-                self.reference.len(),
-            );
+            intrinsics::volatile_set_memory(self.pointer.as_mut_ptr(), value, self.pointer.len());


It seems like for this method (and all the other methods that use volatile intrinsic), we should just have a stable, normal loop that just does read_volatile/write_volatile by default.

We could then add a nightly-only "slice_optimizations" feature, that changes the implementation to use the intrinsics. Then, when rust-lang/rfcs#2728 gets stabilized, we can switch to unconditionally using the stabilized methods.

josephlr · 2022-02-26T03:24:25Z

src/lib.rs

-where
-    R: Deref<Target = [T; N]>,
-{
+impl<T, R, W, const N: usize> VolatilePtr<'_, [T; N], Access<R, W>> {


Maybe I'm confused, but why does as_slice need the 'unstable" feature?

josephlr · 2022-02-26T03:26:56Z

Is there anything I can do to help this move forward? :)

It's mostly a question of if we can get this PR small enough so that I, @Lokathor, or someone else can review the API.

We would probably also want to prototype some use-cases with this API to make sure things are ergonomic.

josephlr · 2022-02-26T03:29:06Z

VolatilePtr doesn't behave like a mutable reference because it's not UB to have two VolatilePtrs pointing to the same address. This also means it can't be Send: If it were one could create two VolatilePtrs pointing to the same address, send one to another thread and create a race condiditon eg. by writing using both pointers from different threads at the same time. This only applies to ReadWrite access though.

I think it would be sound to implement Sync if T is Sync.

I think in the interest of getting this review though, we should postpone discussions of Send/Sync for a followup issue/PR. For the initial implementation, we should NonNull's lead. The type should be unconditionally !Send and !Sync. We can always relax the constraints later.

Lokathor · 2022-02-26T04:23:40Z

This PR is rather old and since it was opened I've experimentally added a "volatile region" type to voladdress: https://github.com/rust-console/voladdress/blob/main/src/volregion.rs

So, that's just how I thought things could work. Maybe it can help as a point of reference. I set it up so that it's never a dynamically sized type, because it's a pain in the butt and adds essentially zero value for my purposes.

The type should be unconditionally !Send and !Sync. We can always relax the constraints later.

This is generally the right call, because (unlike Copy) if someone "knows what they're doing" and wants to override your decision they can just go ahead and do that.

Freax13 · 2022-04-26T14:56:17Z

src/lib.rs

+    pub fn split_at_mut(
+        &mut self,
+        mid: usize,
+    ) -> (
+        VolatilePtr<[T], Access<R, W>>,
+        VolatilePtr<[T], Access<R, W>>,
+    ) {


Could this method take Self as an owned value? A problem with the current implementation is that the returned VolatilePtrs can't outlive the reference to the VolatilePtr, but they really only need to not outlive the lifetime on the VolatilePtr.

This leads to problems in practice:

fn fetch_more_data() -> [u8; 4] { todo!("fetch some data") } fn read_some_data(mut dest: VolatilePtr<[u8], Access<SafeAccess, SafeAccess>>) { while dest.len() != 0 { let chunk = fetch_more_data(); let len = cmp::min(chunk.len(), dest.len()); let (mut head, tail) = dest.split_at_mut(len); // <-- `dest` does not live long enough head[..len].copy_from_slice(&chunk[..len]); dest = tail; // <-- cannot assign to `dest` because it is borrowed } }

(The same code works just fine with &mut [u8].)

I've been trying to prototype an updated implementation for this, and I've also realized that having all the methods take self is probably what we want to do.

Conceptually, it makes sense for our VolatilePtr have self methods as it's a Copy type. It also reduces the overall number of mut vs not-mut method variants.

src/lib.rs

Freax13 · 2022-07-03T19:16:04Z

Is there a good way to make splitting mutable borrows work? We could maybe achieve this by implementing destructuring in a macro and getting access to the fields that way though it's not quite the same a splitting borrows.

Lokathor · 2022-07-03T19:20:17Z

Short answer: Not with just a volatile type.

Fundamentally, rust considers a mutable borrow exclusive, and volatile is used on things that you never have exclusive control over.

So if you want to build some sort of access control layer to the API using the borrow checker, you should build it "above" this base layer of abstraction.

Freax13 · 2022-07-03T19:34:10Z

Fundamentally, rust considers a mutable borrow exclusive, and volatile is used on things that you never have exclusive control over.

As of right now, we never assert that we have exclusive control of the memory, but we require mutable references to a VolatilePtr to allow writing to the memory location. What I want is a bit different: I'd like to be able to get two VolatilePtrs to two different fields of a struct pointed to by a VolatilePtr.
Or put even more concretly: I want this code to work somehow:

Lokathor · 2022-07-03T19:38:47Z

but we require mutable references to a VolatilePtr to allow writing to the memory location.

I simply would not write that code to begin with. These pointers should be Copy and they should take self in methods.

Or put even more concretly: I want this code to work somehow

I completely agree, that code should work.

Freax13 · 2022-07-03T19:44:10Z

I simply would not write that code to begin with. These pointers should be Copy and they should take self in methods.

I explored writing such an api in #28 though @phil-opp correctly pointed out in #28 (comment) that that would require VolatilePtr<T> to be !Sync irregardless of T which we should probably avoid if possible.

Lokathor · 2022-07-03T19:54:10Z

Well according to LLVM it's up to the platform how sync a volatile access is or not, so.... [thumbs up sign while doing a nervous smile] I guess good luck with that if your platform is multi-core and doesn't let them be sync. That sounds terrible to me.

Freax13 · 2022-07-03T20:14:55Z

Well according to LLVM it's up to the platform how sync a volatile access is or not, so.... [thumbs up sign while doing a nervous smile] I guess good luck with that if your platform is multi-core and doesn't let them be sync. That sounds terrible to me.

According to https://doc.rust-lang.org/core/ptr/fn.read_volatile.html#safety data races caused by volatile reads and writes are UB:

Just like in C, whether an operation is volatile has no bearing whatsoever on questions involving concurrent access from multiple threads. Volatile accesses behave exactly like non-atomic accesses in that regard. In particular, a race between a read_volatile and any write operation to the same location is undefined behavior.

But yeah, I totally get what you're saying though, for some applications (especially single threaded ones) having a volatile pointer be !Send + !Sync would be completely fine, but for some applications that's not what you want to do. This would likely also be a problem if we wanted to introduce this into the language: We might need to two different volatile pointer types, one that's Send + Sync and one that isn't. Not sure if there's a way to unify both of those into one type.

Lokathor · 2022-07-03T20:29:33Z

Hmmmmmm, well LLVM has a rather more flexible definition:

Given that definition, R_byte is defined as follows:

If R is volatile, the result is target-dependent. (Volatile is supposed to give guarantees which can support sig_atomic_t in C/C++, and may be used for accesses to addresses that do not behave like normal memory. It does not generally provide cross-thread synchronization.)

Otherwise, if there is no write to the same byte that happens before R_byte, R_byte returns undef for that byte.

Otherwise, if R_byte may see exactly one write, R_byte returns the value written by that write.

Otherwise, if R is atomic, and all the writes R_byte may see are atomic, it chooses one of the values written. See the Atomic Memory Ordering Constraints section for additional constraints on how the choice is made.

Otherwise R_byte returns undef.

I think the stdlib docs are actually just in the wrong here. Particularly because it makes interrupt handlers and signal handlers literally impossible to write if the target doesn't have atomics.

various improvements for the new design

Freax13 · 2023-01-17T16:30:09Z

I fully agree that making Volatile a Copy type makes it easier to use. My main concern with this design is whether we can still make the read/write methods safe without causing undefined behavior. If not, there would not be a big advantage in using Volatile instead of using core::ptr::read_volatile directly.

(from #28 (comment))

With the current implementation the read and write methods are sound. However, the soundness depends on the VolatilePtr being !Send + !Sync.

phil-opp · 2023-01-17T17:12:29Z

After reading through this discussion again, I think there are two possible designs. We can either implement Copy or Send, but not both. Implementing Copy makes the type easier to work with in single-thread environments. Implementing Send makes it possible to store the type in a static, behind a Mutex.

Both approaches seem valid to me, so I implemented them both in #29, as a base for further discussion. The question is now: Which type do we prefer? Or does it make sense to provide them both? Please let me know what you think in #29, so that we can finally replace the current unsound implementation.

New design based on UnsafeCell

a7ae579

phil-opp mentioned this pull request May 18, 2021

Rewrite crate to operate on reference values #13

Merged

bjorn3 reviewed May 18, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

Use *mut T instead of &UnsafeCell<T> since the latter is derefere…

946b37b

…ncable

phil-opp changed the title ~~New design based on UnsafeCell~~ New design based on *mut T May 21, 2021

Lokathor reviewed May 21, 2021

View reviewed changes

src/lib.rs Show resolved Hide resolved

Add access types for allowing only unsafe reads and/or writes

682dd70

josephlr reviewed May 27, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

josephlr reviewed May 27, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

mkroening mentioned this pull request May 29, 2021

Use volatile memory access for memory-mapped I/O hermit-os/kernel#199

Open

Lokathor reviewed Jun 7, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Lokathor mentioned this pull request Jun 7, 2021

Additional utility methods. rust-console/voladdress#17

Closed

Lokathor reviewed Jun 7, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Remove stabilized const_generics feature

0699e4e

phil-opp added 3 commits June 13, 2021 15:05

Redesign access types and adjust (doc)tests

2ece021

Use NonNull instead of *mut T

534ec6e

phil-opp marked this pull request as ready for review June 14, 2021 09:10

phil-opp added 2 commits June 14, 2021 12:12

Fix: do bounds checking in index and index_mut

9f7b015

Add const index functions under a new very_unstable feature

b525f38

josephlr reviewed Jun 22, 2021

View reviewed changes

josephlr reviewed Feb 26, 2022

View reviewed changes

Freax13 reviewed Apr 26, 2022

View reviewed changes

Freax13 reviewed Jun 2, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Freax13 mentioned this pull request Jun 4, 2022

various improvements for the new design #28

Merged

Freax13 added 5 commits June 15, 2022 11:26

add VolatilePtr::as_slice_mut

4accd51

add VolatilePtr::iter & VolatilePtr::iter_mut

a9e8509

add VolatilePtr::is_empty

167adec

add missing access where bounds

aa93fb9

reject mapping to unaligned fields

554c806

phil-opp mentioned this pull request Jul 6, 2022

This Month in Rust OSDev: June 2022 rust-osdev/homepage#104

Merged

10 tasks

Freax13 added 2 commits July 7, 2022 16:30

WIP: owned with borrow

ff0ee4f

fix UAF in doctest

cd57be9

qwandor mentioned this pull request Sep 23, 2022

Fix soundness issues with MMIO and shared memory rcore-os/virtio-drivers#18

Merged

Merge pull request #28 from Freax13/unsafe-cell-but-owned

927b061

various improvements for the new design

phil-opp mentioned this pull request Jan 17, 2023

[v0.5] New design with two wrapper types: VolatilePtr and VolatileRef #29

Merged

phil-opp merged commit 24326c3 into master Jun 24, 2023

phil-opp deleted the unsafe-cell branch June 24, 2023 12:33

		// Todo: is there a way to check that a compile error occurs when trying to use
		// unavailable methods (e.g. `write` when write permission is `UnsafeAccess`)?

New design based on *mut T #22

New design based on *mut T #22

Conversation

phil-opp commented May 18, 2021 • edited Loading

bjorn3 left a comment

Choose a reason for hiding this comment

bjorn3 commented May 18, 2021

RalfJung commented May 20, 2021

phil-opp commented May 21, 2021

RalfJung commented May 21, 2021

Lokathor commented May 21, 2021 • edited Loading

josephlr commented May 27, 2021 • edited Loading

phil-opp commented Jun 13, 2021

Lokathor commented Jun 14, 2021

josephlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephlr commented Jun 22, 2021

josephlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephlr commented Feb 26, 2022

josephlr commented Feb 26, 2022 • edited Loading

Lokathor commented Feb 26, 2022

Freax13 Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freax13 commented Jul 3, 2022

Lokathor commented Jul 3, 2022

Freax13 commented Jul 3, 2022

Lokathor commented Jul 3, 2022

Freax13 commented Jul 3, 2022

Lokathor commented Jul 3, 2022

Freax13 commented Jul 3, 2022

Lokathor commented Jul 3, 2022

Freax13 commented Jan 17, 2023

phil-opp commented Jan 17, 2023 • edited Loading

New design based on `*mut T` #22

New design based on `*mut T` #22

phil-opp commented May 18, 2021 •

edited

Loading

Lokathor commented May 21, 2021 •

edited

Loading

josephlr commented May 27, 2021 •

edited

Loading

josephlr commented Feb 26, 2022 •

edited

Loading

Freax13 Apr 26, 2022 •

edited

Loading

phil-opp commented Jan 17, 2023 •

edited

Loading