Re: [PATCH for-4.5 v11 5/9] xen/arm: Allow hypervisor access to mem_access protected pages

[Julien Grall <julien.grall@linaro.org>]

On 09/29/2014 03:44 PM, Tamas K Lengyel wrote:
>     > +    if ( rc < 0 )
>     > +        return rc;
>     > +
>     > +    /*
>     > +     * We do this first as this is faster in the default case when no
>     > +     * permission is set on the page.
>     > +     */
>     > +    rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
>     > +    if ( rc < 0 )
>     > +        return rc;
>     > +
>     > +    /* Let's check if mem_access limited the access. */
>     > +    switch ( xma )
>     > +    {
>     > +    default:
>     > +    case XENMEM_access_rwx:
> 
>     access_rwx is used to say there is no permission, right? If so, why
>     don't you continue to check permission?
> 
> 
> So things are backward here. There has already been a MMU fault
> (get_page_from_gva failed) and we are trying to determine the cause of
> that fault. If the mem_access permission is rwx or rw, that means it had
> nothing to do with it so we go back to the original path.

This is very confusing. As we should never go there, I would add an
ASSERT to catch buggy implementation.

>     > +    ASSERT(*page);
> 
>     mfn_to_page only returns a valid pointer if the MFN is valid (see
>     mfn_valid).
> 
> 
> Hm, I copied this from get_page_from_gva but I'll remove it.

I meant, you forgot to copy the if ( !mfn_valid(mfn) ) return -EFAULT;

But the ASSERT is pointless as the function may return an invalid
pointer which is not NULL.

Regards,

-- 
Julien Grall