From 744e2cbb8a0b58eccf3a9708c8c28f1ef17e4771 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Mon, 5 Jul 2021 17:42:43 +1000 Subject: extras: Fix UB in `Peripheral` `Peripheral` assumed that interrupts can't be preempted, when they can be preempted by higher priority interrupts. So I put the interrupt handler inside a critical section, and also added checks for whether the state had been dropped before the critical section was entered. I also added a `'static` bound to `PeripheralState`, since `Pin` only guarantees that the memory it directly references will not be invalidated. It doesn't guarantee that memory its pointee references also won't be invalidated. There were already some implementations of `PeripheralState` that weren't `'static`, though, so I added an unsafe `PeripheralStateUnchecked` trait and forwarded the `unsafe` to the constructors of the implementors. --- embassy-nrf/src/buffered_uarte.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'embassy-nrf/src') diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index a5a37b982..1fa98a6b2 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -7,7 +7,7 @@ use core::task::{Context, Poll}; use embassy::interrupt::InterruptExt; use embassy::io::{AsyncBufRead, AsyncWrite, Result}; use embassy::util::{Unborrow, WakerRegistration}; -use embassy_extras::peripheral::{PeripheralMutex, PeripheralState}; +use embassy_extras::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; use embassy_extras::ring_buffer::RingBuffer; use embassy_extras::{low_power_wait_until, unborrow}; @@ -283,7 +283,8 @@ impl<'a, U: UarteInstance, T: TimerInstance> Drop for State<'a, U, T> { } } -impl<'a, U: UarteInstance, T: TimerInstance> PeripheralState for State<'a, U, T> { +// SAFETY: the safety contract of `PeripheralStateUnchecked` is forwarded to `BufferedUarte::new`. +unsafe impl<'a, U: UarteInstance, T: TimerInstance> PeripheralStateUnchecked for State<'a, U, T> { type Interrupt = U::Interrupt; fn on_interrupt(&mut self) { trace!("irq: start"); -- cgit From 1b7ad7080e6a8c96f2622d75b99a4d3bdbc7c394 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Sat, 24 Jul 2021 12:53:57 +1000 Subject: Add `Send/Sync` bounds to `PeripheralState` --- embassy-nrf/src/timer.rs | 2 +- embassy-nrf/src/uarte.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'embassy-nrf/src') diff --git a/embassy-nrf/src/timer.rs b/embassy-nrf/src/timer.rs index a6e91f228..7ff35c320 100644 --- a/embassy-nrf/src/timer.rs +++ b/embassy-nrf/src/timer.rs @@ -29,7 +29,7 @@ pub(crate) mod sealed { pub trait ExtendedInstance {} } -pub trait Instance: Unborrow + sealed::Instance + 'static { +pub trait Instance: Unborrow + sealed::Instance + 'static + Send { type Interrupt: Interrupt; } pub trait ExtendedInstance: Instance + sealed::ExtendedInstance {} diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index 67ec5d73f..985854a5f 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -461,7 +461,7 @@ pub(crate) mod sealed { } } -pub trait Instance: Unborrow + sealed::Instance + 'static { +pub trait Instance: Unborrow + sealed::Instance + 'static + Send { type Interrupt: Interrupt; } -- cgit From 079526559f44a0822574205e8798136f11f207d3 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Tue, 27 Jul 2021 17:28:52 +1000 Subject: Remove critical sections from `PeripheralMutex` interrupt handler by checking the interrupt's priority on startup. Since `PeripheralMutex` is the only way to safely maintain state across interrupts, and it no longer allows setting the interrupt's priority, the priority changing isn't a concern. This also prevents other causes of UB due to the interrupt being exposed during `with`, and allowing enabling the interrupt and setting its context to a bogus pointer. --- embassy-nrf/src/buffered_uarte.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) (limited to 'embassy-nrf/src') diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 1fa98a6b2..2cce122bc 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -176,7 +176,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { pub fn set_baudrate(self: Pin<&mut Self>, baudrate: Baudrate) { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, _irq| { + inner.with(|state| { let r = U::regs(); let timeout = 0x8000_0000 / (baudrate as u32 / 40); @@ -196,7 +196,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, _irq| { + inner.with(|state| { // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, // before any DMA action has started @@ -221,11 +221,11 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, fn consume(self: Pin<&mut Self>, amt: usize) { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, irq| { + inner.as_mut().with(|state| { trace!("consume {:?}", amt); state.rx.pop(amt); - irq.pend(); - }) + }); + inner.pend(); } } @@ -233,7 +233,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncWrite for BufferedUarte<'d, U, fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { let mut inner = self.inner(); inner.as_mut().register_interrupt(); - inner.with(|state, irq| { + let poll = inner.as_mut().with(|state| { trace!("poll_write: {:?}", buf.len()); let tx_buf = state.tx.push_buf(); @@ -254,10 +254,12 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncWrite for BufferedUarte<'d, U, // before any DMA action has started compiler_fence(Ordering::SeqCst); - irq.pend(); - Poll::Ready(Ok(n)) - }) + }); + + inner.pend(); + + poll } } -- cgit From d5ba35424d7eef2cc0c501758d214ce3a6febfc1 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Thu, 29 Jul 2021 15:11:26 +1000 Subject: Replace `PeripheralStateUnchecked` with `register_interrupt_unchecked` --- embassy-nrf/src/buffered_uarte.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'embassy-nrf/src') diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 2cce122bc..9be4d4d54 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -7,7 +7,7 @@ use core::task::{Context, Poll}; use embassy::interrupt::InterruptExt; use embassy::io::{AsyncBufRead, AsyncWrite, Result}; use embassy::util::{Unborrow, WakerRegistration}; -use embassy_extras::peripheral::{PeripheralMutex, PeripheralStateUnchecked}; +use embassy_extras::peripheral::{PeripheralMutex, PeripheralState}; use embassy_extras::ring_buffer::RingBuffer; use embassy_extras::{low_power_wait_until, unborrow}; @@ -175,7 +175,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { pub fn set_baudrate(self: Pin<&mut Self>, baudrate: Baudrate) { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } inner.with(|state| { let r = U::regs(); @@ -195,7 +195,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, U, T> { fn poll_fill_buf(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } inner.with(|state| { // Conservative compiler fence to prevent optimizations that do not // take in to account actions by DMA. The fence has been placed here, @@ -220,7 +220,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, fn consume(self: Pin<&mut Self>, amt: usize) { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } inner.as_mut().with(|state| { trace!("consume {:?}", amt); state.rx.pop(amt); @@ -232,7 +232,7 @@ impl<'d, U: UarteInstance, T: TimerInstance> AsyncBufRead for BufferedUarte<'d, impl<'d, U: UarteInstance, T: TimerInstance> AsyncWrite for BufferedUarte<'d, U, T> { fn poll_write(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8]) -> Poll> { let mut inner = self.inner(); - inner.as_mut().register_interrupt(); + unsafe { inner.as_mut().register_interrupt_unchecked() } let poll = inner.as_mut().with(|state| { trace!("poll_write: {:?}", buf.len()); @@ -285,8 +285,7 @@ impl<'a, U: UarteInstance, T: TimerInstance> Drop for State<'a, U, T> { } } -// SAFETY: the safety contract of `PeripheralStateUnchecked` is forwarded to `BufferedUarte::new`. -unsafe impl<'a, U: UarteInstance, T: TimerInstance> PeripheralStateUnchecked for State<'a, U, T> { +impl<'a, U: UarteInstance, T: TimerInstance> PeripheralState for State<'a, U, T> { type Interrupt = U::Interrupt; fn on_interrupt(&mut self) { trace!("irq: start"); -- cgit