The proposed DMA API is unsafe

[teskje]

It appears that the DMA API described in Chapter 8 - DMA is unsafe.

Consider this example. It is basically the motivating example from the "Immovable buffers" section, but with the complete read_exact API from the end of the chapter, including Pining and the 'static bound. The example shows that it is still possible to pass a stack-allocated array into this (supposedly safe) read_exact function, which means the DMA operation will corrupt the stack. All you need to do is wrap the array in a newtype and impl DerefMut for it.

I believe the root of the issue is a misunderstanding of the Pin API. Contrary to intuition, Pin does in most cases not pin the pointed-to data in memory: If the target type implements Unpin, Pin does nothing at all:

Many types are always freely movable, even when pinned, because they do not rely on having a stable address. This includes all the basic types (like bool, i32, and references) as well as types consisting solely of these types. Types that do not care about pinning implement the Unpin auto-trait, which cancels the effect of Pin<P>.

Basically all types we care about implement Unpin (I believe only self-referential types are supposed not to?). Which means Pining the buffer passed into the DMA doesn't help us make the code safe.

The DMA chapter also mentions the StableDeref trait as an alternative. As far as I see, using this would actually lead to a safe API, so we should change the chapter accordingly.

[teskje]

I thought about the use of StableDeref a bit more. The guarantees it provides with regard to DerefMut are a bit hard to grasp. For example, Vec implements StableDeref, even though it can re-allocate if you push to it. My understanding is that the stable address guarantee for DerefMut types only holds as long as you don't call &mut self methods on the type itself. But the documentation doesn't explicitly state that.

Let's take a step back. Here is what I gather are the minimum requirements a generic safe DMA buffer type has to fulfill:

1. It must be a pointer.
   Otherwise the buffer would be part of the Transfer struct and move around with it on the stack, which we cannot allow (see 3).
2. The referenced buffer must be valid (i.e. not freed) for the length of the transfer.
   Otherwise the compiler could re-use the buffer space for other data, while the DMA was still reading from/writing to it.
3. The referenced buffer must be at a stable address for the length of the transfer.
   Same reason as 2. Additionally, data returned from DMA reads would be incomplete when the buffer moved in the meantime.

Requirements 2 and 3 must be true even if we take mem::forget into account: If the Transfer is mem::forgetten, we won't be able to stop the DMA since the destructor is not run. So the buffer needs to stay valid and stable afterwards.

Looking at the types StableDeref is implemented for out of the box, it looks like all of them fulfill the above requirements. So provided I'm not missing an essential requirement above, using StableDeref for DMA buffers should be fine. In fact, it seems to be exactly what we need in terms of guarantees.

[fg-cfh]

IMHO there is another soundness issue in the DMA example: Technically the compiler fence must be placed before escaping the pointer to the DMA hardware, not after. Otherwise local writes to the buffer could be reordered or removed by the compiler. While this is probably not a practical issue in the specific example, it still invites unnecessary misunderstandings where the difference matters, see e.g. this comment in the embedded rust matrix room which explicitly referred to the embedonomicon example.

Examples in the wild:

• wrong in the stm32*_hal
• correct in nrf_hal.

Other potential changes:

• The rust op-sem team now officially recommends an empty asm!("") block to "hand off" memory to/from DMA. But be aware of this soundness issue in LLVM which needs to be fixed first.
• The example should be updated to use the embedded_dma traits.

Currently:

        let slice = buffer.as_slice();
        let (ptr, len) = (slice.as_ptr(), slice.len());
        self.dma.set_destination_address(USART1_TX, false);
        self.dma.set_source_address(ptr as usize, true);
        self.dma.set_transfer_length(len);
        atomic::compiler_fence(Ordering::Release);    

Fixed:

        // 1. Use embedded_dma::WriteBuffer trait.
        let (ptr, len) = unsafe { buffer.write_buffer() };

        // 2. Compiler fence must be placed after writes
        //    to normal memory but _before_ the pointer
        //    escapes.
        atomic::compiler_fence(Ordering::Release);

        // alternatively - to avoid off-label use of
        // compiler_fence
        // asm!("");
        // or for multi-CPU/non-cortex-m targets:
        // asm("...some architecture-specific CPU memory barrier...");

        self.dma.set_destination_address(USART1_TX, false);
        self.dma.set_source_address(ptr as usize, true);
        self.dma.set_transfer_length(len);

If you agree, I'll provide a PR.

Further discussion of the updated approach to DMA on the rust users forum including some godbolt examples demonstrating miscompilations with misplaced fences:

• Compiler fence and DMA: https://users.rust-lang.org/t/compiler-fence-dma/132027
• asm! as memory barrier: https://users.rust-lang.org/t/how-to-correctly-use-asm-as-memory-barrier-llvm-question/132105