Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2020 16:50, Andrew Cooper wrote:
> On 22/09/2020 14:34, Jan Beulich wrote:
>> On 22.09.2020 15:10, Andrew Cooper wrote:
>>> On 29/07/2020 21:02, Jan Beulich wrote:
>>>> On 28.07.2020 13:37, Andrew Cooper wrote:
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -4013,6 +4013,72 @@ 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;
>>>>> +    void **vaddrs;
>>>>> +    int rc = 0;
>>>>> +
>>>>> +    /* Input sanity. */
>>>>> +    if ( !nr_frames )
>>>>> +        return -EINVAL;
>>>> I can't seem to be able to find an equivalent of this in the old logic,
>>>> and hence this looks like an unwarranted change in behavior to me. We
>>>> have quite a few hypercall ops where some count being zero is simply
>>>> a no-op, i.e. yielding success without doing anything.
>>> There is no possible circumstance when getting here requesting 0 is
>>> legitimate.
>>>
>>> Tolerating a zero input for a mapping request is a very expensive way of
>>> hiding caller bugs.
>> That's just one possible view.
> 
> ... that is fully enforced in the ecosystem we work in.
> 
> You can't get a 0 to here in the first place.

Would you mind pointing me at why this is? All I can see is

    if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
        return -E2BIG;

as far as the caller's argument checking goes here. And
acquire_grant_table(), which is what you're replacing, also is dealing
with zero quite fine afaics. As is arch_acquire_resource().

> However, when it comes to the XLAT and the chunking fixes, getting 0
> here is a hard error, and I'm not interested in breaking the that logic
> for a non-existent corner case.

I don't share this view. It's also interesting that you had to address
similar questions by Paul, isn't it?

Jan