diff mbox series

[02/13] xen/events: avoid removing an event channel while handling it

Message ID 20201103084150.8625-3-jgross@suse.com
State Superseded
Headers show
Series None | expand

Commit Message

Juergen Gross Nov. 3, 2020, 8:41 a.m. UTC 
Today it can happen that an event channel is being removed from the
system while the event handling loop is active. This can lead to a
race resulting in crashes or WARN() splats when trying to access the
irq_info structure related to the event channel.

Fix this problem by using a rwlock taken as reader in the event
handling loop and as writer when deallocating the irq_info structure.

As the observed problem was a NULL dereference in evtchn_from_irq()
make this function more robust against races by testing the irq_info
pointer to be not NULL before dereferencing it.

And finally make all accesses to evtchn_to_irq[row][col] atomic ones
in order to avoid seeing partial updates of an array element in irq
handling. Note that irq handling can be entered only for event channels
which have been valid before, so any not populated row isn't a problem
in this regard, as rows are only ever added and never removed.

This is XSA-331.

This is upstream commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2

Cc: stable@vger.kernel.org
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reported-by: Jinoh Kang <luke1337@theori.io>
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Wei Liu <wl@xen.org>
---
 drivers/xen/events/events_base.c | 41 ++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 5 deletions(-)

Comments

Pavel Machek Nov. 3, 2020, 1:15 p.m. UTC | #1 
Hi!

> Today it can happen that an event channel is being removed from the

> system while the event handling loop is active. This can lead to a

> race resulting in crashes or WARN() splats when trying to access the

> irq_info structure related to the event channel.

> 

> Fix this problem by using a rwlock taken as reader in the event

> handling loop and as writer when deallocating the irq_info structure.

> 

> As the observed problem was a NULL dereference in evtchn_from_irq()

> make this function more robust against races by testing the irq_info

> pointer to be not NULL before dereferencing it.

> 

> And finally make all accesses to evtchn_to_irq[row][col] atomic ones

> in order to avoid seeing partial updates of an array element in irq

> handling. Note that irq handling can be entered only for event channels

> which have been valid before, so any not populated row isn't a problem

> in this regard, as rows are only ever added and never removed.

> 

> This is XSA-331.

> 

> This is upstream commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2


This one is mismerged.


> @@ -1242,6 +1269,8 @@ static void __xen_evtchn_do_upcall(void)

>  	int cpu = get_cpu();

>  	unsigned count;

>  

> +	read_lock(&evtchn_rwlock);

> +

>  	do {

>  		vcpu_info->evtchn_upcall_pending = 0;

>  

> @@ -1256,6 +1285,8 @@ static void __xen_evtchn_do_upcall(void)

>  		__this_cpu_write(xed_nesting_count, 0);

>  	} while (count != 1 || vcpu_info->evtchn_upcall_pending);

>  

> +	read_unlock(&evtchn_rwlock);

> +

>  out:


read_unlock needs to be after the out: label. Or better yet, goto can
be avoided.

Best regards,
								Pavel
Signed-off-by: Pavel Machek (CIP) <pavel@denx.de>


diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index cef70f4b52ef..ba36bdd49d22 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1556,8 +1556,8 @@ static void __xen_evtchn_do_upcall(void)
 	do {
 		vcpu_info->evtchn_upcall_pending = 0;
 
-		if (__this_cpu_inc_return(xed_nesting_count) - 1)
-			goto out;
+		if (__this_cpu_inc_return(xed_nesting_count) != 1)
+			break;
 
 		xen_evtchn_handle_events(cpu, &ctrl);
 
@@ -1568,8 +1568,6 @@ static void __xen_evtchn_do_upcall(void)
 	} while (count != 1 || vcpu_info->evtchn_upcall_pending);
 
 	read_unlock(&evtchn_rwlock);
-
-out:
 	/*
 	 * Increment irq_epoch only now to defer EOIs only for
 	 * xen_irq_lateeoi() invocations occurring from inside the loop


-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Juergen Gross Nov. 3, 2020, 1:58 p.m. UTC | #2 
On 03.11.20 14:15, Pavel Machek wrote:
> Hi!

> 

>> Today it can happen that an event channel is being removed from the

>> system while the event handling loop is active. This can lead to a

>> race resulting in crashes or WARN() splats when trying to access the

>> irq_info structure related to the event channel.

>>

>> Fix this problem by using a rwlock taken as reader in the event

>> handling loop and as writer when deallocating the irq_info structure.

>>

>> As the observed problem was a NULL dereference in evtchn_from_irq()

>> make this function more robust against races by testing the irq_info

>> pointer to be not NULL before dereferencing it.

>>

>> And finally make all accesses to evtchn_to_irq[row][col] atomic ones

>> in order to avoid seeing partial updates of an array element in irq

>> handling. Note that irq handling can be entered only for event channels

>> which have been valid before, so any not populated row isn't a problem

>> in this regard, as rows are only ever added and never removed.

>>

>> This is XSA-331.

>>

>> This is upstream commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2

> 

> This one is mismerged.


Thanks for noticing!

Greg, do you want me to send the series again or only this patch?


Juergen

> 

> 

>> @@ -1242,6 +1269,8 @@ static void __xen_evtchn_do_upcall(void)

>>   	int cpu = get_cpu();

>>   	unsigned count;

>>   

>> +	read_lock(&evtchn_rwlock);

>> +

>>   	do {

>>   		vcpu_info->evtchn_upcall_pending = 0;

>>   

>> @@ -1256,6 +1285,8 @@ static void __xen_evtchn_do_upcall(void)

>>   		__this_cpu_write(xed_nesting_count, 0);

>>   	} while (count != 1 || vcpu_info->evtchn_upcall_pending);

>>   

>> +	read_unlock(&evtchn_rwlock);

>> +

>>   out:

> 

> read_unlock needs to be after the out: label. Or better yet, goto can

> be avoided.

> 

> Best regards,

> 								Pavel

> Signed-off-by: Pavel Machek (CIP) <pavel@denx.de>

> 

> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c

> index cef70f4b52ef..ba36bdd49d22 100644

> --- a/drivers/xen/events/events_base.c

> +++ b/drivers/xen/events/events_base.c

> @@ -1556,8 +1556,8 @@ static void __xen_evtchn_do_upcall(void)

>   	do {

>   		vcpu_info->evtchn_upcall_pending = 0;

>   

> -		if (__this_cpu_inc_return(xed_nesting_count) - 1)

> -			goto out;

> +		if (__this_cpu_inc_return(xed_nesting_count) != 1)

> +			break;

>   

>   		xen_evtchn_handle_events(cpu, &ctrl);

>   

> @@ -1568,8 +1568,6 @@ static void __xen_evtchn_do_upcall(void)

>   	} while (count != 1 || vcpu_info->evtchn_upcall_pending);

>   

>   	read_unlock(&evtchn_rwlock);

> -

> -out:

>   	/*

>   	 * Increment irq_epoch only now to defer EOIs only for

>   	 * xen_irq_lateeoi() invocations occurring from inside the loop

> 

>
Greg KH Nov. 3, 2020, 2:05 p.m. UTC | #3 
On Tue, Nov 03, 2020 at 02:58:11PM +0100, Jürgen Groß wrote:
> On 03.11.20 14:15, Pavel Machek wrote:

> > Hi!

> > 

> > > Today it can happen that an event channel is being removed from the

> > > system while the event handling loop is active. This can lead to a

> > > race resulting in crashes or WARN() splats when trying to access the

> > > irq_info structure related to the event channel.

> > > 

> > > Fix this problem by using a rwlock taken as reader in the event

> > > handling loop and as writer when deallocating the irq_info structure.

> > > 

> > > As the observed problem was a NULL dereference in evtchn_from_irq()

> > > make this function more robust against races by testing the irq_info

> > > pointer to be not NULL before dereferencing it.

> > > 

> > > And finally make all accesses to evtchn_to_irq[row][col] atomic ones

> > > in order to avoid seeing partial updates of an array element in irq

> > > handling. Note that irq handling can be entered only for event channels

> > > which have been valid before, so any not populated row isn't a problem

> > > in this regard, as rows are only ever added and never removed.

> > > 

> > > This is XSA-331.

> > > 

> > > This is upstream commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2

> > 

> > This one is mismerged.

> 

> Thanks for noticing!

> 

> Greg, do you want me to send the series again or only this patch?


Please resend the whole series, that's much easier for me than to try to
pick one out and replace it with another.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 83bb9fdbadc6..385974314102 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -32,6 +32,7 @@ 
 #include <linux/slab.h>
 #include <linux/irqnr.h>
 #include <linux/pci.h>
+#include <linux/spinlock.h>
 
 #ifdef CONFIG_X86
 #include <asm/desc.h>
@@ -70,6 +71,23 @@  const struct evtchn_ops *evtchn_ops;
  */
 static DEFINE_MUTEX(irq_mapping_update_lock);
 
+/*
+ * Lock protecting event handling loop against removing event channels.
+ * Adding of event channels is no issue as the associated IRQ becomes active
+ * only after everything is setup (before request_[threaded_]irq() the handler
+ * can't be entered for an event, as the event channel will be unmasked only
+ * then).
+ */
+static DEFINE_RWLOCK(evtchn_rwlock);
+
+/*
+ * Lock hierarchy:
+ *
+ * irq_mapping_update_lock
+ *   evtchn_rwlock
+ *     IRQ-desc lock
+ */
+
 static LIST_HEAD(xen_irq_list_head);
 
 /* IRQ <-> VIRQ mapping. */
@@ -104,7 +122,7 @@  static void clear_evtchn_to_irq_row(unsigned row)
 	unsigned col;
 
 	for (col = 0; col < EVTCHN_PER_ROW; col++)
-		evtchn_to_irq[row][col] = -1;
+		WRITE_ONCE(evtchn_to_irq[row][col], -1);
 }
 
 static void clear_evtchn_to_irq_all(void)
@@ -141,7 +159,7 @@  static int set_evtchn_to_irq(unsigned evtchn, unsigned irq)
 		clear_evtchn_to_irq_row(row);
 	}
 
-	evtchn_to_irq[row][col] = irq;
+	WRITE_ONCE(evtchn_to_irq[row][col], irq);
 	return 0;
 }
 
@@ -151,7 +169,7 @@  int get_evtchn_to_irq(unsigned evtchn)
 		return -1;
 	if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL)
 		return -1;
-	return evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)];
+	return READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]);
 }
 
 /* Get info for IRQ */
@@ -260,10 +278,14 @@  static void xen_irq_info_cleanup(struct irq_info *info)
  */
 unsigned int evtchn_from_irq(unsigned irq)
 {
-	if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
+	const struct irq_info *info = NULL;
+
+	if (likely(irq < nr_irqs))
+		info = info_for_irq(irq);
+	if (!info)
 		return 0;
 
-	return info_for_irq(irq)->evtchn;
+	return info->evtchn;
 }
 
 unsigned irq_from_evtchn(unsigned int evtchn)
@@ -447,16 +469,21 @@  static int __must_check xen_allocate_irq_gsi(unsigned gsi)
 static void xen_free_irq(unsigned irq)
 {
 	struct irq_info *info = info_for_irq(irq);
+	unsigned long flags;
 
 	if (WARN_ON(!info))
 		return;
 
+	write_lock_irqsave(&evtchn_rwlock, flags);
+
 	list_del(&info->list);
 
 	set_info_for_irq(irq, NULL);
 
 	WARN_ON(info->refcnt > 0);
 
+	write_unlock_irqrestore(&evtchn_rwlock, flags);
+
 	kfree(info);
 
 	/* Legacy IRQ descriptors are managed by the arch. */
@@ -1242,6 +1269,8 @@  static void __xen_evtchn_do_upcall(void)
 	int cpu = get_cpu();
 	unsigned count;
 
+	read_lock(&evtchn_rwlock);
+
 	do {
 		vcpu_info->evtchn_upcall_pending = 0;
 
@@ -1256,6 +1285,8 @@  static void __xen_evtchn_do_upcall(void)
 		__this_cpu_write(xed_nesting_count, 0);
 	} while (count != 1 || vcpu_info->evtchn_upcall_pending);
 
+	read_unlock(&evtchn_rwlock);
+
 out:
 
 	put_cpu();