`GuestMemory` returns pointers but does not guarantee their validity

[DemiMarie]

Functions GuestMemory and the like return pointers but do not make any guarantees about their validity, making the returned pointers useless. In practice, callers unsoundly make assumptions about the validity of pointers.

There are three alternatives I can think of:

1. Make GuestMemory and friends unsafe to implement.
2. Deprecate the functions returning raw pointers, and replace them with functions returning VolatileSlice, whose member functions do provide guarantees about the pointers they return.
3. Do both (1) and (2).

Personally I prefer the third option.

[DemiMarie]

I tried making GuestMemory unsafe to implement, providing a helper GuestMemoryImpl trait that is not unsafe, and implementing GuestMemory for any type implementing GuestMemoryImpl. I also did the same with GuestMemoryRegion. Unfortunately, this breaks the linux-loader build due to what might actually be a rustc bug.

[roypat]

Hey, thanks for opening an issue

1. Make GuestMemory and friends unsafe to implement.

I don't really have a problem with this, it makes sense

2. Deprecate the functions returning raw pointers, and replace them with functions returning VolatileSlice, whose member functions do provide guarantees about the pointers they return.

I also don't really have a problem with this. We already have functions returning volatile slice, so downstream users can start using them today (and really should if they're generic over guest memory, because as you mention, there is no way to trust the returned pointers, making the functions useless). I know the xen folks are unhappy about functions returning pointers anyway, because they cannot be supported on xen platforms.

3. Do both (1) and (2).

This I don't quite understand though. If we dont have any functions returning pointers, then theres no need to make the trait unsafe.

[DemiMarie]

My preference would be to cut a new minor release with the functions returning raw pointers deprecated, immediately followed by a major release in which they are removed. That said, I think we still need to handle the situation where a contiguous range of guest physical memory is not contiguous in host virtual memory. Should the functions return an iterator over volatile slices? Or should callers need to perform this iteration themselves?

[bonzini]

Make GuestMemory and friends unsafe to implement.

You should make a trait unsafe is a safe implementation and safe caller code can result in undefined behavior. Here get_host_address() requires unsafe code in the caller (in your case, the call to the unsafe method vfio_dma_map) so there's no reason for GuestMemory to be unsafe.

On the other hand, deprecating get_host_address() should have been done a long time ago.

That said, I think we still need to handle the situation where a contiguous range of guest physical memory is not contiguous in host virtual memory.

This is being discussed in #327.

[roypat]

Make GuestMemory and friends unsafe to implement.

You should make a trait unsafe is a safe implementation and safe caller code can result in undefined behavior. Here get_host_address() requires unsafe code in the caller (in your case, the call to the unsafe method vfio_dma_map) so there's no reason for GuestMemory to be unsafe.

Yes, because of what you say, there is no need to make GuestMemory unsafe to ensure soundness of the API. But the trait being safe does mean that the pointer returned by get_host_address() is completely useless if one wants to use it in code that is generic over M: GuestMemory (because it's impossible to argue for soundness of dereferencing it). Marking the trait unsafe would fix this, because it would allow code that is generic M: GuestMemory to make assumptions about the pointer returned by get_host_address() based on the safety invariant of the trait.

If one is working with a specific implementation of GuestMemory, then this problem again goes away, because one can appeal to the specific argumentation to argue why the returned pointer is safe to dereference, so there are some scenarios where get_host_address() could be used (although this is also a bit dodgy).

On the other hand, deprecating get_host_address() should have been done a long time ago.

But it does seem like we're all fairly in agreement on deprecating/removing get_host_address(), so in that case let's just do that and keep GuestMemory a safe trait.

[bonzini]

the pointer returned by get_host_address() is completely useless if one wants to use it in code that is generic over M: GuestMemory

Agreed but I am not even sure what the documented safety invariant would be for get_host_address (which says a lot of the usefulness of that method), and I don't think it would be any easier to establish such an invariant for the trait as a whole. So yeah, let's just deprecate it.

[rbradford]

We have a bunch of places in CH where we call .get_host_address() - it's not immediately obvious to me what the replacement would be.

[bonzini]

Use get_slice() and get a raw pointer from there.

[DemiMarie]

Use get_slice() and get a raw pointer from there.

After doing a bounds check on the returned slice.