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

[Andrew Cooper <amc96@cam.ac.uk>]

On 12/01/2021 08:15, Jan Beulich wrote:
> 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?

It's really not that simple.

I've dropped this particular hunk from v3 (as it is not necessary for
safety at this point in the series), but retained the equivalent logical
change in later patches where it does become necessary.

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

If you want to talk consistency, I'm fixing an inconsistency with this
change, not introducing one.

The existing logic will already yield EINVAL for addr==0, num !=0,
rather than writing the size back.

addr!=0, num==0 is the odd-combination-out.  Its not a useful input, and
won't be seen from legitimate callers, but can crop up from bugs in the
continuation logic.

~Andrew