tock-register-interface ReadWrite / WriteOnly potentially unsound without UnsafeCell?
Hi,
I was looking through the code in tock-register-interface, and found this line:
As I understand it, this is mutating arbitrary Copy data through an immutable reference. This may be a bit naive of me, but I think this brings up two safety issues.
First, according to the nomicon, mutating through an immutable reference like this is undefined behavior:
Behavior considered undefined
[...]
- Mutating immutable data. All data inside a const item is immutable. Moreover, all data reached through a shared reference or data owned by an immutable binding is immutable, unless that data is contained within an UnsafeCell.
The specific problem is that this *mut T is obtained from an &T.
Second, as far as I can tell, ReadWrite is Sync. According to the ptr docs, read_volatile and write_volatile aren't thread safe:
The precise rules for validity are not determined yet. The guarantees that are provided at this point are very minimal:
[...]
- All accesses performed by functions in this module are non-atomic in the sense of atomic operations used to synchronize between threads. This means it is undefined behavior to perform two concurrent accesses to the same location from different threads unless both accesses only read from memory. Notice that this explicitly includes
read_volatileandwrite_volatile: Volatile accesses cannot be used for inter-thread synchronization.
I'm not sure how much tock kernel code running on microcontrollers would run into accidentally abusing it being Sync. But the undefined behavior from using a *mut ptr gotten from an immutable reference seems like something to avoid.
Would it be reasonable to use an UnsafeCell here, instead? It would fix both issues, as with it one can avoid creating an &T, and it is non-Sync. It also has #[repr(transparent)], so I don't think it'd change the behavior otherwise.
With that said, I'm a novice at embedded development, so there's likely something I missed. Thoughts?
daboross
All 5 comments
I am pretty confused by this issue, and wondering if perhaps you linked the wrong lines in the register interface. Where is the get() method mutating anything? It just returns a copy of the value it finds in the mapped register -- its not returning a mutable reference (or a *mut T, so I am confused where the references to *mut T are coming from).
hudson-ayers
on 8 Jul 2020
Yes, it should have linked to set() rather than get().
I agree that unsafe { ::core::ptr::write_volatile(&self.value as *const T as *mut T, value) } triggers undefined behavior because it casts a &T to a *mut T then writes to it. Solving this requires using UnsafeCell.
In the context of Tock, being Sync is actually fine: Tock's kernel is single-threaded so putting Sync types into static locations is fine. However, tock-register-interface is available on crates.io, and could be used in a multithreaded embedded system, where it is an unsound API.
Last, even if we use UnsafeCell, there is another issue. &UnsafeCell<T> still doesn't allow the hardware to mutate the contained value! All Rust references (including references to &UnsafeCell) have the LLVM dereferenceable attribute, which allows LLVM to dereference them arbitrarily and assume they haven't changed (unless LLVM generates code that changes them). Unfortunately, this is not compatible with MMIO registers. There is more background at https://github.com/rust-lang/unsafe-code-guidelines/issues/33. This issue is much harder to fix than the others.
jrvanwhy
on 8 Jul 2020
Yeah, we should just wrap it in UnsafeCell. The implementation of UnsafeCell is the same thing, except it's marked as a special lang-item (which I guess is reserved for future potential optimizations that take advantage of these undefined behavior restrictions).
Regarding Sync, @jrvanwhy is right that this isn't technically a problem in Tock, and it's useful in order to be able to store these in static variables. Though, with StaticRef I'm not certain we need that anymore.
alevy
on 8 Jul 2020
wondering if perhaps you linked the wrong lines in the register interface
I did! I meant to link to set rather than get, as @jrvanwhy noted. I'll fix that.
In the context of Tock, being Sync is actually fine:
This makes sense! If tock is purely single-threaded, then it seems like there aren't any concerns. Makes sense that it's useful for static variables too - though it does seem like everything (or at least, everything in the tock repo) still compiles today if we change it to UnsafeCell and thus remove the Sync impl?
Last, even if we use
UnsafeCell, there is another issue.&UnsafeCell<T>still doesn't allow the _hardware_ to mutate the contained value!
This is troubling - and interesting!
Do you think it'd be worth changing to UnsafeCell anyways, even if we still have this issue, though?
From what I can tell, while there aren't any hard guarantees for hardware-mutated values, there's nothing explicitly disallowing it, either. In that case, UnsafeCell could still be a positive if it means going from something explicitly disallowed (even if only for future optimizations) to something that might not be allowed, but isn't explicitly disallowed either.
daboross
on 9 Jul 2020
Last, even if we use
UnsafeCell, there is another issue.&UnsafeCell<T>still doesn't allow the _hardware_ to mutate the contained value!This is troubling - and interesting!
Do you think it'd be worth changing to
UnsafeCellanyways, even if we still have this issue, though?
Yes.
jrvanwhy
on 9 Jul 2020
Related issues
bbbert
路
15Comments
bradjc
路
3Comments
alexandruradovici
路
8Comments
bradjc
路
6Comments
alistair23
路
8Comments
Most helpful comment
Yes.