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

Jan


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