Re: [PATCH v3 1/7] xen/gnttab: Rework resource acquisition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.01.2021 23:56, Andrew Cooper wrote:
> On 18/01/2021 08:23, Jan Beulich wrote:
>> On 15.01.2021 17:57, Andrew Cooper wrote:
>>> On 15/01/2021 11:56, Jan Beulich wrote:
>>>>> +    /* Grow table if necessary. */
>>>>> +    grant_write_lock(gt);
>>>>> +    rc = -EINVAL;
>>>>> +    switch ( id )
>>>>> +    {
>>>>> +    case XENMEM_resource_grant_table_id_shared:
>>>>> +        vaddrs = gt->shared_raw;
>>>>> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
>>>> ... this will degenerate (and still cause an error) when frame
>>>> is also zero, and will cause undue growing of the table when
>>>> frame is non-zero yet not overly large.
>>> Urgh, yes - that is why I had the check.
>>>
>>> In which case I retract my change between v2 and v3 here.
>>>
>>>> As indicated before, I'm of the clear opinion that here - like
>>>> elsewhere - a number of zero frames requested means that no
>>>> action be taken at all, and success be returned.
>>> The general world we work in (POSIX) agrees with my opinion over yours
>>> when it comes to this matter.
>> I assume you are referring to mmap()? I ask because I think there
>> are numerous counter examples (some even in the C standard):
>> malloc() & friends allow for either behavior. memcpy() / memmove()
>> ...
> 
> This entire infrastructure lives behind an mmap() (or equivalent) system
> call.  Other specs are not relevant.

With this you're implying restrictions on what callers might
use this for. I don't see why a ring-0 only environment
would necessarily have anything like mmap().

Anyway, I'm not going to further comment on any of the below.
I'm not going to either ack or nak this change, so if you
have the agreement of others feel free to put in.

Jan

> Any request for a zero sized mapping is a bug.  It is either a buggy
> caller, or buggy continuation logic.
> 
>>> I spent a lot of time and effort getting this logic correct in v2, and I
>>> do not have any further time to waste adding complexity to support a
>>> non-existent corner case, nor is it reasonable to further delay all the
>>> work which is depending on this series.  This entire mess is already too
>>> damn complicated, without taking extra complexity.
>>>
>>> Entertaining the idea of supporting 0 length requests is really not as
>>> simple as you seem to think it is, and is a large part of why I'm
>>> stubbornly refusing to do so.
>> I'd be really happy to be educated of the complications; sadly
>> so far you've only claimed ones would exist without actually
>> going into sufficient detail. In particular I don't view placing 
>>
>>     if ( size == 0 )
>>         return 0;
>>
>> suitably early coming anywhere near "complexity". Even more so
>> that as per your reply you mean to undo removal of a respective
>> check, just that in your version it'll return an error instead
>> of success.
> 
> I am not being a belligerent arse for the sake of it.  I've got far
> better things I ought to be doing with my time.
> 
> I spent a lot of time getting this working, and with sufficient testing
> to demonstrate it working in practice, particularly in continuation cases.
> 
> You've spent a lot of time and effort insisting that I reintroduce
> support a fundamentally broken corner case, despite my best attempts to
> demonstrate why it will livelock in the hypervisor because of the mess
> which is the double looping between the compat and native handlers.
> 
> You've also spent a lot of time obfuscating the overflow checks and
> breaking them in the process.
> 
> You are welcome, in your own time - and not mine, to use the testing
> infrastructure I've already provided to discover why the compat mess has
> a habit of livelocking in certain continuation cases.  (It won't
> actually livelock if you switch to using return 0.  You'll instead hit
> the ASSERT_UNREACHABLE() I put in a failsafe path specifically to avoid
> bugs in this are turning back into XSAs.)
> 
> Combined with the fact that 0 length requests are buggy in all
> circumstances, I chose to implement logic which is robust even in the
> case of failure, because the compat logic is almost intractable and
> borderline untestable because noone runs 32bit kernels in anger these
> days.  I can't commit my test infrastructure which spots the problems,
> because we obviously can't have a hypercall which panics when the input
> buffer doesn't match the test pattern.
> 
> I am not inclined to adopt an approach which is fundamentally more
> likely to contain security vulnerabilities in a fragile and untestable
> area of code.  You are going to have to come up with a far more
> compelling argument that "because I want to support 0 length mapping
> requests" to change my mind.
> 
> ~Andrew
>