Re: [PATCH v2 for-4.14] x86/vmx: use P2M_ALLOC in vmx_load_pdptrs instead of P2M_UNSHARE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.06.2020 18:19, Tamas K Lengyel wrote:
> While forking VMs running a small RTOS system (Zephyr) a Xen crash has been
> observed due to a mm-lock order violation while copying the HVM CPU context
> from the parent. This issue has been identified to be due to
> hap_update_paging_modes first getting a lock on the gfn using get_gfn. This
> call also creates a shared entry in the fork's memory map for the cr3 gfn. The
> function later calls hap_update_cr3 while holding the paging_lock, which
> results in the lock-order violation in vmx_load_pdptrs when it tries to 
> unshare
> the above entry when it grabs the page with the P2M_UNSHARE flag set.
> 
> Since vmx_load_pdptrs only reads from the page its usage of P2M_UNSHARE was
> unnecessary to start with. Using P2M_ALLOC is the appropriate flag to ensure
> the p2m is properly populated and to avoid the lock-order violation we
> observed.

Using P2M_ALLOC is not going to address the original problem though
afaict: You may hit the mem_sharing_fork_page() path that way, and
via nominate_page() => __grab_shared_page() => mem_sharing_page_lock()
you'd run into a lock order violation again.

The change is an improvement, so I'd be fine with it going in this
way, but only as long as the description mentions that there's still
an open issue here (which may be non-trivial to address). Or perhaps
combining with your v1 change is the way to go (for now or even
permanently)?

Jan