Re: [PATCH 12/20] xen: avoid calling rcu_lock_*target_domain when an XSM hook exists

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

On 09/11/2012 03:36 AM, Jan Beulich wrote:
>>>> On 10.09.12 at 21:49, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -195,30 +195,6 @@ double_gt_unlock(struct grant_table *lgt, struct 
>> grant_table *rgt)
>>          spin_unlock(&rgt->lock);
>>  }
>>  
>> -static struct domain *gt_lock_target_domain_by_id(domid_t dom)
>> -{
>> -    struct domain *d;
>> -    int rc = GNTST_general_error;
>> -
>> -    switch ( rcu_lock_target_domain_by_id(dom, &d) )
>> -    {
>> -    case 0:
>> -        return d;
>> -
>> -    case -ESRCH:
>> -        gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom);
>> -        rc = GNTST_bad_domain;
>> -        break;
>> -
>> -    case -EPERM:
>> -        rc = GNTST_permission_denied;
>> -        break;
>> -    }
>> -
>> -    ASSERT(rc < 0 && -rc <= MAX_ERRNO);
>> -    return ERR_PTR(rc);
>> -}
>> -
> 
> Removing that function again is fine as long as you retain the
> distinguished error codes, which ...
> 
>>  static inline int
>>  __get_maptrack_handle(
>>      struct grant_table *t)
>> @@ -1304,11 +1280,12 @@ gnttab_setup_table(
>>          goto out1;
>>      }
>>  
>> -    d = gt_lock_target_domain_by_id(op.dom);
>> -    if ( IS_ERR(d) )
>> +    d = rcu_lock_domain_by_any_id(op.dom);
>> +    if ( d == NULL )
>>      {
>> -        op.status = PTR_ERR(d);
>> -        goto out1;
>> +        gdprintk(XENLOG_INFO, "Bad domid %d.\n", op.dom);
>> +        op.status = GNTST_bad_domain;
> 
> ... you don't.

Actually, I do. The only distinguishing error code here is
GNTST_permission_denied, which is now only triggered by the XSM
hook.

>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -570,7 +570,8 @@ long do_memory_op(unsigned long cmd, 
>> XEN_GUEST_HANDLE(void) arg)
>>               && (reservation.mem_flags & XENMEMF_populate_on_demand) )
>>              args.memflags |= MEMF_populate_on_demand;
>>  
>> -        if ( unlikely(rcu_lock_target_domain_by_id(reservation.domid, &d)) )
>> +        d = rcu_lock_domain_by_any_id(reservation.domid);
>> +        if ( d == NULL )
>>              return start_extent;
>>          args.domain = d;
>>  
>> @@ -619,9 +620,9 @@ long do_memory_op(unsigned long cmd, 
>> XEN_GUEST_HANDLE(void) arg)
>>          if ( copy_from_guest(&domid, arg, 1) )
>>              return -EFAULT;
>>  
>> -        rc = rcu_lock_target_domain_by_id(domid, &d);
>> -        if ( rc )
>> -            return rc;
>> +        d = rcu_lock_domain_by_any_id(domid);
>> +        if ( d == NULL )
>> +            return -ESRCH;
> 
> And quite similarly here and further down you're losing -EPERM.
> 
> Jan
> 
Same logic: rcu_lock_domain_by_any_id will succeed where -EPERM was
returned by rcu_lock_target_domain_by_id; the check is moved to the
XSM hook.

-- 
Daniel De Graaf
National Security Agency