Xen project Mailing List

On Tue, Nov 22, 2016 at 3:12 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 14.11.16 at 11:34, <JBeulich@xxxxxxxx> wrote:
> They need to be range checked against the current table limit in any
> event.
>
> Reported-by: Huawei PSIRT <psirt@xxxxxxxxxx>
>
> Move the code to where it belongs, eliminating a number of duplicate
> definitions. Add locking. Produce proper error codes, and consume them
> instead of making one up. Check grant type. Convert parameter types at
> once.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Tamas? (The minor fix needed to address Andrew's reply doesn't seem
to warrant sending out a v2.)

> ---
> Note that likely there's more work needed subsequently: The grant isn't
> being marked in use for the duration of the use of the GFN. But I'll
> leave that to someone actually knowing how to properly to test this.

Hi Jan,

unfortunately I don't have a good way to test this either as I never used memsharing with grefs before. The above comment about the grant not being marked for in-use makes me wonder whether this is a regression from this patch or whether that just was never the case. Either way, I can see this being an issue only if memory is being removed by hot-plugging, which AFAIK is not a supported scenario anyway. The rest of the patch is fairly mechanical, so:

Acked-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>

> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -753,75 +753,25 @@ int mem_sharing_debug_gfn(struct domain
> return num_refs;
> }
>
> -#define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
> -#define shared_entry_v1(t, e) \
> - ((t)->shared_v1[(e)/SHGNT_PER_PAGE_V1][(e)%SHGNT_PER_PAGE_V1])
> -#define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t))
> -#define shared_entry_v2(t, e) \
> - ((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2])
> -#define STGNT_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
> -#define status_entry(t, e) \
> - ((t)->status[(e)/STGNT_PER_PAGE][(e)%STGNT_PER_PAGE])
> -
> -static grant_entry_header_t *
> -shared_entry_header(struct grant_table *t, grant_ref_t ref)
> -{
> - ASSERT (t->gt_version != 0);
> - if ( t->gt_version == 1 )
> - return (grant_entry_header_t*)&shared_entry_v1(t, ref);
> - else
> - return &shared_entry_v2(t, ref).hdr;
> -}
> -
> -static int mem_sharing_gref_to_gfn(struct domain *d,
> - grant_ref_t ref,
> - unsigned long *gfn)
> -{
> - if ( d->grant_table->gt_version < 1 )
> - return -1;
> -
> - if ( d->grant_table->gt_version == 1 )
> - {
> - grant_entry_v1_t *sha1;
> - sha1 = &shared_entry_v1(d->grant_table, ref);
> - *gfn = sha1->frame;
> - }
> - else
> - {
> - grant_entry_v2_t *sha2;
> - sha2 = &shared_entry_v2(d->grant_table, ref);
> - *gfn = sha2->full_page.frame;
> - }
> -
> - return 0;
> -}
> -
> -
> int mem_sharing_debug_gref(struct domain *d, grant_ref_t ref)
> {
> - grant_entry_header_t *shah;
> + int rc;
> uint16_t status;
> - unsigned long gfn;
> + gfn_t gfn;
>
> - if ( d->grant_table->gt_version < 1 )
> + rc = mem_sharing_gref_to_gfn(d->grant_table, ref, &gfn, &status);
> + if ( rc )
> {
> - MEM_SHARING_DEBUG(
> - "Asked to debug [dom=%d,gref=%d], but not yet inited.\n",
> - d->domain_id, ref);
> - return -EINVAL;
> - }
> - (void)mem_sharing_gref_to_gfn(d, ref, &gfn);
> - shah = shared_entry_header(d->grant_table, ref);
> - if ( d->grant_table->gt_version == 1 )
> - status = shah->flags;
> - else
> - status = status_entry(d->grant_table, ref);
> + MEM_SHARING_DEBUG("Asked to debug [dom=%d,gref=%u]: error %d.\n",
> + d->domain_id, ref, rc);
> + return rc;
> + }
>
> MEM_SHARING_DEBUG(
> "==> Grant [dom=%d,ref=%d], status=%x. ",
> d->domain_id, ref, status);
>
> - return mem_sharing_debug_gfn(d, gfn);
> + return mem_sharing_debug_gfn(d, gfn_x(gfn));
> }
>
> int mem_sharing_nominate_page(struct domain *d,
> @@ -1422,23 +1372,24 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_P
> case XENMEM_sharing_op_nominate_gref:
> {
> grant_ref_t gref = mso.u.nominate.u.grant_ref;
> - unsigned long gfn;
> + gfn_t gfn;
> shr_handle_t handle;
>
> rc = -EINVAL;
> if ( !mem_sharing_enabled(d) )
> goto out;
> - if ( mem_sharing_gref_to_gfn(d, gref, &gfn) < 0 )
> + rc = mem_sharing_gref_to_gfn(d->grant_table, gref, &gfn, NULL);
> + if ( rc < 0 )
> goto out;
>
> - rc = mem_sharing_nominate_page(d, gfn, 3, &handle);
> + rc = mem_sharing_nominate_page(d, gfn_x(gfn), 3, &handle);
> mso.u.nominate.handle = handle;
> }
> break;
>
> case XENMEM_sharing_op_share:
> {
> - unsigned long sgfn, cgfn;
> + gfn_t sgfn, cgfn;
> struct domain *cd;
> shr_handle_t sh, ch;
>
> @@ -1470,35 +1421,38 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_P
> grant_ref_t gref = (grant_ref_t)
> (XENMEM_SHARING_OP_FIELD_GET_GREF(
> mso.u.share.source_gfn));
> - if ( mem_sharing_gref_to_gfn(d, gref, &sgfn) < 0 )
> + rc = mem_sharing_gref_to_gfn(d->grant_table, gref, &sgfn,
> + NULL);
> + if ( rc < 0 )
> {
> rcu_unlock_domain(cd);
> - rc = -EINVAL;
> goto out;
> }
> - } else {
> - sgfn = mso.u.share.source_gfn;
> }
> + else
> + sgfn = _gfn(mso.u.share.source_gfn);
>
> if ( XENMEM_SHARING_OP_FIELD_IS_GREF(mso.u.share.client_gfn) )
> {
> grant_ref_t gref = (grant_ref_t)
> (XENMEM_SHARING_OP_FIELD_GET_GREF(
> mso.u.share.client_gfn));
> - if ( mem_sharing_gref_to_gfn(cd, gref, &cgfn) < 0 )
> + rc = mem_sharing_gref_to_gfn(cd->grant_table, gref, &cgfn,
> + NULL);
> + if ( rc < 0 )
> {
> rcu_unlock_domain(cd);
> - rc = -EINVAL;
> goto out;
> }
> - } else {
> - cgfn = mso.u.share.client_gfn;
> }
> + else
> + cgfn = _gfn(mso.u.share.client_gfn);
>
> sh = mso.u.share.source_handle;
> ch = mso.u.share.client_handle;
>
> - rc = mem_sharing_share_pages(d, sgfn, sh, cd, cgfn, ch);
> + rc = mem_sharing_share_pages(d, gfn_x(sgfn), sh,
> + cd, gfn_x(cgfn), ch);
>
> rcu_unlock_domain(cd);
> }
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3438,6 +3438,53 @@ void grant_table_init_vcpu(struct vcpu *
> v->maptrack_tail = MAPTRACK_TAIL;
> }
>
> +#ifdef CONFIG_HAS_MEM_SHARING
> +int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> + gfn_t *gfn, uint16_t *status)
> +{
> + int rc = 0;
> + uint16_t flags = 0;
> +
> + grant_read_lock(gt);
> +
> + if ( gt->gt_version < 1 )
> + rc = -EINVAL;
> + else if ( ref >= nr_grant_entries(gt) )
> + rc = -ENOENT;
> + else if ( gt->gt_version == 1 )
> + {
> + const grant_entry_v1_t *sha1 = &shared_entry_v1(gt, ref);
> +
> + flags = sha1->flags;
> + *gfn = _gfn(sha1->frame);
> + }
> + else
> + {
> + const grant_entry_v2_t *sha2 = &shared_entry_v2(gt, ref);
> +
> + flags = sha2->hdr.flags;
> + if ( flags & GTF_sub_page )
> + *gfn = _gfn(sha2->sub_page.frame);
> + else
> + *gfn = _gfn(sha2->full_page.frame);
> + }
> +
> + if ( (flags & GTF_type_mask) != GTF_permit_access )
> + rc = -ENXIO;
> + else if ( !rc && status )
> + {
> + if ( gt->gt_version == 1 )
> + *status = flags;
> + else
> + *status = status_entry(gt, ref);
> + }
> +
> + grant_read_unlock(gt);
> +
> + return rc;
> +}
> +#endif
> +
> static void gnttab_usage_print(struct domain *rd)
> {
> int first = 1;
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -149,4 +149,7 @@ static inline unsigned int grant_to_stat
> GRANT_STATUS_PER_PAGE;
> }
>
> +int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> + gfn_t *gfn, uint16_t *status);
> +
> #endif /* __XEN_GRANT_TABLE_H__ */

Re: [Xen-devel] Ping: [PATCH] x86/memshr: properly check grant references