search

riscv-software-src / riscv-pk

RISC-V Proxy Kernel
Other
579 stars 306 forks source link

pk: thwart an attempt from the compiler to optimize #261

Closed compnerd closed 2 years ago

compnerd commented 2 years ago

The memory manager maintains the first free page as the page after the _end synthetic emitted by the linker. This value is stored in a translation unit local variable. This value is only ever written to from init_early_alloc which is static and only ever invoked from pk_vm_init. Furthermore, the value that first_free_page is ever set to is computed as a rounding of the address of _end. Because the address of the symbol cannot change during execution of a normal program, this is effectively a constant, making the computed value a "constant" which can be re-materialized. Now, with the knowledge that the value is effectively a constant that can be re-materialized and the fact that the value is ever written to at a single position, we can simply re-materialize the value if it was ever changed in free_page_addr. This will allow the 8-byte value to be truncated to 1-byte.

Now, we can inline __early_pgalloc_align, and because the combination of __early_alloc and __early_pgalloc_align is small, we can inline that again at the two sites locally. This changes the __augment_page_freelist to re-materialize the constant when needed for the allocation.

The re-materialization however uses a pc-relative addressing, which now computes a different value than expected - the address has become a VA rather than a PA. This results in the address computed by free_page_addr (which is the result of the __early_pgalloc_align) to be a virtual address after the relocation, which then propagates through __early_alloc to the value in __augment_page_freelist, which is then consumed by __page_alloc, which will treat the now VA as a PA and perform an additional translation to a VA.

Mark the value as volatile to indicate that the value must be read at all points to thwart the size optimization of the compiler resulting in a mis-compilation resulting in the eventual invalid memory access during the memset that follows the allocation.

Thanks to @nzmichaelh for the help in tracking this down!

aswaterman commented 2 years ago

Sounds like y'all had fun tracking this one down! I've manually merged this via 1d6f1bd0126596a17f97581346d8cea78cb3b5e9, in which I moved the volatile to the initialization, rather than to every access. I think it makes the intent a bit clearer. You should double-check that it still works for you.

compnerd commented 2 years ago

Hmm, I think I agree with you that it is more clear that way - it is more explicit about what is volatile. Thanks for that tweak. I believe that it continues to behave properly in that variant as well.