x86: immediate_exit in KVM_RUN should probably be AtomicU8

[phip1611]

In cloud-hypervisor/cloud-hypervisor#7428 (comment) we are having an intense discussion about KVMs immediate_exit:

kvm/kvm-ioctls/src/ioctls/vcpu.rs

Line 1682 in 48d93d6

kvm_run.immediate_exit = val;

• The immediate_exit field should be AtomicU8
• We somehow need to expose this nicely to higher-level APIs, possibly give higher-level APIs control over the memory ordering (but I'm not too sure on the latter)

It is unfortunate that bindgen can't really automate this and it needs manual adjustment. From looking at the KVM specification and code we know that this field is supposed to be used atomically.

[rbradford]

Looking through bindgen documentation that's not doable - but we should be able to do the right thing in the kvm-ioctls wrapper method with a combination of std::mem::offset_of, AtomicU8::from_ptr(ptr) and then .store() on the atomic.

[roypat]

Does KVM access this variable atomically? We had the same discussion in firecracker a bit ago, and IIRC KVM doesn't

[phip1611]

Does KVM access this variable atomically? We had the same discussion in firecracker a bit ago, and IIRC KVM doesn't

Please look at the last sentence of my initial post :)

[roypat]

Does KVM access this variable atomically? We had the same discussion in firecracker a bit ago, and IIRC KVM doesn't

Please look at the last sentence of my initial post :)

I thought READ_ONCE was about making sure the compiler doesn't muck about with the memory access, but is unrelated to acquire/release semantics of atomic variables

[phip1611]

I'm also not 100% sure, but:

Yeah, READ_ONCE/WRITE_ONCE is generally used in the kernel under assumptions that it's basically equivalent to (or stronger than) relaxed atomics, even though by spec the implementation via volatile isn't the same

Source

[DemiMarie]

Generally, bindgen is not a good tool for generating safe kernel API wrappers. For instance, it doesn’t know that a lot of u64 fields are actually pointers.

[olivereanderson]

Looking through bindgen documentation that's not doable - but we should be able to do the right thing in the kvm-ioctls wrapper method with a combination of std::mem::offset_of, AtomicU8::from_ptr(ptr) and then .store() on the atomic.

This will unfortunately still be unsound because kvm_ioctls::VcpuFd::run produces a mutable reference to kvm_run and it is thus unsafe for us to read or write to the immediate exit flag be it in a signal handler on the same thread or on another thread altogether. It doesn't matter if only our code reads or writes to kvm_run.immediate_exit, it is still undefined behavior. We can also not soundly work around this issue with alias mappings.

The best way forward would be to change it to AtomicU8 or at the very least an UnsafeCell<u8> and then downstream crates/applications have means to modify the immediate exit flag while we are in KVM_RUN without being in violation of Rust's abstract machine/unsafe guidelines.

EDIT: Actually we might have UB even if we change the type to be AtomicU8/ UnsafeCell<u8> , because one could still obtain a &mut u8 through &mut kvm_run 🙁

[olivereanderson]

The best thing would probably be to just use raw pointers for all interactions with kvm_run. That would probably be a lot of ecosystem churn though.

[rbradford]

The best thing would probably be to just use raw pointers for all interactions with kvm_run. That would probably be a lot of ecosystem churn though.

Or could we return an opaque type and then provides methods on it to manipulate the immediate_exit safely including the required barriers?

[olivereanderson]

The best thing would probably be to just use raw pointers for all interactions with kvm_run. That would probably be a lot of ecosystem churn though.

Or could we return an opaque type and then provides methods on it to manipulate the immediate_exit safely including the required barriers?

I would suggest refactoring the internals of kvm-ioctls to avoid creating &mut kvm_run and interacting with kvm_run via raw pointers instead and removing the public method VcpuFd::get_kvm_run (or at the very least making it unsafe).

This will be necessary no matter what if we want to soundly update immediate_exit while we are in KVM_RUN. I honestly don't know how much VcpuFd::get_kvm_run is used in downstream crates to estimate the churn of removing that public method (or making it unsafe) though.

In addition, as you suggest, we could indeed also have a method on VcpuFd that returns an opaque type for (more) safe interactions with immediate_exit. There could also be other methods taking &mut self and returning mutable references to other fields of kvm_run thus making it easier (for downstream crates) to update kvm_run in a safe manner.

[olivereanderson]

I would argue that we can close this now: #373 is what we actually need.