`BufferViewMut` is unsound due to WC memory

[human-0]

Description
When mapping a buffer, wgpu can give a slice to memory that does not behave correctly with Rust atomics on x86-64, probably because it uses write-combining memory.

Repro steps

use std::sync::atomic::{AtomicPtr, AtomicU64, Ordering};

fn test(mem: &mut u8) {
    const ITERS: u64 = 10_000;

    let counter = AtomicU64::new(0);
    let ptr = AtomicPtr::new(mem);

    std::thread::scope(|s| {
        s.spawn(|| {
            let ptr = ptr.load(Ordering::Relaxed);
            let mut value = 0;

            while value < ITERS {
                value += 1;

                unsafe {
                    ptr.write(value as u8);
                }

                counter.store(value, Ordering::Release);

                while !counter.load(Ordering::Acquire).is_multiple_of(2) {}
                value += 1;
            }
        });

        s.spawn(|| {
            let ptr = ptr.load(Ordering::Relaxed);

            let mut value = 0;
            while value < ITERS {
                while counter.load(Ordering::Acquire).is_multiple_of(2) {}
                value += 1;

                unsafe {
                    assert_eq!(ptr.read(), value as u8);
                }

                value += 1;
                counter.store(value, Ordering::Release);
            }
        });
    })
}

fn main() {
    // This works
    test(&mut 0);

    let instance = wgpu::Instance::new(&wgpu::InstanceDescriptor::default());

    let adapter_options = wgpu::RequestAdapterOptions {
        power_preference: wgpu::PowerPreference::HighPerformance,
        ..Default::default()
    };
    let adapter = pollster::block_on(instance.request_adapter(&adapter_options)).unwrap();

    let (device, _queue) =
        pollster::block_on(adapter.request_device(&wgpu::DeviceDescriptor::default())).unwrap();

    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
        label: None,
        size: wgpu::COPY_BUFFER_ALIGNMENT,
        usage: wgpu::BufferUsages::MAP_WRITE,
        mapped_at_creation: true,
    });

    let mut buffer = buffer.get_mapped_range_mut(..);

    // This doesn't
    test(&mut buffer[0]);
}

The first thread writes sucessive odd numbers to the memory, and the second thread reads them. The raw pointer operations should be synchronised by the acquire/release operations, and the test sequence passes miri with regular memory.

Note that the sequence can be rewritten using relaxed atomic operations using AtomicU8::from_ptr, or without any unsafe code using AtomicU8::from_mut (unstable).

Expected vs observed behavior
The test function should exit successfully for both regular and wgpu mapped memory. However, with wgpu memory the test panics with:

assertion `left == right` failed
  left: 0
 right: 1

Platform
wgpu 28.0.0 on x86-64 Linux with Vulkan backend, Nvidia Pascal GPU with proprietary drivers. I suspect that this will reproduce on any discrete GPU with mappable device-local memory.

[inner-daemons]

Thanks for reporting this! Definitely doesn't seem like there will be a clean fix. Are there situations where its actually important to use CPU atomics on write-mappable memory?

I plan to ask about this in the next meeting, since it might be an issue relevant to spec people.

[cwfitzgerald]

Bleh.

But this isn't going to affect the spec as WC memory will only be exposed directly to the gpu process, the content process is going to get a shadow copy of the memory which will be copied back over.

[inner-daemons]

@cwfitzgerald Surely it will still affect spec compliance on native, even if Firefox can prevent this? Will this affect the CTS if it has any tests for stuff like this?

[human-0]

Are there situations where its actually important to use CPU atomics on write-mappable memory?

The atomics aren't necessarily on the mappable memory (although they could be), but they should synchronise it. A problematic use case would be something like:

let mut buffer = buffer.get_mapped_range_mut(..);
multithreaded_init(buffer.as_mut_slice());
...

It is also probably technically UB just to write to the slice on a single thread even though it 'works'.

It turns out that directly handling atomics is not necessary to observe issues. Many common sync mechanisms using atomics (e.g. std::sync::Mutex) seem to include an atomic fetch_* or compare_exchange{_weak}, which makes them work as expected for on x86, but others, like ringbuf, don't:

fn test(mut mem: &mut u8) {
    const ITERS: u64 = 10_000;

    std::thread::scope(|s| {
        let (mut producer1, mut consumer1) = ringbuf::SharedRb::new(1).split();
        let (mut producer2, mut consumer2) = ringbuf::SharedRb::new(1).split();
        s.spawn(move || {
            let mut counter = 0;
            for _ in 0..ITERS {
                *mem = counter;
                while let Err(m) = producer1.try_push(mem) {
                    mem = m;
                }

                counter = counter.wrapping_add(1);

                mem = loop {
                    if let Some(m) = consumer2.try_pop() {
                        break m;
                    }
                };

                assert_eq!(*mem, counter);
            }
        });

        s.spawn(move || {
            let mut counter = 0;
            for _ in 0..ITERS {
                let mut mem = loop {
                    if let Some(m) = consumer1.try_pop() {
                        break m;
                    }
                };

                assert_eq!(*mem, counter);
                counter = counter.wrapping_add(1);
                *mem = counter;

                while let Err(m) = producer2.try_push(mem) {
                    mem = m;
                }
            }
        });
    });
}
}

assertion `left == right` failed
  left: 0
 right: 1

It's quite possible that even Mutex<&mut [u8]> might fail on some non-x86 systems, but I don't have any non-x86 systems with discrete GPUs to test.

[kpreid]

A possible solution to this problem is to make buffer views not dereference to &mut [u8], but instead, be (or dereference to) a type which offers functions for writing arbitrary subslices, but not reading them or interpreting them as atomic types. Ideally, these operations would optimize to equivalent code so there would be no inefficiency.

Such a wrapper type would also let us get rid of the weird caveat currently documented,

It is possible to read the buffer using this view, but doing so is not recommended, as it is likely to be slow.

because the view would be truly write-only. It would offer familiar methods such as copy_from_slice(), not by dereferencing to slice::copy_from_slice() as it does today, but by providing them itself, and not provide any methods for reading.

(If there is interest in this solution, I would be willing to write a PR which defines such a restricted API, though without making any claim that it is sound.)

[kpreid]

Does this issue apply to QueueWriteBufferView as well as BufferViewMut?

[cwfitzgerald]

Yes