Re: [Xen-devel] [PATCH v2 4/6] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Jan,

Thanks for having a look at the patch.

2017-08-28 16:29 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>> On 27.08.17 at 10:36, <blackskygg@xxxxxxxxx> wrote:
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
>>              return -EINVAL;
>>          }
>>
>> -        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
>> +        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
>>          if ( rc )
>>          {
>>              rcu_unlock_domain(od);
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long 
>> fgfn,
>>      if ( tdom == fdom )
>>          goto out;
>>
>> -    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
>> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);
>
> I continue to dislike the added arguments here, as being pointless
> to pass. I'm not the maintainer of either of the modified files, so I
> won't (and can't) veto the change though.
>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -525,10 +525,14 @@ static XSM_INLINE int 
>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>      return xsm_default_action(action, d1, d2);
>>  }
>>
>> -static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 
>> *d, struct domain *t)
>> +static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 
>> *cd,
>> +                                           struct domain *d, struct domain 
>> *t)
>>  {
>> +    int rc;
>>      XSM_ASSERT_ACTION(XSM_TARGET);
>
> Missing blank line between declaration and statements.

Sorry. Will fix this.

>
>> -    return xsm_default_action(action, d, t);
>> +    rc = xsm_default_action(action, cd, d);
>> +    if (rc) return rc;
>
> Coding style. In any event, as suggested before the whole thing is
> easier to write as
>
>> +    return xsm_default_action(action, cd, t);
>
>     return xsm_default_action(action, cd, d) ?: xsm_default_action(action, 
> cd, t);

But aren't we going to preserve the error code here?

Cheers,

Zhongze Liu

>
> anyway imo (suitably split across lines if needed, of course).
>
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -1165,9 +1165,15 @@ static int flask_remove_from_physmap(struct domain 
>> *d1, struct domain *d2)
>>      return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
>>  }
>>
>> -static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
>> +static int flask_map_gmfn_foreign(struct domain *cd,
>> +                                  struct domain *d, struct domain *t)
>>  {
>> -    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
>> MMU__MAP_WRITE);
>> +    int rc;
>> +    rc = domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | 
>> MMU__MAP_WRITE);
>> +    if (rc) return rc;
>> +    rc = domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | 
>> MMU__MAP_WRITE);
>> +    if (rc) return rc;
>> +    return domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>>  }
>
> At least the style problems mentioned above apply here too.
>
> Jan
>

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