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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.12 at 16:33, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> On 09/11/2012 10:16 AM, Jan Beulich wrote:
>>>>> On 11.09.12 at 15:40, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>> On 09/11/2012 03:55 AM, Jan Beulich wrote:
>>>>>>> On 10.09.12 at 21:49, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> 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.
> 
> 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.

That's true for the pin-page case in do_mmuext_op(), but not
for any other of its sub-ops (particularly not for the TLB
flushing ones, for which I can't even see how XSM denying the
operation could be useful in any way, the more that presently
foreigndom/pg_owner is being ignored for many of these, i.e.
they act locally even if an otherwise valid foreign domain was
specified).

> 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.

Indeed - if the per-request check makes no sense, then dropping
that one would be the preferred route over the one you took
here. Same for do_mmu_update().

Jan


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