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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.08.18 at 10:55, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 09 August 2018 09:47
>> 
>> >>> 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.

Oh, sorry - I've ignored the outer if().

>> > @@ -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. 

Oh, right. Except that only arch_acquire_resource() could currently
set the flag, and hence from this patch's perspective it's not visible
that this is an "out" flag. I guess you mean to set the flag in
acquire_grant_table() or next to the call to it.

> Also, the grant table would have potentially grown even though the op will 
> fail. Is that what's concerning you?

But a failure post-grow other than the one above is relatively unlikely,
isn't it?

Jan



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