Re: [Xen-devel] [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Jan,

I've updated the comments according to your previous suggestions,
do they look good to you?

2018-02-01 18:23 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>> On 30.01.18 at 18:50, <blackskygg@xxxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4126,6 +4126,10 @@ int xenmem_add_to_physmap_one(
>>          }
>>          case XENMAPSPACE_gmfn_foreign:
>>              return p2m_add_foreign(d, idx, gfn_x(gpfn), 
>> extra.foreign_domid);
>> +        case XENMAPSPACE_gmfn_share:
>> +            gdprintk(XENLOG_WARNING,
>> +                     "XENMAPSPACE_gmfn_share is currently not supported on 
>> x86\n");
>> +            break;
>
> Please don't - a hypervisor log message isn't really useful here. It
> should be the tool stack to disallow respective options on x86
> until that's implemented.
>
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -227,6 +227,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
>>                                        Stage-2 using the Normal Memory
>>                                        Inner/Outer Write-Back Cacheable
>>                                        memory attribute. */

/*
 * GMFN from another dom, but with different privilege requirements.
 *
 * Suppose that (c), the current domain, is trying to map pages from (t) into
 * (d). gmfn_share dosen't require that (d) has privilege over (t). This
 * enables some usecases such as dom0 trying to share memory pages between
 * two unprivileged guests, which will otherwise be impossible using
 * gmfn_foreign. This is XENMEM_add_to_physmap_batch only, and currently ARM
 * only.
 *
 * The exact XSM privilege requirements are as follows:
 *
 * ================== ============== ===================== =====================
 *                    (c) over (d)   (c) over (t)          (d) over (t)
 * ================== ============== ===================== =====================
 * _foreign (dummy)   XSM_TARGET     NO                    XSM_TARGET
 * _share (dummy)     XSM_TARGET     XSM_TARGET            NO
 * _foreign (flask)   MMU__PHYSMAP   NO                    MMU__MAP_READ/WRITE
 * _share (flask)     MMU__PHYSMAP   MMU__MAP_READ/WRITE   MMU__SHARE
 * ================== ============== ===================== =====================
 */
>> +#define XENMAPSPACE_gmfn_share   6 /* Same as *_gmfn_foreign, but this is
>> +                                      for a privileged dom to share pages
>> +                                      between two doms. */
>> +
>
> The comment doesn't make clear why XENMAPSPACE_gmfn_foreign
> then can't be used. In particular it is left open how _both_ domains
> would be specified.
>
> Also XENMAPSPACE_gmfn_foreign is restricted to
> XENMEM_add_to_physmap_batch (a comment says so) - how about
> this new one? According to the actual code changes you do, there's
> no meaningful difference, in which case the restriction should be
> named here as well.
>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -521,6 +521,12 @@ static XSM_INLINE int 
>> xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
>>      return xsm_default_action(action, d, t);
>>  }
>>
>> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, 
>> struct domain *t)
>
> Line length.
>
>> +{

  /*
   * This action also requires that @current targets @d, but it has already been
   * checked somewhere higher in the call stack.
   *
   * Be aware that this is not an exact default equivalence of its flask variant
   * which also checks if @d and @t "are allowed to share memory pages", for we
   * don't have a proper default equivalence of such a check.
   */
>> +    XSM_ASSERT_ACTION(XSM_TARGET);
>> +    return xsm_default_action(action, current->domain, t);
>
> How does this represent a proper default equivalent of ...
>
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, 
> struct domain *t)
>      return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
> MMU__MAP_WRITE);
>  }
>
> +static int flask_map_gmfn_share(struct domain *d, struct domain *t)
> +{
    /*
     * This action also requires that @current has MMU__MAP_READ/WRITE over @d,
     * but that has already been checked somewhere higher in the call stack (for
     * example, by flask_add_to_physmap()).
     */
> +    return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) 
> ?:
> +        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>
> ... this?


Cheers,

Zhongze Liu.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel