Re: [Xen-devel] [PATCH] x86/vm_event: fix race between vmx_vmexit_handler() and vm_event_resume()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.17 at 09:22, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> The introspection agent can reply to a vm_event faster than
> vmx_vmexit_handler() can complete in some cases, where it is then
> not safe for vm_event_set_registers() to modify v->arch.user_regs.

This needs more explanation: I cannot see the connection between
vmx_vmexit_handler() completing and (un)safety of modification of
v->arch.user_regs. The latter is unsafe as long as the vCPU hasn't
gone through __context_switch(), and the former doesn't call that
function directly or indirectly (i.e. I think it is more than just
vmx_vmexit_handler() which needs to be completed by the time
register modification becomes safe to do).

> This patch ensures that vm_event_resume() code only sets per-VCPU
> data to be used for the actual setting of registers only in
> hvm_do_resume() (similar to the model used to control setting of CRs
> and MSRs).

I think the second "only" would better be "later".

> The patch additionally removes the sync_vcpu_execstate(v) call from
> vm_event_resume(), which is no longer necessary, which removes the
> associated broadcast TLB flush (read: performance improvement).

Depending on the better explanation above, it may or may not be
further necessary to also better explain the "no longer necessary"
part here.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -473,6 +473,39 @@ static bool hvm_get_pending_event(struct vcpu *v, struct 
> x86_event *info)
>      return hvm_funcs.get_pending_event(v, info);
>  }
>  
> +static void hvm_vm_event_set_registers(struct vcpu *v)

This could be const, as *v itself isn't being modified, but maybe it's
better to leave it non-const indeed.

> +{
> +    ASSERT(v == current);
> +
> +    if ( v->arch.vm_event->set_gprs )

I think we will want an unlikely() here. We anyway can only hope for
the compiler to always inline this function, such that non-VM-event
setups don't get penalized by the extra call here. Strictly speaking
the function doesn't belong into this file ...

> +    {
> +        struct cpu_user_regs *regs = guest_cpu_user_regs();
> +
> +        regs->rax = v->arch.vm_event->gprs.rax;
> +        regs->rbx = v->arch.vm_event->gprs.rbx;
> +        regs->rcx = v->arch.vm_event->gprs.rcx;
> +        regs->rdx = v->arch.vm_event->gprs.rdx;
> +        regs->rsp = v->arch.vm_event->gprs.rsp;
> +        regs->rbp = v->arch.vm_event->gprs.rbp;
> +        regs->rsi = v->arch.vm_event->gprs.rsi;
> +        regs->rdi = v->arch.vm_event->gprs.rdi;
> +
> +        regs->r8 = v->arch.vm_event->gprs.r8;
> +        regs->r9 = v->arch.vm_event->gprs.r9;
> +        regs->r10 = v->arch.vm_event->gprs.r10;
> +        regs->r11 = v->arch.vm_event->gprs.r11;
> +        regs->r12 = v->arch.vm_event->gprs.r12;
> +        regs->r13 = v->arch.vm_event->gprs.r13;
> +        regs->r14 = v->arch.vm_event->gprs.r14;
> +        regs->r15 = v->arch.vm_event->gprs.r15;
> +
> +        regs->rflags = v->arch.vm_event->gprs.rflags;
> +        regs->rip = v->arch.vm_event->gprs.rip;
> +
> +        v->arch.vm_event->set_gprs = 0;

false (and true/bool elsewhere)

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -357,6 +357,13 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>  {
>      vm_event_response_t rsp;
>  
> +    /*
> +     * vm_event_resume() runs either from XEN_DOMCTL_VM_EVENT_OP_*, or
> +     * EVTCHN_send from the introspection consumer.  Both contexts are
> +     * guaranteed not to be the subject of vm_event responses.
> +     */
> +    ASSERT(d != current->domain);

What is this meant to guard against? It surely isn't ...

> @@ -375,13 +382,6 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>          v = d->vcpu[rsp.vcpu_id];
>  
>          /*
> -         * Make sure the vCPU state has been synchronized for the custom
> -         * handlers.
> -         */
> -        if ( atomic_read(&v->vm_event_pause_count) )
> -            sync_vcpu_execstate(v);

... a "replacement" for this, as the state of "current" doesn't reflect
whether register state has been saved (that's this_cpu(curr_vcpu)
instead). Nor would a comparison of domains seem to be the right
thing - a comparison of vcpus ought to suffice (and be less strict,
allowing for something hypothetical like self-introspection).

Jan

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