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

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

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).
>
> Jan
>

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.

~Andrew