VirtAddr::try_new() succeeds on non-canonical addresses

[josephlr]

From the VirtAddr::try_new docs:

This function tries to performs sign extension of bit 47 to make the address canonical. It succeeds if bits 48 to 64 are either a correct sign extension (i.e. copies of bit 47) or all null. Else, an error is returned.

This means that (surprisingly) you can provide a non-canonical address 0x00008123DEADBEEF and, instead of try_new failing, you get the VirtAddr 0xFFFF8123DEADBEEF. As the try_new implementation is used throughout VirtAddr's methods, this means you get weird behavior like VirtAddr::new(addr).as_u64() == addr and VirtAddr::from_ptr(ptr).as_ptr() == ptr not necessarily being true, which is quite confusing.

It's especially confusing if you're working on a 5-level paging system where the virtual address space now has 57 bits. In that case, 0x0008123DEADBEEF is a canonical virtual address. I would either expect VirtAddr::try_new to work in this case (returning the passed in address) or fail (returning Err, as this library doesn't have to support 5-level paging). What I wouldn't want is for the virtual address to be silently modified.

Issues related to this have come up in #298. One of the consequences of the current implementation is that adding an u64 to a VirtAddr can "jump" the gap between the lower and upper half of the virtual address space. For example:

let x = VirtAddr::new(0x0000_7FFF_FFFF_FFFF);
let y = x + 5u64;
assert_eq!(y, VirtAddr::new(0xFFFF_8000_0000_0004));
assert_eq!(y - x, 0xFFFF_0000_0000_0005);
assert_ne!((x + 5u64) - x, 5u64);
// But subtraction doesn't "jump" the gap
let _ = y - 5u64; // This panics

which seems very unintuitive.

I would propose the following breaking change: VirtAddr::try_new and VirtAddr::new only succeed if the provided address is canonical and fail otherwise. This removes an unexpected foot-gun and simplifies the implementation.

[josephlr]

@phil-opp would you be OK with this breaking change for 0.15 ?

[phil-opp]

A fully agree that this behavior wasn't a good idea. Many people treat the lower and upper half as separate address spaces (e.g. user and kernel space) so that the boundary should not be crossing silently. I agree with your point about 5-level paging too.

So yes, I think we should make that breaking change. I normally try to avoid any breaking change that silently changes the runtime behavior, but in this case I don't see a better migration path. At least it only changes some behavior into errors, so all users that relied on the old behavior should notice the change at some point.

(There might be users that want to treat the virtual address space as one contiguous address space and want to keep the current gap jump behavior. In that case, we could add some separate jumping_add methods that expose the previous behavior. But let's wait if there is any interest in this at all.)

[josephlr]

(There might be users that want to treat the virtual address space as one contiguous address space and want to keep the current gap jump behavior. In that case, we could add some separate jumping_add methods that expose the previous behavior. But let's wait if there is any interest in this at all.)

Sounds reasonable to me.

[phil-opp]

I just left a comment in #293 that is related to this: https://github.com/rust-osdev/x86_64/issues/293#issuecomment-903260820

[josephlr]

Fixed by #370