DMA unsound - Simultaneous write to identical address possible

[Finomnis]

Problem

The user can implement his own WriteTarget object. This can lead to things like this:

//! # Memory to memory DMA transfer Example
//!
//! This application demonstrates how to use DMA to transfer data from memory to memory buffers.
//!
//! See the `Cargo.toml` file for Copyright and licence details.
#![no_std]
#![no_main]

use cortex_m::singleton;
use cortex_m_rt::entry;
use hal::dma::{single_buffer, DMAExt, WriteTarget};
use hal::pac;
use panic_halt as _;
use rp2040_hal as hal;

/// The linker will place this boot block at the start of our program image. We
/// need this to help the ROM bootloader get our code up and running.
#[link_section = ".boot2"]
#[used]
pub static BOOT2: [u8; 256] = rp2040_boot2::BOOT_LOADER_GENERIC_03H;

/// External high-speed crystal on the Raspberry Pi Pico board is 12 MHz. Adjust
/// if your board has a different frequency
const XTAL_FREQ_HZ: u32 = 12_000_000u32;

#[entry]
fn main() -> ! {
    // Grab our singleton objects
    let mut pac = pac::Peripherals::take().unwrap();
    let _core = pac::CorePeripherals::take().unwrap();

    // Set up the watchdog driver - needed by the clock setup code
    let mut watchdog = hal::watchdog::Watchdog::new(pac.WATCHDOG);

    // Configure the clocks
    let _clocks = hal::clocks::init_clocks_and_plls(
        XTAL_FREQ_HZ,
        pac.XOSC,
        pac.CLOCKS,
        pac.PLL_SYS,
        pac.PLL_USB,
        &mut pac.RESETS,
        &mut watchdog,
    )
    .ok()
    .unwrap();

    // Initialize DMA.
    let dma = pac.DMA.split(&mut pac.RESETS);

    // Use DMA to transfer some bytes (single buffering).
    let tx_buf1 = singleton!(: [u8; 16] = [0x42; 16]).unwrap();
    let tx_buf2 = singleton!(: [u8; 16] = [0x69; 16]).unwrap();
    let mut rx_buf = singleton!(: [u8; 16] = [0; 16]).unwrap();
    let rx_buf_ref = DmaTargetRef::new(&mut rx_buf);

    // Use a single_buffer to read from tx_buf and write into rx_buf
    let transfer = single_buffer::Config::new(dma.ch0, tx_buf1, rx_buf_ref).start();
    drop(transfer);
    let transfer = single_buffer::Config::new(dma.ch1, tx_buf2, rx_buf).start();
    // !!!!!!!!!!!!!!!!
    // The second transfer was started without waiting for the first transfer, with both DMAs writing to the same memory address!
    // A solution would be to `Seal` the `WriteTarget` trait.
    // !!!!!!!!!!!!!!!!

    // Wait for DMA channels to finish
    let (_ch, _tx_buf, _rx_buf) = transfer.wait();

    loop {}
}

pub struct DmaTargetRef<'a, T> {
    buf: &'a mut T,
}

impl<'a, T> DmaTargetRef<'a, T> {
    pub fn new(buf: &'a mut T) -> Self {
        Self { buf }
    }
}

impl<'a, T> WriteTarget for DmaTargetRef<'a, T>
where
    T: WriteTarget,
{
    type TransmittedWord = T::TransmittedWord;

    fn tx_treq() -> Option<u8> {
        <&mut [u8]>::tx_treq()
    }

    fn tx_address_count(&mut self) -> (u32, u32) {
        self.buf.tx_address_count()
    }

    fn tx_increment(&self) -> bool {
        self.buf.tx_increment()
    }
}

Solution

Seal the WriteTarget and ReadTarget traits.

[jannic]

Alternative solution: mark both traits as unsafe.

[ithinuel]

Could we stop the DMA/transfer when the transfer is dropped?
Wouldn't this avoid breaking some aliasing rules?

[Finomnis]

Could we stop the DMA/transfer when the transfer is dropped? Wouldn't this avoid breaking some aliasing rules?

You can still return arbitrary numbers from tx_address_count, so at least this one has to be unsafe.

[jannic]

You can still return arbitrary numbers from tx_address_count, so at least this one has to be unsafe.

Exactly. This is proof enough that the ability to create arbitrary implementations of WriteTarget and ReadTarget from safe code is unsound. No need to construct a data race by dropping a running transfer.

AFAIK we have the two already mentioned options:

• make the traits sealed, so that user code can't provide additional implementations of those traits at all
• make the traits unsafe so that user code needs to observe some safety guarantees when implementing them

Of course we can (and probably should) mark the traits as unsafe even if we also make them sealed. In that case, the unsafe keyword is more like a reminder for ourselves that we need to be careful when implementing the traits, as other code can't implement the traits anyway.

I think that @Finomnis suggestion of sealing the traits is the better choice: We can always relax the restriction later and switch to the other choice, but the other way around would be a breaking change.
But it'd like to hear from people with more experience using the DMA engine if the provided implementations are sufficient for most use cases, or if in practice it is necessary for user code to provide additional implementations.

In any case, we can already mark the traits as unsafe.

[Finomnis]

Additional, I like the idea of canceling the transfer on drop, that would allow us to implement WriteTarget for &mut WriteTarget and would even allow us to use &mut Channel. This would greatly simplify using it from within rtic. It would basically be a scope guard then.

Edit: never mind. I now understand the reason you took the ownership route. mem::forget would bypass the cancelation and create an unsound situation again.

[jannic]

Marking the traits sealed in the straight-forward way is problematic: I can't easily impl Sealed for the types that currently impl ReadTarget. There is a blanket impl for all types implementing ReadBuffer:

unsafe impl<B: ReadBuffer> ReadTarget for B

When I try to to impl Sealed for all those types, like this,

impl<B: ReadBuffer> Sealed for B {}                                                            

I get several errors due to conflicts:

error[E0119]: conflicting implementations of trait `Sealed` for type `u32`
   --> rp2040-hal/src/float/mod.rs:39:9
    |
39  |         impl Sealed for $ty {}
    |         ^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `u32`
...
43  | int_impl!(u32);
    | -------------- in this macro invocation
    |
   ::: rp2040-hal/src/dma/mod.rs:194:1
    |
194 | impl<B: ReadBuffer> Sealed for B {}
    | -------------------------------- first implementation here
    |
    = note: upstream crates may add a new impl of trait `core::ops::Deref` for type `u32` in future versions
    = note: upstream crates may add a new impl of trait `stable_deref_trait::StableDeref` for type `u32` in future versions
    = note: this error originates in the macro `int_impl` (in Nightly builds, run with -Z macro-backtrace for more info)

I think the root cause of those conflicts is that re-using the same trait, typelevel::Sealed, to seal multiple different traits, is a bad idea. But I don't fully understand the proper use of this pattern yet.

[jannic]

It is possible to make the traits sealed by defining distinct sealing traits:

 pub(crate) mod private { 
     /// Super traits used to mark traits with an exhaustive set of                                  
     /// implementations                                                                           
     ///                                                                                             
     /// These are separate from `crate::typelevel::Sealed` to avoid                               
     /// conflicts because of overlapping generic implementations. 
     pub trait SealedR {}                                                   
     pub trait SealedW {}                        
 }                                                                        

[...]

pub unsafe trait ReadTarget: private::SealedR

[...]

pub unsafe trait WriteTarget: private::SealedW

But I get the impression that the little added safety gained isn't worth the added complexity.
So I vote for just merging #621 and leave the traits un-sealed. 🦭

[jannic]

#621 was merged. Shall we close this ticket or is there more to do?

[Finomnis]

Not sure, topic too complicated :D

[9names]

If you find more unsoundness we can always open a new issue. The question is whether you think this is sufficient to avoid what we currently know about.

[Finomnis]

I agree that we should continue this discussion once we find an actual reproducible example of unsoundness.

[jannic]

Ok. With #621 all known instances of unsoundness should be formally solved, as the traits are now marked as unsafe, so it's the responsibility of the implementing code to provide a sound implementation.

There always is potential to improve usability, and there may be unknown soundness issues. Let's close this ticket and open a new one if necessary.

BTW, thanks to @Finomnis for reporting this issue!