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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 09 August 2018 09:47
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v22 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> >>> On 08.08.18 at 16:16, <paul.durrant@xxxxxxxxxx> wrote:
> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > +                                       unsigned long idx, mfn_t *mfn)
> > +{
> > +    const struct grant_table *gt = d->grant_table;
> > +
> > +    ASSERT(gt->gt_version == 2);
> > +
> > +    if ( idx >= nr_status_frames(gt) )
> > +    {
> > +        unsigned long nr_status;
> > +        unsigned long nr_grant;
> > +
> > +        nr_status = idx + 1; /* sufficient frames to make idx valid */
> > +        nr_grant = status_to_grant_frames(nr_status);
> > +
> > +        if ( nr_grant <= nr_grant_frames(gt) ) /* overflow check */
> > +            return -EINVAL;
> 
> If the table is currently empty, this would always fail, wouldn't
> it? You haven't grown the table yet by this point.

Good point, I should be checking nr_status not nr_grant.

> 
> > +        if ( nr_grant <= gt->max_grant_frames )
> > +            gnttab_grow_table(d, nr_grant);
> 
> And here (other than originally in gnttab_map_frame()) you
> invoke gnttab_grow_table() perhaps pointlessly (when the table
> doesn't in fact need growing).

Well if idx is out of range (which is how the code got in here) then the table 
must need to grow.

> 
> > @@ -1027,6 +1066,11 @@ static int acquire_resource(
> >
> >      switch ( xmar.type )
> >      {
> > +    case XENMEM_resource_grant_table:
> > +        rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
> > +                                 mfn_list);
> > +        break;
> > +
> >      default:
> >          rc = arch_acquire_resource(d, xmar.type, xmar.id, xmar.frame,
> >                                     xmar.nr_frames, mfn_list, &xmar.flags);
> > @@ -1046,6 +1090,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) &&
> > +             !is_hardware_domain(currd) )
> > +            return -EOPNOTSUPP;
> > +
> 
> Now that I look at this again - wouldn't this check better live ahead
> of the main switch()? I find it odd, for example, that in this case the
> grant table would still have got grown.

This can't live ahead of the main switch because XENMEM_rsrc_acq_caller_owned 
is passed-out flag, not a passed-in one. 
Also, the grant table would have potentially grown even though the op will 
fail. Is that what's concerning you?

  Paul

> 
> Jan
> 


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