On Tue, Nov 22, 2016 at 3:12 AM, Jan Beulich wrote: > >>> On 14.11.16 at 11:34, wrote: > > They need to be range checked against the current table limit in any > > event. > > > > Reported-by: Huawei PSIRT > > > > 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 > > 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 > > --- 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__ */ > >