Re: [Xen-devel] xc_hvm_inject_trap() races

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/08/2016 11:39 AM, Razvan Cojocaru wrote:
> On 11/08/2016 11:19 AM, Razvan Cojocaru wrote:
>> On 11/08/2016 10:15 AM, Jan Beulich wrote:
>>>>>> On 07.11.16 at 18:01, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> On 11/07/2016 06:10 PM, Jan Beulich wrote:
>>>>>>>> On 07.11.16 at 16:24, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>> The one-shot vm_event does sound reasonable. I could set a flag
>>>>>> per-VCPU, basically similar to v->arch.hvm_vcpu.inject_trap.vector, and
>>>>>> fire a single event from hvm_inject_trap() if it's set (then unset it) -
>>>>>> the flag would be set via an xc_monitor_next_interrupt() call in libxc.
>>>>>
>>>>> Doing this in hvm_inject_trap() would not cover all cases afict.
>>>>> I'd suggest doing this from hvm_do_resume() _after_ the
>>>>> (conditional) call to hvm_inject_trap(), if there is _any_ event
>>>>> pending.
>>>>
>>>> But that would only cover the hypercall-injected traps. The condition in
>>>> hvm_do_resume() is "if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )",
>>>> and inject_trap.vector seems to only ever be set by the hypercall:
>>>> [...]
>>>> So if the next interrupt is not caused by the hypercall, we'll never get
>>>> another event. Am I reading the code wrong?
>>>
>>> No, maybe I expressed myself ambiguously: I meant to say that the
>>> event should be delivered from hvm_do_resume(), but _outside_ the
>>> conditional guarding the call to hvm_inject_trap(). Otherwise things
>>> would have been worse than when doing it inside hvm_inject_trap().
>>
>> While working on this patch, I've had a new idea, which would require
>> less changes and fix the problem in a more elegant manner if validated.
>> Looking at vmx_idtv_reinject(), the real problem seems to be that
>> VM_ENTRY_INTR_INFO is being written to directly:
>>
>> 3229 static void vmx_idtv_reinject(unsigned long idtv_info)
>> 3230 {
>> 3231
>> 3232     /* Event delivery caused this intercept? Queue for redelivery. */
>> 3233     if ( unlikely(idtv_info & INTR_INFO_VALID_MASK) )
>> 3234     {
>> 3235         if ( hvm_event_needs_reinjection(MASK_EXTR(idtv_info,
>> 3236
>> INTR_INFO_INTR_TYPE_MASK),
>> 3237                                          idtv_info &
>> INTR_INFO_VECTOR_MASK) )
>> 3238         {
>> 3239             /* See SDM 3B 25.7.1.1 and .2 for info about masking
>> resvd bits. */
>> 3240             __vmwrite(VM_ENTRY_INTR_INFO,
>> 3241                       idtv_info & ~INTR_INFO_RESVD_BITS_MASK);
>> 3242             if ( idtv_info & INTR_INFO_DELIVER_CODE_MASK )
>> 3243             {
>> 3244                 unsigned long ec;
>> 3245
>> 3246                 __vmread(IDT_VECTORING_ERROR_CODE, &ec);
>> 3247                 __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, ec);
>> 3248             }
>> 3249         }
>> 3250
>> 3251         /*
>> 3252          * Clear NMI-blocking interruptibility info if an NMI
>> delivery faulted.
>> 3253          * Re-delivery will re-set it (see SDM 3B 25.7.1.2).
>> 3254          */
>> 3255         if ( cpu_has_vmx_vnmi &&
>> 3256              ((idtv_info & INTR_INFO_INTR_TYPE_MASK) ==
>> 3257               MASK_INSR(X86_EVENTTYPE_NMI, INTR_INFO_INTR_TYPE_MASK)) )
>> 3258         {
>> 3259             unsigned long intr_info;
>> 3260
>> 3261             __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_info);
>> 3262             __vmwrite(GUEST_INTERRUPTIBILITY_INFO,
>> 3263                       intr_info & ~VMX_INTR_SHADOW_NMI);
>> 3264         }
>> 3265     }
>> 3266 }
>>
>> where the hypercall looks at v->arch.hvm_vcpu.inject_trap.vector only.
>> Then we notice that the hypercall _fails_immediately_ with -EBUSY if
>> v->arch.hvm_vcpu.inject_trap.vector is already set:
>>
>> 5922     case HVMOP_inject_trap:
>> 5923     {
>> 5924         xen_hvm_inject_trap_t tr;
>> 5925         struct domain *d;
>> 5926         struct vcpu *v;
>> 5927
>> 5928         if ( copy_from_guest(&tr, arg, 1 ) )
>> 5929             return -EFAULT;
>> 5930
>> 5931         rc = rcu_lock_remote_domain_by_id(tr.domid, &d);
>> 5932         if ( rc != 0 )
>> 5933             return rc;
>> 5934
>> 5935         rc = -EINVAL;
>> 5936         if ( !is_hvm_domain(d) )
>> 5937             goto injtrap_fail;
>> 5938
>> 5939         rc = xsm_hvm_control(XSM_DM_PRIV, d, op);
>> 5940         if ( rc )
>> 5941             goto injtrap_fail;
>> 5942
>> 5943         rc = -ENOENT;
>> 5944         if ( tr.vcpuid >= d->max_vcpus || (v = d->vcpu[tr.vcpuid])
>> == NULL )
>> 5945             goto injtrap_fail;
>> 5946
>> 5947         if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
>> 5948             rc = -EBUSY;
> 
> Actually the fix should be even simpler than that: we can add to this if
> " || hvm_event_pending(v)".
> 
> Objections?

Nevermind, vmx_event_pending() has a fair ASSERT that v == curr:

1789 static int vmx_event_pending(struct vcpu *v)
1790 {
1791     unsigned long intr_info;
1792
1793     ASSERT(v == current);
1794     __vmread(VM_ENTRY_INTR_INFO, &intr_info);
1795
1796     return intr_info & INTR_INFO_VALID_MASK;
1797 }

The inject_trap.vector solution seems to be the only plausible one.
Sorry for the spam.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel