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

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

On 10/09/14 02:32, Chao Peng wrote:
> On Tue, Sep 09, 2014 at 02:15:59PM +0100, Jan Beulich wrote:
>>>>> On 09.09.14 at 14:44, <andrew.cooper3@citrix.com> wrote:
>>> 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.
>>> 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?)
>> They can - current->mc_state.flags has _MCSF_in_multicall
>> set.
>>
>>> and certainly shouldn't distinguish between a regular hypercall
>>> and multicall.
>> I agree with this. Yet it's a bug in the caller to request no
>> preemption at this layer for a constituent hypercall that can itself
>> preempt. But that's only a problem for the caller, not for the
>> hypervisor.
>>
>>> 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.
>> Certainly not - call_list is being copied over a second time a few
>> lines after that NDEBUG section.
>>
>> Jan
>>
> Clear, thank you two for your discussion.
> So the only thing need to be done here is fixing the potential security
> issue, right? I will follow this.
>
> Thanks,
> Chao

Actually, on further thought, using multicalls like this cannot possibly
be correct from a functional point of view.

Even with the no preempt flag between a wrmsr/rdmsr hypercall pair,
there is no guarantee that accesses to remote cpus msrs won't interleave
with a different natural access, clobbering the results of the wrmsr.

However this is solved, the wrmsr/rdmsr pair *must* be part of the same
synchronous thread of execution on the appropriate cpu.  You can trust
that interrupts won't play with these msrs, but you absolutely can't
guarantee that IPI/wrmsr/IPI/rdmsr will work.

~Andrew