Re: [PATCH 17/20] arch/x86: use XSM hooks for get_pg_owner access checks

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 09/11/2012 10:16 AM, Jan Beulich wrote:
>>>> On 11.09.12 at 15:40, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> On 09/11/2012 03:55 AM, Jan Beulich wrote:
>>>>>> On 10.09.12 at 21:49, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -2882,11 +2882,6 @@ static struct domain *get_pg_owner(domid_t domid)
>>>>          pg_owner = rcu_lock_domain(dom_io);
>>>>          break;
>>>>      case DOMID_XEN:
>>>> -        if ( !IS_PRIV(curr) )
>>>> -        {
>>>> -            MEM_LOG("Cannot set foreign dom");
>>>> -            break;
>>>> -        }
>>>>          pg_owner = rcu_lock_domain(dom_xen);
>>>>          break;
>>>>      default:
>>>> @@ -2895,12 +2890,6 @@ static struct domain *get_pg_owner(domid_t domid)
>>>>              MEM_LOG("Unknown domain '%u'", domid);
>>>>              break;
>>>>          }
>>>> -        if ( !IS_PRIV_FOR(curr, pg_owner) )
>>>> -        {
>>>> -            MEM_LOG("Cannot set foreign dom");
>>>> -            rcu_unlock_domain(pg_owner);
>>>> -            pg_owner = NULL;
>>>> -        }
>>>>          break;
>>>>      }
>>>>  
>>>> @@ -3008,6 +2997,13 @@ long do_mmuext_op(
>>>>          goto out;
>>>>      }
>>>>  
>>>> +    rc = xsm_mmuext_op(d, pg_owner);
>>>> +    if ( rc )
>>>> +    {
>>>> +        rcu_unlock_domain(pg_owner);
>>>> +        goto out;
>>>> +    }
>>>> +
>>>
>>> While this part is fine, ...
>>>
>>>>      for ( i = 0; i < count; i++ )
>>>>      {
>>>>          if ( hypercall_preempt_check() )
>>>> @@ -3483,11 +3479,6 @@ long do_mmu_update(
>>>>              rc = -EINVAL;
>>>>              goto out;
>>>>          }
>>>> -        if ( !IS_PRIV_FOR(d, pt_owner) )
>>>> -        {
>>>> -            rc = -ESRCH;
>>>> -            goto out;
>>>> -        } 
>>>
>>> ... this one isn't (at least in conjunction with them all becoming
>>> indirect calls unconditionally) - you replace a single validation per
>>> set of requests with one validation per request.
>>
>> Is it still a problem if the check is inlined? If so, I could add an
>> additional XSM hook where the old IS_PRIV check was done, and make the
>> check inside the loop an inlined noop in the XSM-disabled case.
> 
> It's not a problem for the inlined case I would say, but I do
> think that performance here matters even if XSM is enabled.
> 
> Jan
> 

That's a problem that should probably be addressed in a different patch,
since the current XSM hook is already implemented as one per request.
This is because it needs access to the page being mapped in order to check
for MMIO mappings. Since those are not the normal case, I think this could
be improved using a lazier hook - or by dropping the per-request XSM check,
because I think it might be re-implementing the existing mmio rangeset
checks, and should just rely on those.

-- 
Daniel De Graaf
National Security Agency