Re: [PATCH v15 01/11] multicall: add no preemption ability between two calls

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/09/14 12:51, Jan Beulich wrote:
>>>> On 09.09.14 at 12:51, <andrew.cooper3@citrix.com> wrote:
>> On 09/09/14 11:39, Jan Beulich wrote:
>>>>>> On 09.09.14 at 08:43, <chao.p.peng@linux.intel.com> wrote:
>>>> On Fri, Sep 05, 2014 at 11:46:20AM +0100, Andrew Cooper wrote:
>>>>> On 05/09/14 09:37, Chao Peng wrote:
>>>>>> Add a flag to indicate if the execution can be preempted between two
>>>>>> calls. If not specified, stay preemptable.
>>>>>>
>>>>>> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
>>>>>> ---
>>>>>>  xen/common/multicall.c   |    5 ++++-
>>>>>>  xen/include/public/xen.h |    4 ++++
>>>>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
>>>>>> index fa9d910..83b96eb 100644
>>>>>> --- a/xen/common/multicall.c
>>>>>> +++ b/xen/common/multicall.c
>>>>>> @@ -40,6 +40,7 @@ do_multicall(
>>>>>>      struct mc_state *mcs = &current->mc_state;
>>>>>>      uint32_t         i;
>>>>>>      int              rc = 0;
>>>>>> +    bool_t           preemptable = 0;
>>>>>>  
>>>>>>      if ( unlikely(__test_and_set_bit(_MCSF_in_multicall, &mcs->flags)) )
>>>>>>      {
>>>>>> @@ -52,7 +53,7 @@ do_multicall(
>>>>>>  
>>>>>>      for ( i = 0; !rc && i < nr_calls; i++ )
>>>>>>      {
>>>>>> -        if ( i && hypercall_preempt_check() )
>>>>>> +        if ( preemptable && hypercall_preempt_check() )
>>>>>>              goto preempted;
>>>>>>  
>>>>>>          if ( unlikely(__copy_from_guest(&mcs->call, call_list, 1)) )
>>>>>> @@ -61,6 +62,8 @@ do_multicall(
>>>>>>              break;
>>>>>>          }
>>>>>>  
>>>>>> +        preemptable = mcs->call.flags & MC_NO_PREEMPT;
>>>>>> +
>>>>> Please consider what would happen if a malicious guest set NO_PREEMPT on
>>>>> every multicall entry.
>>>> OK, I see. My direct purpose here is to support batch operations for
>>>> XENPF_resource_op added in next patch. Recall what Jan suggested in v14
>>>> comments, we have 3 possible ways to support XENPF_resource_op batch:
>>>> 1) Add a field in the xenpf_resource_op to indicate the iteration;
>>>> 2) Fiddle multicall mechanism, just like this patch;
>>>> 3) Add a brand new hypercall.
>>>>
>>>> So perhaps I will give up option 2) before I can see any improvement
>>>> here. While option 3) is aggressive, so I'd go option 1) through I also
>>>> don't quite like it (Totally because the iteration is transparent for user).
>>> The I suppose you didn't really understand Andrew's comment: I
>>> don't think he was suggesting to drop the approach, but instead
>>> to implement it properly (read: securely).
>> That is certainly one part of it.
>>
>> However, there is the other open question (dropped from this context) of
>> how to deal with a multicall which has NO_PREEMT set, which itself
>> preempts, and I don't have a good answer for this.
> The pretty natural answer to this is - the specific handler knows
> best what to do.
>
> Jan
>

Given our past history at retrofitting preempting into existing
hypercalls, the multicaller has no idea whether the ops they have
selected will preempt or not, and no way to guarentee that the behaviour
will stay the same in future.

The multicall dispatches to the regular hypercall handlers, which
(cant?) and certainly shouldn't distinguish between a regular hypercall
and multicall.

As I have been looking through this code, I have noticed that the NDEBUG
parameter corruption will break half of our existing preemption logic,
which does use some of the parameters to hold preemption information.

~Andrew