Re: [Xen-devel] [PATCH] arm/mem_access: properly handle traps caused by no-longer current settings

[Julien Grall <julien.grall@xxxxxxx>]

On 02/08/2016 22:34, Tamas K Lengyel wrote:
> On Tue, Aug 2, 2016 at 2:02 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hello Tamas,
> >
> > Thank you for taking care of this bug.
> >
> > On 02/08/2016 19:53, Tamas K Lengyel wrote:
> > > When mem_access settings change, the active vCPUs may still cause a
> > > violation
> > > until the TLB gets flushed. Instead of just reinjecting the violation to
> > > the
> > > guest, in this patch we direct the vCPU to retry the access where
> > > appropriate or we crash the domain where the mem_access settings are
> > > corrupted.
> > With this patch p2m_mem_access_check will always return false. So I am not
> > sure why you want to return in p2m_mem_access_check.
> That's not the case, it returns true if mem_access is not enabled on
> the domain, which means whatever caused the trap wasn't mem_access and
> thus we should fall back on the default behavior, which is injecting
> the fault to the guest.
>
> > > Requested-by: Julien Grall <julien.grall@xxxxxxx>
> > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> > > ---
> > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Cc: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > >  xen/arch/arm/p2m.c | 29 ++++++++++++++++++++++++++---
> > >  1 file changed, 26 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index 40a0b80..a4b6b7b 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -1657,8 +1657,26 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t
> > > gla, const struct npfec npfec)
> > >          return true;
> > >
> > >      rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), &xma);
> > > -    if ( rc )
> > > -        return true;
> > > +    switch (rc )
> > > +    {
> > > +    case -ESRCH:
> > > +        /*
> > > +         * If we can't find any mem_access setting for this page then the
> > > page
> > > +         * might have just been removed and the event was triggered by no
> > > longer
> > > +         * valid settings. The vCPU should just retry to get to the
> > > proper error
> > > +         * path.
> > > +         */
> > > +        return false;
> > > +    case -ERANGE:
> > > +        /*
> > > +         * The mem_access settings are corrupted. Crashing the domain is
> > > the
> > > +         * appropriate step in this case.
> > > +         */
> > > +        domain_crash(v->domain);
> > > +        return false;
> > > +    };
> > > +
> > > +    ASSERT(!rc);
> >
> > It would be easier to do:
> >
> > rc = p2m_mem_access_check(gpa, gva, npfec);
> > if (!rc)
> >   return;
> >
> > by
> >
> > p2m_mem_access_check(gpa, gva, npfec);
> > return;
> >
> > in both do_trap_instr_abort_guest and do_trap_data_abort_guest.
> >
> > This would also helps to fallback on another permission check if in the
> > future we decide to use permission for other reasons.
> >
> > Or is there any reason you may want to inject a data abort to the guest if
> > memaccess has failed (i.e return true)?
> Yes, if the fault wasn't caused by mem_access (ie. it's not enabled on
> the domain).
Well, the data abort can only be a permission fault if memaccess is 
inuse so far. Unless there is another race condition in the memaccess 
code and in this case this is not the fault of the guest. So sending a 
data abort to the guest will not really help to know what's going on.
Also, you are assuming that it will never be possible in the future to 
have another usage of the permission fault. By returning false you say 
"I handled the fault, it is not necessary to hand over to someone else".
The right thing here is:
        1) Try to handle memaccess
        2) Re-execute the instruction

The instruction will fault again if it was really a permission issue. 
Otherwise it will normally be executed.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel