Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition

[Andrew Cooper <amc96@xxxxxxxxx>]

On 25/09/2020 14:17, Jan Beulich wrote:
> On 22.09.2020 20:24, Andrew Cooper wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,81 @@ static int gnttab_get_shared_frame_mfn(struct domain 
>> *d,
>>      return 0;
>>  }
>>  
>> +int gnttab_acquire_resource(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    unsigned int i = nr_frames, tot_frames;
>> +    mfn_t tmp;
>> +    void **vaddrs = NULL;
>> +    int rc;
>> +
>> +    /* Input sanity. */
>> +    if ( !nr_frames )
>> +        return -EINVAL;
> I continue to object to this becoming an error.

It's not a path any legitimate caller will ever exercise.  POSIX defines
any mmap() of zero length to be an error, and I completely agree.

The problem isn't, per say, with accepting bogus arguments.  It is the
quantity of additional complexity in the hypervisor required to support
accepting the bogus input cleanly.

There are exactly 2 cases where 0 might be found here.  Either the
caller passed it in directly (and bypassed the POSIX check which would
reject the attempt), or some part of multi-layer continuation handling
went wrong on the previous iteration.

For this hypercall (by the end of the series), _acquire_resource()
returning 0 is specifically treated as an error so we don't livelock in
32-chunking loop until some other preemption kicks in.

In this case, the check isn't actually necessary because it is (will be)
guarded higher up the call chain in a more general way, but I'm not
interested in adding unnecessary extra complexity (to area I've had to
rewrite from scratch to remove the bugs) simply to support a
non-existent usecase.

~Andrew