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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 26 November 2018 15:32
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Jan Beulich <jbeulich@xxxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> Subject: Re: [PATCH] viridian: fix assertion failure
> 
> On Mon, Nov 26, 2018 at 03:08:50PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne
> > > Sent: 26 November 2018 15:04
> > > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Jan Beulich <jbeulich@xxxxxxxx>;
> > > Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>
> > > Subject: Re: [PATCH] viridian: fix assertion failure
> > >
> > > 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.
> > >
> >
> > Damn. Missed that the side effect was in the initializer. I'll need to
> move it below the check.
> >
> > > 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.
> >
> > I shouldn't need to reference count. When the MSR is set by the guest
> there is always an unmap then a map. It is just the context load case that
> should only map on the first call (to handle resume correctly).
> 
> OK, I haven't looked at the usage, but if there's a paired number of
> maps/unmaps adding a reference counter to viridian_page is fairly
> trivial and would make the implementation of map/unmap always correct
> regardless of it's usage.
> 

No, maps and unmaps are not paired. You can have an arbitrary number of maps, 
but only one unmap. Anyway, v2 coming in a minute.

  Paul

> Thanks, Roger.

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