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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2021 22:22, Andrew Cooper wrote:
> 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.

Is there any, besides s/-EINVAL/0/ for the line above?

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

In a couple of cases you've been calling out the principle of least
surprise. This holds for the entirety of batched (in whatever form)
hypercalls then as well. Either zero elements means "no-op"
everywhere, or it gets treated as an error everywhere. Anything
else is inconsistent and hence possibly surprising.

To be clear - if others agree with your view, I'm not meaning to
NAK the change in this form. But I'm also not going to knowingly
ack introduction of inconsistencies.

Jan