| Message ID | 5829A143020000780011E59D@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/11/16 10:34, Jan Beulich wrote: > --- 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; This will clobber the EINVAL/ENOENT cases. It wants to be pared with an !rc &&. With this fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > + 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;
>>> On 14.11.16 at 12:56, <andrew.cooper3@citrix.com> wrote: > On 14/11/16 10:34, Jan Beulich wrote: >> --- 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; > > This will clobber the EINVAL/ENOENT cases. It wants to be pared with an > !rc &&. Oh, indeed - thanks for noticing. > With this fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks, Jan
>>> On 14.11.16 at 11:34, <JBeulich@suse.com> wrote: > They need to be range checked against the current table limit in any > event. > > Reported-by: Huawei PSIRT <psirt@huawei.com> > > 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@suse.com> 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. > > --- 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__ */
On Tue, Nov 22, 2016 at 3:12 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 14.11.16 at 11:34, <JBeulich@suse.com> wrote: > > They need to be range checked against the current table limit in any > > event. > > > > Reported-by: Huawei PSIRT <psirt@huawei.com> > > > > 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@suse.com> > > 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@tklengyel.com> > > --- 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__ */ > >
>>> On 22.11.16 at 17:13, <tamas@tklengyel.com> wrote: > On Tue, Nov 22, 2016 at 3:12 AM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 14.11.16 at 11:34, <JBeulich@suse.com> wrote: >> > They need to be range checked against the current table limit in any >> > event. >> > >> > Reported-by: Huawei PSIRT <psirt@huawei.com> >> > >> > 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@suse.com> >> >> 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. This was never the case. I wouldn't dare to submit a patch knowingly breaking something. > 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@tklengyel.com> Thanks. Jan
On Tue, Nov 22, 2016 at 09:17:43AM -0700, Jan Beulich wrote: > >>> On 22.11.16 at 17:13, <tamas@tklengyel.com> wrote: > > On Tue, Nov 22, 2016 at 3:12 AM, Jan Beulich <JBeulich@suse.com> wrote: > > > >> >>> On 14.11.16 at 11:34, <JBeulich@suse.com> wrote: > >> > They need to be range checked against the current table limit in any > >> > event. > >> > > >> > Reported-by: Huawei PSIRT <psirt@huawei.com> > >> > > >> > 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@suse.com> > >> > >> 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. > > This was never the case. I wouldn't dare to submit a patch > knowingly breaking something. > > > 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@tklengyel.com> Release-acked-by: Wei Liu <wei.liu2@citrix.com>
--- 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__ */