[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.