Re: [Xen-devel] [PATCHv2] xen/events: avoid race with raising an event in unmask_evtchn()

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 25 Mar 2013, David Vrabel wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> In unmask_evtchn(), when the mask bit is cleared after testing for
> pending and the event becomes pending between the test and clear, then
> the upcall will not become pending and the event may be lost or
> delayed.
> 
> Avoid this by always clearing the mask bit before checking for
> pending.  If a hypercall is needed, remask the event as
> EVTCHNOP_unmask will only retrigger pending events if they were
> masked.
> 
> This fixes a regression introduced in 3.7 by
> b5e579232d635b79a3da052964cb357ccda8d9ea (xen/events: fix
> unmask_evtchn for PV on HVM guests) which reordered the clear mask and
> check pending operations.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

It looks correct to me.
Assuming that you tested it with a PV on HVM guest:

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>



> Changes in v2:
> - set mask before hypercall.
> ---
>  drivers/xen/events.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index d17aa41..aa85881 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -403,11 +403,23 @@ static void unmask_evtchn(int port)
>  
>       if (unlikely((cpu != cpu_from_evtchn(port))))
>               do_hypercall = 1;
> -     else
> +     else {
> +             /*
> +              * Need to clear the mask before checking pending to
> +              * avoid a race with an event becoming pending.
> +              *
> +              * EVTCHNOP_unmask will only trigger an upcall if the
> +              * mask bit was set, so if a hypercall is needed
> +              * remask the event.
> +              */
> +             sync_clear_bit(port, BM(&s->evtchn_mask[0]));
>               evtchn_pending = sync_test_bit(port, BM(&s->evtchn_pending[0]));
>  
> -     if (unlikely(evtchn_pending && xen_hvm_domain()))
> -             do_hypercall = 1;
> +             if (unlikely(evtchn_pending && xen_hvm_domain())) {
> +                     sync_set_bit(port, BM(&s->evtchn_mask[0]));
> +                     do_hypercall = 1;
> +             }
> +     }
>  
>       /* Slow path (hypercall) if this is a non-local port or if this is
>        * an hvm domain and an event is pending (hvm domains don't have
> @@ -418,8 +430,6 @@ static void unmask_evtchn(int port)
>       } else {
>               struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
>  
> -             sync_clear_bit(port, BM(&s->evtchn_mask[0]));
> -
>               /*
>                * The following is basically the equivalent of
>                * 'hw_resend_irq'. Just like a real IO-APIC we 'lose
> -- 
> 1.7.2.5
> 

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