Re: [Xen-devel] [PATCH] viridian: fix assertion failure

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 26, 2018 at 02:54:54PM +0000, Paul Durrant wrote:
> Whilst attempting to crash an apparently wedged Windows domain using
> 'xen-hvmcrash' I managed to trigger the following ASSERT:
> 
> (XEN) Assertion '!vp->ptr' failed at viridian.c:607
> 
> with stack:
> 
> (XEN)    [<ffff82d08032c55d>] viridian_map_guest_page+0x1b4/0x1b6
> (XEN)    [<ffff82d08032b1db>] viridian_synic_load_vcpu_ctxt+0x39/0x3b
> (XEN)    [<ffff82d08032b90d>] viridian.c#viridian_load_vcpu_ctxt+0x93/0xcc
> (XEN)    [<ffff82d0803096d6>] hvm_load+0x10e/0x19e
> (XEN)    [<ffff82d080274c6d>] arch_do_domctl+0xb74/0x25b4
> (XEN)    [<ffff82d0802068ab>] do_domctl+0x16f7/0x19d8
> 
> This happened because viridian_map_guest_page() was not written to cope
> with being called multiple times, but this is unfortunately exactly what
> happens when xen-hvmcrash re-loads the domain context (having clobbered
> the values of RIP).
> 
> This patch simply makes viridian_map_guest_page() return immediately if it
> finds the page already mapped (i.e. vp->ptr != NULL).
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/viridian/viridian.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c 
> b/xen/arch/x86/hvm/viridian/viridian.c
> index 7d73f41de6..b99501eea2 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -604,7 +604,8 @@ void viridian_map_guest_page(struct vcpu *v, struct 
> viridian_page *vp)
>      unsigned long gmfn = vp->msr.fields.pfn;
>      struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
>  
> -    ASSERT(!vp->ptr);
> +    if ( vp->ptr )
> +        return;

Isn't this likely to get the page reference counting out of sync? You
will return without a put_page, and viridian_unmap_guest_page will set
vp->ptr = NULL and call put_page_and_type without taking into account
the reference count.

If viridian_map_guest_page needs to be called multiple times you
likely need some reference counting AFAICT, so that the last call to
viridian_unmap_guest_page is the one that releases the page and sets
vp->ptr = NULL.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel