Re: [PATCH v8 06/16] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2021 15:04, Andrew Cooper wrote:
> On 01/02/2021 13:03, Jan Beulich wrote:
>> On 01.02.2021 12:11, Andrew Cooper wrote:
>>> On 01/02/2021 10:10, Roger Pau Monné wrote:
>>>> On Sat, Jan 30, 2021 at 02:58:42AM +0000, Andrew Cooper wrote:
>>>>> @@ -636,15 +662,45 @@ int compat_memory_op(unsigned int cmd, 
>>>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>>>                      compat_frame_list[i] = frame;
>>>>>                  }
>>>>>  
>>>>> -                if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
>>>>> +                if ( __copy_to_compat_offset(cmp.mar.frame_list, 
>>>>> start_extent,
>>>>>                                               compat_frame_list,
>>>>> -                                             cmp.mar.nr_frames) )
>>>>> +                                             done) )
>>>>>                      return -EFAULT;
>>>> Is it fine to return with a possibly pending continuation already
>>>> encoded here?
>>>>
>>>> Other places seem to crash the domain when that happens.
>>> Hmm.  This is all a total mess.  (Elsewhere the error handling is also
>>> broken - a caller who receives an error can't figure out how to recover)
>>>
>>> But yes - I think you're right - the only thing we can do here is `goto
>>> crash;` and woe betide any 32bit kernel which passes a pointer to a
>>> read-only buffer.
>> I'd like to ask you to reconsider the "goto crash", both the one
>> you mention above and the other one already present in the patch.
>> Wiring all the cases where we mean to crash the guest into a
>> single domain_crash() invocation has the downside that when
>> observing such a case one can't remotely know which path has led
>> there. Therefore I'd like to suggest individual domain_crash()
>> invocations on every affected path. Elsewhere in the file there
>> already is such an instance, commented "Cannot cancel the
>> continuation...".
> 
> But they're all logically the same, are they not?

Depends on what "logically the same" here means. To me different
paths and different causes aren't necessarily "the same".

Jan