Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The proposed DMA API is unsafe #64

Open
teskje opened this issue Apr 6, 2020 · 1 comment
Open

The proposed DMA API is unsafe #64

teskje opened this issue Apr 6, 2020 · 1 comment

Comments

@teskje
Copy link

teskje commented Apr 6, 2020

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 added a commit to teskje/stm32f3xx-hal that referenced this issue Apr 7, 2020
Using Pin for DMA buffers does not provide the necessary safety.
See rust-embedded/embedonomicon#64
@teskje
Copy link
Author

teskje commented Apr 8, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant