Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/08/18 10:00, Paul Durrant wrote:
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index d9ec711c73..8e623ea08e 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -358,6 +358,12 @@ static inline unsigned int 
> grant_to_status_frames(unsigned int grant_frames)
>      return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE, 
> GRANT_STATUS_PER_PAGE);
>  }
>  
> +/* Number of grant table entries. Caller must hold d's gr. table lock.*/

Really? this is some straight arithmetic on the integer parameter.

> +static inline unsigned int status_to_grant_frames(unsigned int status_frames)
> +{
> +    return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE, 
> GRANT_PER_PAGE);
> +}
> +
>  /* Check if the page has been paged out, or needs unsharing.
>     If rc == GNTST_okay, *page contains the page struct with a ref taken.
>     Caller must do put_page(*page).
> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, 
> grant_ref_t ref,
>  }
>  #endif
>  
> +/* caller must hold write lock */
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> +                                       unsigned long idx, mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +
> +    ASSERT(gt->gt_version == 2);
> +
> +    if ( idx >= nr_status_frames(gt) )
> +    {
> +        unsigned long nr = status_to_grant_frames(idx + 1);

Why the +1 ? Won't that cause a failure if you attempt to map the
maximum valid index?

> +
> +        if ( nr <= gt->max_grant_frames )
> +            gnttab_grow_table(d, nr);

You want to capture the return value of grow_table(), which at least
distinguishes between ENODEV and ENOMEM.

> +
> +        if ( idx >= nr_status_frames(gt) )
> +            return -EINVAL;

This can probably(?) be asserted if gnttab_grow_table() returns
successfully.

> +    }
> +
> +    *mfn = _mfn(virt_to_mfn(gt->status[idx]));
> +    return 0;
> +}
> +
> +/* caller must hold write lock */
> +static int gnttab_get_shared_frame_mfn(struct domain *d,
> +                                       unsigned long idx, mfn_t *mfn)
> +{
> +    struct grant_table *gt = d->grant_table;
> +
> +    ASSERT(gt->gt_version != 0);
> +
> +    if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
> +        gnttab_grow_table(d, idx + 1);

Similarly wrt rc.

> +
> +    if ( idx >= nr_grant_frames(gt) )
> +        return -EINVAL;
> +
> +    *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
> +    return 0;
> +}
> +
>  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>                       mfn_t *mfn)
>  {
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index e29d596727..427f65a5e1 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -23,6 +23,7 @@
>  #include <xen/numa.h>
>  #include <xen/mem_access.h>
>  #include <xen/trace.h>
> +#include <xen/grant_table.h>
>  #include <asm/current.h>
>  #include <asm/hardirq.h>
>  #include <asm/p2m.h>
> @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
>  
> +static int acquire_grant_table(struct domain *d, unsigned int id,
> +                               unsigned long frame,
> +                               unsigned int nr_frames,
> +                               xen_pfn_t mfn_list[])
> +{
> +    unsigned int i = nr_frames;
> +
> +    /* Iterate backwards in case table needs to grow */
> +    while ( i-- != 0 )
> +    {
> +        mfn_t mfn = INVALID_MFN;
> +        int rc;
> +
> +        switch ( id )
> +        {
> +        case XENMEM_resource_grant_table_id_shared:
> +            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
> +            break;
> +
> +        case XENMEM_resource_grant_table_id_status:
> +            rc = gnttab_get_status_frame(d, frame + i, &mfn);
> +            break;
> +
> +        default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        if ( rc )

Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN) here?  I
don't think anything good will come of handing INVALID_MFN back to the
caller.

> +            return rc;
> +
> +        mfn_list[i] = mfn_x(mfn);
> +    }
> +
> +    return 0;
> +}
> +
>  static int acquire_resource(
>      XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
>  {
> @@ -1046,6 +1089,16 @@ static int acquire_resource(
>          xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
>          unsigned int i;
>  
> +        /*
> +         * FIXME: until foreign pages inserted into the P2M are properly
> +         *        reference counted, it is unsafe to allow mapping of
> +         *        non-caller-owned resource pages unless the caller is
> +         *        the hardware domain.
> +         */
> +        if (!(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&

Space.

> +            !is_hardware_domain(currd) )
> +            return -EOPNOTSUPP;

EPERM perhaps?

~Andrew

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