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

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

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

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@citrix.com>
Cc: stable@vger.kernel.org
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
--
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

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

[Stefano Stabellini]

On Mon, 25 Mar 2013, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> 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@citrix.com>
> Cc: stable@vger.kernel.org
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>



> 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
>