Brainstorming: what could a `Future`-based `libtock-rs` look like?

[jrvanwhy]

This week's discussions have convinced me to take another look at how libtock-rs could integrate with the futures ecosystem. Unfortunately, I'm having a hard time seeing how that could work. I decided to open this issue to gather ideas.

For example: what should the async equivalent of this API look like?

/// Reads data from the UART into `buffer`. Blocks
/// until the buffer is completely full, then returns.
fn read(buffer: &mut [u8]) { ... }

One idea is to return a type that implements Future which completes when the read is finished:

/// Fills `buffer` with data from the UART. The returned
/// future completes when `buffer` has been filled.
fn read(buffer: &mut [u8]) -> impl Future<Output = ()> { ... }

However, I don't see a sound way to implement that API. If read() calls ReadWriteAllow, then the following invocation causes a use-after-free:

{
    let mut buffer = [0; 8];
    // Shares buffer with the kernel
    let future = read(&mut buffer);
    // Prevents the un-allow from happening
    core::mem::forget(future);
}
// The kernel has a dangling ReadWriteAllow here.

We could delay sharing buffer with the kernel until the returned Future is polled, but this is unsound as well:

{
    let waker = /* Waker creation omitted because it's verbose */;
    let mut buffer = [0; 8];
    let mut future = Box::pin(read(&mut buffer));
    // Shares buffer with the kernel.
    let _ = future.as_mut().poll(&mut Context::from_waker(&waker));
    // Prevents the un-allow from happening.
    core::mem::forget(future);
}
// The kernel has a dangling ReadWriteAllow here.

I can think of other designs that are probably sound, but impractical and/or unergonomic:

// Note the 'static -- this only works with global buffers! IIRC that's
// unacceptable for Ti50, and very expensive in general.
fn read(buffer: &'static mut [u8]) -> impl Future<Output = &'static mut [u8]> { ... }

// Pass ownership of the buffer, relies on dynamic memory allocation. Probably
// more practical than the above but still expensive.
fn read(buffer: Box<[u8]>) -> impl Future<Output = Box<[u8]>> { ... }

Any other ideas on how this could work?

[jrvanwhy]

Idea: Pin-based Allow

I think there's a sound design based on Pin's Drop guarantee. The buffer to be shared must be part of a !Unpin type, which un-shares the buffer on Drop. Here's a simplified example (which ignores things like variable length and driver/allow numbers):

#[derive(Default)]
struct AllowRwBuffer {
    buffer: [u8; 8],
    _pinned: core::marker::PhantomPinned,
}

impl AllowRwBuffer {
    pub fn allow_rw(self: Pin<&mut Self>) {
        // Perform the Allow system call here.
    }

    pub fn get_mut_buffer(self: Pin<&mut Self>) -> &mut [u8; 8] {
        // Perform an un-allow here, if the buffer has been shared. The
        // following is pseudocode (you'd actually need some unsafe to
        // access self.buffer).
        &mut self.buffer
    }
}

impl Drop for AllowRwBuffer {
    fn drop(&mut self) {
        // Perform an un-allow here, if the buffer has been shared.
    }
}

Then the interface of read would look like:

fn read(buffer: Pin<&mut AllowRwBuffer>) -> impl Future<Output = ()> {
    buffer.allow_rw();
    /* ... */
}

Using read from within an async function would look something like:

let mut buffer: AllowRwBuffer = Default::default();
let mut buffer = pin!(buffer);
read(buffer.get_mut()).await;
// Access the data here with buffer.get_mut_buffer();

@kupiakos Does this look sound to you? I know there's a lot still to figure out, but I wanted to type up the basics somewhere.

[kupiakos]

@jrvanwhy as far as I know, that is generally sound.

I have one soundness concern: shouldn't it be keeping track of whether it's currently allowed with the kernel, so Drop doesn't double-unallow if you called get_mut_buffer?

The issue with the previous Pin-based designs is:

• Pin doesn't guarantee a struct is Dropped, it guarantees that the memory of the struct won't be reused without a Drop.
• When you mem::forget a future with a &mut of a buffer, the struct containing the &mut isn't reused, but the buffer it pointed to may be because the exclusive &mut now inaccessible.
• However, Rust doesn't understand that losing access to the &mut needed an unallow and so Rust erroneously thinks that the buffer is free to be reused or dropped by safe code.

By making the buffer owned by the pinned object, this guarantees that if the future is forgotten and the unallow never occurs, the buffer also remains inaccessible by surrounding Rust code and exclusively accessed by the kernel or subscribe callbacks.

@ComputerDruid does this sound right to you?

I'm skeptical that an API that requires all buffers to be wrapped in a Pinned object would work for Ti50. It would likely be an incredibly non-trivial refactor, and so I doubt there's the will for that to happen when a closure-based API is tolerable.

[jrvanwhy]

I have one soundness concern: shouldn't it be keeping track of whether it's currently allowed with the kernel, so Drop doesn't double-unallow if you called get_mut_buffer?

Double-unallow isn't a soundness issue. Unless some other code had re-allowed a buffer between the two un-allow calls, the second un-allow will just be a no-op. If some other code had re-allowed a buffer, then the second un-allow will just revoke the kernel's access to that other buffer.

However, the tests I've done so far (which are very limited) showed that tracking whether the buffer is currently allowed reduces code size, as it can allow the compiler to optimize the second unallow away.

I'm skeptical that an API that requires all buffers to be wrapped in a Pinned object would work for Ti50. It would likely be an incredibly non-trivial refactor, and so I doubt there's the will for that to happen when a closure-based API is tolerable.

I think there's a path forward that allows the closure-based API and the Pin-based API to coexist. My idea is to have a reference type that can be constructed using either API, which represents the ability to share a particular buffer for a particular lifetime:

// Safety invariant: buffer must be safe to share with the kernel for 'buffer.
pub struct AllowRwRef<'buffer> {
    buffer: *mut [u8],
    // PhantomData goes here.
}

impl<'buffer> AllowRwRef<'buffer> {
    pub fn from_closure(buffer: &'buffer mut [u8],
                        handle: Handle<AllowRw<'buffer, _, _, _>>) -> Self { ... }

    pub fn from_pin(allow_rw_buffer: Pin<&'buffer mut AllowRwBuffer>) -> Self { ... }

    // Allow and un-allow methods go here, signatures TBD.
}

There definitely are some open questions about the design, though. How would it track whether the buffer has been allowed to allow for the optimization I mentioned above? We could add a reference to the is-allowed flag to AllowRwRef, but then AllowRwRef would take two registers to pass into a function. We could put the is-allowed flag into a static and use generics to pass in a type that knows how to find the static, which avoids that overhead, but that's complex/messy and it may be difficult to avoid monomorphization bloat.

Oh, and because of https://github.com/tock/libtock-rs/pull/340#issuecomment-964845464, we'll probably want to keep the closure-based API for Subscribe anyway. A Pin-based Subscribe API cannot soundly allow for arbitrary Upcall implementations.

[kupiakos]

Here's a Zulip thread discussing many of the same ideas a few years earlier (i.e. something like DMA on an unowned buffer has to be callback-based and can't really use async)