Add AsRef impl for DumbMapping

[notgull]

In softbuffer's DRM implementation, we need to have a way to get a [u8] slice for our Deref implementation. Right now, we just create an entire buffer of zeroes and read from that. As that is unidiomatic, this PR adds an AsRef impl to DumbMapping to get a reference to the underlying memory.

See:
rust-windowing/softbuffer#135 (comment)

[MarijnS95]

UB question: is the mapped memory for this DumbMapping initialized, or should it only be accessible as &[MaybeUninit<u8>] to be defined behaviour?

[notgull]

It may be uninitialized... but in that case the AsMut implementation is invalid as well.

[Slabity]

UB question: is the mapped memory for this DumbMapping initialized, or should it only be accessible as &[MaybeUninit<u8>] to be defined behaviour?

Assuming one can only make a DumbMapping from a DumbBuffer using map_dumb_buffer on the control device, I don't believe it's possible for it to be uninitialized, right? Or is there some path that might cause an issue?

[ids1024]

Memory allocations by the OS should generally always be zeroed, for security, so an application can't read sensitive information from a different application. I think the same should apply for dumb buffers?

Regardless, if the OS returned arbitrary memory, I don't think that would be "uninitialized" in the sense of compiler undefined behavior. It won't be annotated as uninitialized in LLVM IR, and the compiler can't distinguish it from other memory obtained through FFI that's been "initialized".

[Slabity]

Ah, I see. In that case, it would definitely be dependent on how the kernel implements DumbBuffer creation. But since you can unmap and remap the memory multiple times (and still expect it to keep what you wrote in the buffer), it's probably not necessary to use MaybeUninit. If memory from another process is in the DumbBuffer when it's created, then that's a kernel bug.

But you are right, I don't think LLVM IR would annotate a mmap region as uninitialized. That sounds like it would cause a huge number of issues if that was the case.

That said, would it be a good idea to add a Deref/DerefMut implementation for this as well? A DumbMapping is essentially a pointer by all means, which is what those traits are intended for.

[ids1024]

Yeah, whether or not the kernel has zeroed the memory isn't really relevant to whether or not it's "uninitialized" . What matters is whether or not LLVM's optimizer considers it such, and it's not going to try to analyze whether or not the memory allocated by a system call or dynamic library call is uninitialized. (Except calls made though the language's own allocator.)

And good point. It makes sense to offer Deref and DerefMut as types like Vec do.

[Slabity]

Hey @notgull - Would you be able to add a Deref and DerefMut implementation to your PR? If not, I can add one after this gets merged.

[notgull]

Hey @notgull - Would you be able to add a Deref and DerefMut implementation to your PR? If not, I can add one after this gets merged.

Done!

[Slabity]

Looks good. Just merged

[MarijnS95]

It may be uninitialized... but in that case the AsMut implementation is invalid as well.

Yes, my concern was with the existing API as well.

But since you can unmap and remap the memory multiple times (and still expect it to keep what you wrote in the buffer), it's probably not necessary to use MaybeUninit.

That's a poor assumption to make (in general): just because there are cases where the developer knows the memory is initialized doesn't mean it is always initialized and shouldn't drive the API in such a way. It's in the name of the type MaybeUninit after all...
And there are functions to write to the MaybeUninit and get a (mutable borrow, iirc) initialized value back. Unfortunately the slice counterparts are still unstable, but could otherwise be used to have one distinct point where the user initializes the memory, where &mut [MaybeUninit<u8>] turns into &mut [u8].

(Those still being unstable is probably good enough of a reason to just skip this for now...)

Assuming one can only make a DumbMapping from a DumbBuffer using map_dumb_buffer on the control device, I don't believe it's possible for it to be uninitialized, right? Or is there some path that might cause an issue?

Is DumbBuffer guaranteed to have a reserved memory region or reserved pages, or could the kernel allocate them as soon as the user starts writing to those addresses? Meaning that a read prior to that could be UB? Can it use MAP_UNINITIALIZED?

It's been a while, but perhaps these might still be relevant:

https://docs.rs/presser/latest/presser/
https://www.ralfj.de/blog/2019/07/14/uninit.html

[ids1024]

Is DumbBuffer guaranteed to have a reserved memory region or reserved pages, or could the kernel allocate them as soon as the user starts writing to those addresses? Meaning that a read prior to that could be UB? Can it use MAP_UNINITIALIZED?

Normally kernels are not even compiled with support for MAP_UNINITIALIZED.

But I don't believe it actually matters either way, for safety. The memory isn't allocated by LLVM, so optimizations related to uninitialized memory don't apply. LLVM doesn't know if we've created an anonymous mmap or an mmap of a file, and it doesn't care as long as the memory isn't mutated while the reference to it exists. And mapping in new memory pages, regardless of there content, isn't mutation in the relevant sense, since it's not observable to userspace.

[ids1024]

Though I think there is a different soundness issue possible with DumbBuffer here: map_dumb_buffer is a safe function, uses MAP_SHARED, and doesn't seem to guarantee that no other mapping/fd of the buffer exists. So that could be used to violate aliasing rules.

[MarijnS95]

This isn't about mutation, it's about possibly reading uninitialized bytes, which is considered undefined behaviour?

[ids1024]

"Uninitialized memory" isn't actually a concept that exists at the hardware-level, so what matters is whether or not the compiler understands the memory to be uninitialized. Which applies to stack and heap memory it has allocated but not initialized, but shouldn't apply to memory from a dynamic library or assembly (that isn't being invoked by the compiler's own allocator).

[Slabity]

Is there a reason we would consider reading mapped memory given to us for a dumb buffer UB but not reading mapped memory for, lets say, a regular file? In my mind, they can both be considered MaybeUninit as there's no guarantee on what the initial content of the data in memory is, but that's true of pretty much any IO operations.

That's a poor assumption to make (in general): just because there are cases where the developer knows the memory is initialized doesn't mean it is always initialized and shouldn't drive the API in such a way. It's in the name of the type MaybeUninit after all...

Yes, in the general case that's absolutely true, especially when it comes to memory allocation. But I don't think it's a poor assumption to make in the case of IO though. Programs should be able to assume that file operations are going to behave sanely. Honestly the idea of using MaybeUninit for anything more than a hint to the compiler for allocation feels a bit extreme for its intended purpose.

Now that it's been brought up, I am a bit more concerned about the aliasing/mutability issues of having the memory mapped with MAP_SHARED. Is it possible to use MAP_PRIVATE to make the memory CoW? Or would that break how the underlying DumbBuffer gets updated?

[MarijnS95]

@Slabity reading fixed byte ranges from a regular file is something completely different from mapping a range of bytes of "unknown origin". In my eyes DumbBuffer is an output surface that the caller should write to so that the hardware can scan out of that memory buffer. Is it intended for readbacks?

[Slabity]

@Slabity reading fixed byte ranges from a regular file is something completely different from mapping a range of bytes of "unknown origin".

Different in what way? They both have the same soundness issues of never knowing the contents of the mapped region with certainty even after being written to (at least with MAP_SHARED), and they both hold no guarantees on the backing storage of the memory (cache? disk? temp file? DMA buffer? another process?). I understand the difference at a systems-level, but from an application or compiler view, there's not really a major difference.

In my eyes DumbBuffer is an output surface that the caller should write to so that the hardware can scan out of that memory buffer. Is it intended for readbacks?

Intended? No idea.

But a DumbBuffer can be shared between processes using PRIME, and copying a GEM-allocated buffer into one can be a cheap and simple way of reading it into system memory without needing to use a graphics API like GL/Vulkan (especially on systems where gem_prime_mmap isn't implemented). I'd say that's a pretty valid use case.