On Tue, Nov 22, 2016 at 3:12 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 14.11.16 at 11:34, <jbeul...@suse.com> wrote:
> > They need to be range checked against the current table limit in any
> > event.
> >
> > Reported-by: Huawei PSIRT <ps...@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 <jbeul...@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 <ta...@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__ */
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
