linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
@ 2021-06-11 13:54 irqchip-bot for Marc Zyngier
  2021-07-05 17:23 ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: irqchip-bot for Marc Zyngier @ 2021-06-11 13:54 UTC (permalink / raw)
  To: linux-kernel; +Cc: Marc Zyngier, tglx

The following commit has been merged into the irq/irqchip-next branch of irqchip:

Commit-ID:     d4a45c68dc81f9117ceaff9f058d5fae674181b9
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Mon, 05 Apr 2021 12:57:27 +01:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00

irqdomain: Protect the linear revmap with RCU

It is pretty odd that the radix tree uses RCU while the linear
portion doesn't, leading to potential surprises for the users,
depending on how the irqdomain has been created.

Fix this by moving the update of the linear revmap under
the mutex, and the lookup under the RCU read-side lock.

The mutex name is updated to reflect that it doesn't only
cover the radix-tree anymore.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/irqdomain.h |  5 ++--
 kernel/irq/irqdomain.c    | 49 +++++++++++++++++---------------------
 2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 340cc04..2b696c9 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -151,6 +151,7 @@ struct irq_domain_chip_generic;
  * Revmap data, used internally by irq_domain
  * @revmap_size: Size of the linear map table @revmap[]
  * @revmap_tree: Radix map tree for hwirqs that don't fit in the linear map
+ * @revmap_mutex: Lock for the revmap
  * @revmap: Linear table of irq_data pointers
  */
 struct irq_domain {
@@ -173,8 +174,8 @@ struct irq_domain {
 	irq_hw_number_t hwirq_max;
 	unsigned int revmap_size;
 	struct radix_tree_root revmap_tree;
-	struct mutex revmap_tree_mutex;
-	struct irq_data *revmap[];
+	struct mutex revmap_mutex;
+	struct irq_data __rcu *revmap[];
 };
 
 /* Irq domain flags */
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 7a4e388..8fbadee 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -213,7 +213,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size,
 
 	/* Fill structure */
 	INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL);
-	mutex_init(&domain->revmap_tree_mutex);
+	mutex_init(&domain->revmap_mutex);
 	domain->ops = ops;
 	domain->host_data = host_data;
 	domain->hwirq_max = hwirq_max;
@@ -504,13 +504,12 @@ static void irq_domain_clear_mapping(struct irq_domain *domain,
 	if (irq_domain_is_nomap(domain))
 		return;
 
-	if (hwirq < domain->revmap_size) {
-		domain->revmap[hwirq] = NULL;
-	} else {
-		mutex_lock(&domain->revmap_tree_mutex);
+	mutex_lock(&domain->revmap_mutex);
+	if (hwirq < domain->revmap_size)
+		rcu_assign_pointer(domain->revmap[hwirq], NULL);
+	else
 		radix_tree_delete(&domain->revmap_tree, hwirq);
-		mutex_unlock(&domain->revmap_tree_mutex);
-	}
+	mutex_unlock(&domain->revmap_mutex);
 }
 
 static void irq_domain_set_mapping(struct irq_domain *domain,
@@ -520,13 +519,12 @@ static void irq_domain_set_mapping(struct irq_domain *domain,
 	if (irq_domain_is_nomap(domain))
 		return;
 
-	if (hwirq < domain->revmap_size) {
-		domain->revmap[hwirq] = irq_data;
-	} else {
-		mutex_lock(&domain->revmap_tree_mutex);
+	mutex_lock(&domain->revmap_mutex);
+	if (hwirq < domain->revmap_size)
+		rcu_assign_pointer(domain->revmap[hwirq], irq_data);
+	else
 		radix_tree_insert(&domain->revmap_tree, hwirq, irq_data);
-		mutex_unlock(&domain->revmap_tree_mutex);
-	}
+	mutex_unlock(&domain->revmap_mutex);
 }
 
 static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
@@ -911,12 +909,12 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
 		return 0;
 	}
 
+	rcu_read_lock();
 	/* Check if the hwirq is in the linear revmap. */
 	if (hwirq < domain->revmap_size)
-		return domain->revmap[hwirq]->irq;
-
-	rcu_read_lock();
-	data = radix_tree_lookup(&domain->revmap_tree, hwirq);
+		data = rcu_dereference(domain->revmap[hwirq]);
+	else
+		data = radix_tree_lookup(&domain->revmap_tree, hwirq);
 	rcu_read_unlock();
 	return data ? data->irq : 0;
 }
@@ -1499,18 +1497,17 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 	if (irq_domain_is_nomap(d->domain))
 		return;
 
+	/* Fix up the revmap. */
+	mutex_lock(&d->domain->revmap_mutex);
 	if (d->hwirq < d->domain->revmap_size) {
 		/* Not using radix tree */
-		d->domain->revmap[d->hwirq] = d;
-		return;
+		rcu_assign_pointer(d->domain->revmap[d->hwirq], d);
+	} else {
+		slot = radix_tree_lookup_slot(&d->domain->revmap_tree, d->hwirq);
+		if (slot)
+			radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
 	}
-
-	/* Fix up the revmap. */
-	mutex_lock(&d->domain->revmap_tree_mutex);
-	slot = radix_tree_lookup_slot(&d->domain->revmap_tree, d->hwirq);
-	if (slot)
-		radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
-	mutex_unlock(&d->domain->revmap_tree_mutex);
+	mutex_unlock(&d->domain->revmap_mutex);
 }
 
 /**

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
  2021-06-11 13:54 [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU irqchip-bot for Marc Zyngier
@ 2021-07-05 17:23 ` Guenter Roeck
  2021-07-05 18:01   ` Marc Zyngier
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-07-05 17:23 UTC (permalink / raw)
  To: linux-kernel; +Cc: Marc Zyngier, tglx

Hi,

On Fri, Jun 11, 2021 at 01:54:36PM -0000, irqchip-bot for Marc Zyngier wrote:
> The following commit has been merged into the irq/irqchip-next branch of irqchip:
> 
> Commit-ID:     d4a45c68dc81f9117ceaff9f058d5fae674181b9
> Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9
> Author:        Marc Zyngier <maz@kernel.org>
> AuthorDate:    Mon, 05 Apr 2021 12:57:27 +01:00
> Committer:     Marc Zyngier <maz@kernel.org>
> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00
> 
> irqdomain: Protect the linear revmap with RCU
> 
> It is pretty odd that the radix tree uses RCU while the linear
> portion doesn't, leading to potential surprises for the users,
> depending on how the irqdomain has been created.
> 
> Fix this by moving the update of the linear revmap under
> the mutex, and the lookup under the RCU read-side lock.
> 
> The mutex name is updated to reflect that it doesn't only
> cover the radix-tree anymore.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>

This patch results in various RCU warnings when booting mipsel images
in qemu. I can not revert the patch due to subsequent changes, so I
don't know if a simple revert fixes the problem. Log messages and
bisect log see below.

Guenter

---

...
=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------
include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
no locks held by swapper/0/0.

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000000 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
        8209fe9c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209fe38 820fcec0
        00000000 00000000 80ecfc64 8209fcc0 fffffbff 00000148 00000000 00000000
        00000000 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 00000000
        80fb0000 80faea08 00000000 80fb1858 00000000 00000000 00000000 81100000
        ...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801a4164>] lock_acquire+0x418/0x498
[<801c0d80>] __irq_resolve_mapping+0x60/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8


=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------
include/linux/rcupdate.h:688 rcu_read_lock() used illegally while idle!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
 #0: 80fd43cc (rcu_read_lock){....}-{1:2}, at: __irq_resolve_mapping+0x28/0x1f0

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000080 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
        8209ff1c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209feb8 820fcec0
        00000000 00000000 80ecfc64 8209fd40 fffffbff 00000169 00000000 00000000
        81349a50 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 80fd0000
        81730000 80fb0000 00000080 80fb1858 00000000 00000000 00000000 81100000
        ...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801c0ee8>] __irq_resolve_mapping+0x1c8/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8


=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------
kernel/irq/irqdomain.c:920 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
 #0: 80fd43cc (rcu_read_lock){....}-{1:2}, at: __irq_resolve_mapping+0x28/0x1f0

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000080 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
        8209ff1c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209feb8 820fcec0
        00000000 00000000 80ecfc64 8209fd40 fffffbff 00000189 00000000 00000000
        81349a50 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 80fd0000
        81730000 80fb0000 00000080 80fb1858 00000000 00000000 00000000 81100000
        ...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801c0e68>] __irq_resolve_mapping+0x148/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8


=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------
include/linux/rcupdate.h:716 rcu_read_unlock() used illegally while idle!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
 #0: 80fd43cc (rcu_read_lock){....}-{1:2}, at: __irq_resolve_mapping+0x28/0x1f0

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000080 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
        8209ff1c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209feb8 820fcec0
        00000000 00000000 80ecfc64 8209fd40 fffffbff 000001a9 00000000 00000000
        81349a50 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 80fd0000
        81730000 80fb0000 00000080 80fb1858 00000000 00000000 00000000 81100000
        ...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801c0ea8>] __irq_resolve_mapping+0x188/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8


=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------
include/trace/events/lock.h:58 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
 #0: 80fd3c4c ((console_sem).lock){-...}-{2:2}, at: down_trylock+0x18/0x4c

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 817924dc 8209fcfc 817924f0 00001fec 80ee11b8 80fb0000 81061e40 8071e768
        00000000 3bdd5c9d 80fb0000 810f9930 000001ec 00000001 8209fc98 820fcec0
        00000000 00000000 80ecfc64 00000001 0000000a 00000003 00000001 6d6f4320
        00000000 817926e9 817944dc 00000001 80fb0000 00000001 80ecfc64 81061e20
        80faea08 80e92708 00000000 80fb0000 00000000 00000000 00000000 81100000
        ...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801a834c>] lock_release+0x3d0/0x46c
[<80c6080c>] _raw_spin_unlock_irqrestore+0x24/0xa0
[<80198854>] down_trylock+0x34/0x4c
[<801aef18>] __down_trylock_console_sem+0x44/0x120
[<801af370>] console_trylock+0x1c/0x88
[<801b2ccc>] vprintk_emit+0x124/0x404
[<801b2fcc>] vprintk_default+0x20/0x2c
[<80c3e6ac>] printk+0x34/0x58
[<80c3d884>] lockdep_rcu_suspicious+0x40/0x114
[<801a4164>] lock_acquire+0x418/0x498
[<801c0d80>] __irq_resolve_mapping+0x60/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8

---
bisect log:

# bad: [007b350a58754a93ca9fe50c498cc27780171153] Merge tag 'dlm-5.14' of git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm
# good: [62fb9874f5da54fdb243003b386128037319b219] Linux 5.13
git bisect start '007b350a5875' '62fb9874f5da'
# good: [36824f198c621cebeb22966b5e244378fa341295] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
git bisect good 36824f198c621cebeb22966b5e244378fa341295
# good: [122fa8c588316aacafe7e5a393bb3e875eaf5b25] Merge tag 'for-5.14-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
git bisect good 122fa8c588316aacafe7e5a393bb3e875eaf5b25
# bad: [3563f55ce65462063543dfa6a8d8c7fbfb9d7772] Merge tag 'pm-5.14-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect bad 3563f55ce65462063543dfa6a8d8c7fbfb9d7772
# good: [c54b245d011855ea91c5beff07f1db74143ce614] Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace
git bisect good c54b245d011855ea91c5beff07f1db74143ce614
# bad: [21edf50948728f55b685ad95f196ba46196eb767] Merge tag 'irq-core-2021-06-29' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad 21edf50948728f55b685ad95f196ba46196eb767
# bad: [c64638d5091a5630d0f5f5ab7001bdee1ad194f2] Merge branch irq/generic_handle_domain_irq-core into irq/irqchip-next
git bisect bad c64638d5091a5630d0f5f5ab7001bdee1ad194f2
# bad: [d4a45c68dc81f9117ceaff9f058d5fae674181b9] irqdomain: Protect the linear revmap with RCU
git bisect bad d4a45c68dc81f9117ceaff9f058d5fae674181b9
# good: [13a9a5d17d07cec8181ea0843674ce48c191628e] powerpc: Add missing linux/{of.h,irqdomain.h} include directives
git bisect good 13a9a5d17d07cec8181ea0843674ce48c191628e
# good: [1da027362a7db422243601e895e6f8288389f435] irqdomain: Reimplement irq_linear_revmap() with irq_find_mapping()
git bisect good 1da027362a7db422243601e895e6f8288389f435
# good: [4f86a06e2d6ece5316e4c42fbf946ee22acb30f3] irqdomain: Make normal and nomap irqdomains exclusive
git bisect good 4f86a06e2d6ece5316e4c42fbf946ee22acb30f3
# good: [48b15a7921d60680babe59f64e127816585a585c] irqdomain: Cache irq_data instead of a virq number in the revmap
git bisect good 48b15a7921d60680babe59f64e127816585a585c
# first bad commit: [d4a45c68dc81f9117ceaff9f058d5fae674181b9] irqdomain: Protect the linear revmap with RCU

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
  2021-07-05 17:23 ` Guenter Roeck
@ 2021-07-05 18:01   ` Marc Zyngier
  2021-07-05 18:23     ` Guenter Roeck
  2021-07-08  8:00   ` [irqchip: irq/irqchip-fixes] irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry irqchip-bot for Marc Zyngier
  2021-07-09  9:23   ` irqchip-bot for Marc Zyngier
  2 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2021-07-05 18:01 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-kernel, tglx

Hi Guenter,

On Mon, 05 Jul 2021 18:23:52 +0100,
Guenter Roeck <linux@roeck-us.net> wrote:
> 
> Hi,
> 
> On Fri, Jun 11, 2021 at 01:54:36PM -0000, irqchip-bot for Marc Zyngier wrote:
> > The following commit has been merged into the irq/irqchip-next branch of irqchip:
> > 
> > Commit-ID:     d4a45c68dc81f9117ceaff9f058d5fae674181b9
> > Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9
> > Author:        Marc Zyngier <maz@kernel.org>
> > AuthorDate:    Mon, 05 Apr 2021 12:57:27 +01:00
> > Committer:     Marc Zyngier <maz@kernel.org>
> > CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00
> > 
> > irqdomain: Protect the linear revmap with RCU
> > 
> > It is pretty odd that the radix tree uses RCU while the linear
> > portion doesn't, leading to potential surprises for the users,
> > depending on how the irqdomain has been created.
> > 
> > Fix this by moving the update of the linear revmap under
> > the mutex, and the lookup under the RCU read-side lock.
> > 
> > The mutex name is updated to reflect that it doesn't only
> > cover the radix-tree anymore.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> This patch results in various RCU warnings when booting mipsel images
> in qemu. I can not revert the patch due to subsequent changes, so I
> don't know if a simple revert fixes the problem. Log messages and
> bisect log see below.

Thanks for the heads up. Do you have a config file I can use to
reproduce this? The QEMU invocation runes would certainly help too.

It strikes me that in drivers/irqchip/irq-mips-cpu.c,
plat_irq_dispatch() now uses the irqdomain resolution before
irq_enter() took place. That's certainly a latent bug. I'll fix that
regardless, but I'd like to make sure this is what you are seeing too.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
  2021-07-05 18:01   ` Marc Zyngier
@ 2021-07-05 18:23     ` Guenter Roeck
  2021-07-05 18:43       ` Marc Zyngier
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2021-07-05 18:23 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-kernel, tglx

Hi Marc,

On 7/5/21 11:01 AM, Marc Zyngier wrote:
> Hi Guenter,
> 
> On Mon, 05 Jul 2021 18:23:52 +0100,
> Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Hi,
>>
>> On Fri, Jun 11, 2021 at 01:54:36PM -0000, irqchip-bot for Marc Zyngier wrote:
>>> The following commit has been merged into the irq/irqchip-next branch of irqchip:
>>>
>>> Commit-ID:     d4a45c68dc81f9117ceaff9f058d5fae674181b9
>>> Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9
>>> Author:        Marc Zyngier <maz@kernel.org>
>>> AuthorDate:    Mon, 05 Apr 2021 12:57:27 +01:00
>>> Committer:     Marc Zyngier <maz@kernel.org>
>>> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00
>>>
>>> irqdomain: Protect the linear revmap with RCU
>>>
>>> It is pretty odd that the radix tree uses RCU while the linear
>>> portion doesn't, leading to potential surprises for the users,
>>> depending on how the irqdomain has been created.
>>>
>>> Fix this by moving the update of the linear revmap under
>>> the mutex, and the lookup under the RCU read-side lock.
>>>
>>> The mutex name is updated to reflect that it doesn't only
>>> cover the radix-tree anymore.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>
>> This patch results in various RCU warnings when booting mipsel images
>> in qemu. I can not revert the patch due to subsequent changes, so I
>> don't know if a simple revert fixes the problem. Log messages and
>> bisect log see below.
> 
> Thanks for the heads up. Do you have a config file I can use to
> reproduce this? The QEMU invocation runes would certainly help too.
> 
> It strikes me that in drivers/irqchip/irq-mips-cpu.c,
> plat_irq_dispatch() now uses the irqdomain resolution before
> irq_enter() took place. That's certainly a latent bug. I'll fix that
> regardless, but I'd like to make sure this is what you are seeing too.
> 

See http://server.roeck-us.net/qemu/mipsel/

config		Complete configuration file
defconfig	Shortened configuration file
rootfs.cpio	root file system (initrd)
run.sh		qemu run script (tested with qemu 4.2.1 and 6.0.0)
vmlinux		Kernel image experiencing the problem (v5.13-9883-gaf9efb8b661)

Hope this helps,
Guenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
  2021-07-05 18:23     ` Guenter Roeck
@ 2021-07-05 18:43       ` Marc Zyngier
  2021-07-05 20:36         ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2021-07-05 18:43 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-kernel, tglx

On Mon, 05 Jul 2021 19:23:27 +0100,
Guenter Roeck <linux@roeck-us.net> wrote:
> 
> Hi Marc,
> 
> On 7/5/21 11:01 AM, Marc Zyngier wrote:
> > Hi Guenter,
> > 
> > On Mon, 05 Jul 2021 18:23:52 +0100,
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >> 
> >> Hi,
> >> 
> >> On Fri, Jun 11, 2021 at 01:54:36PM -0000, irqchip-bot for Marc Zyngier wrote:
> >>> The following commit has been merged into the irq/irqchip-next branch of irqchip:
> >>> 
> >>> Commit-ID:     d4a45c68dc81f9117ceaff9f058d5fae674181b9
> >>> Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9
> >>> Author:        Marc Zyngier <maz@kernel.org>
> >>> AuthorDate:    Mon, 05 Apr 2021 12:57:27 +01:00
> >>> Committer:     Marc Zyngier <maz@kernel.org>
> >>> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00
> >>> 
> >>> irqdomain: Protect the linear revmap with RCU
> >>> 
> >>> It is pretty odd that the radix tree uses RCU while the linear
> >>> portion doesn't, leading to potential surprises for the users,
> >>> depending on how the irqdomain has been created.
> >>> 
> >>> Fix this by moving the update of the linear revmap under
> >>> the mutex, and the lookup under the RCU read-side lock.
> >>> 
> >>> The mutex name is updated to reflect that it doesn't only
> >>> cover the radix-tree anymore.
> >>> 
> >>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >> 
> >> This patch results in various RCU warnings when booting mipsel images
> >> in qemu. I can not revert the patch due to subsequent changes, so I
> >> don't know if a simple revert fixes the problem. Log messages and
> >> bisect log see below.
> > 
> > Thanks for the heads up. Do you have a config file I can use to
> > reproduce this? The QEMU invocation runes would certainly help too.
> > 
> > It strikes me that in drivers/irqchip/irq-mips-cpu.c,
> > plat_irq_dispatch() now uses the irqdomain resolution before
> > irq_enter() took place. That's certainly a latent bug. I'll fix that
> > regardless, but I'd like to make sure this is what you are seeing too.
> > 
> 
> See http://server.roeck-us.net/qemu/mipsel/
> 
> config		Complete configuration file
> defconfig	Shortened configuration file
> rootfs.cpio	root file system (initrd)
> run.sh		qemu run script (tested with qemu 4.2.1 and 6.0.0)
> vmlinux		Kernel image experiencing the problem (v5.13-9883-gaf9efb8b661)
> 
> Hope this helps,

It definitely helps, and confirms my hunch. With the patch below, I'm
not getting the warnings anymore. I'm pretty sure a number of other
MIPS systems suffer from similar issues, which I'll address similarly.

Please let me know if that addresses the issue on your end.

Thanks,

	M.

diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
index d1477ecb1af9..57561e0e6e8d 100644
--- a/arch/mips/include/asm/irq.h
+++ b/arch/mips/include/asm/irq.h
@@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);
 
 extern void do_IRQ(unsigned int irq);
 
+struct irq_domain;
+extern void do_domain_IRQ(struct irq_domain *domain, unsigned int irq);
+
 extern void arch_init_irq(void);
 extern void spurious_interrupt(void);
 
diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
index 85b6c60f285d..c76005cd3b79 100644
--- a/arch/mips/kernel/irq.c
+++ b/arch/mips/kernel/irq.c
@@ -21,6 +21,7 @@
 #include <linux/kallsyms.h>
 #include <linux/kgdb.h>
 #include <linux/ftrace.h>
+#include <linux/irqdomain.h>
 
 #include <linux/atomic.h>
 #include <linux/uaccess.h>
@@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq)
 	irq_exit();
 }
 
+void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq)
+{
+	struct irq_desc *desc;
+
+	irq_enter();
+	check_stack_overflow();
+
+	desc = irq_resolve_mapping(domain, hwirq);
+	if (likely(desc))
+		handle_irq_desc(desc);
+
+	irq_exit();
+}
diff --git a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
index 0bbb0b2d0dd5..0c7ae71a0af0 100644
--- a/drivers/irqchip/irq-mips-cpu.c
+++ b/drivers/irqchip/irq-mips-cpu.c
@@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = {
 asmlinkage void __weak plat_irq_dispatch(void)
 {
 	unsigned long pending = read_c0_cause() & read_c0_status() & ST0_IM;
-	unsigned int virq;
 	int irq;
 
 	if (!pending) {
@@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void)
 
 	pending >>= CAUSEB_IP;
 	while (pending) {
+		struct irq_domain *d;
+
 		irq = fls(pending) - 1;
 		if (IS_ENABLED(CONFIG_GENERIC_IRQ_IPI) && irq < 2)
-			virq = irq_linear_revmap(ipi_domain, irq);
+			d = ipi_domain;
 		else
-			virq = irq_linear_revmap(irq_domain, irq);
-		do_IRQ(virq);
+			d = irq_domain;
+
+		do_domain_IRQ(d, irq);
 		pending &= ~BIT(irq);
 	}
 }

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
  2021-07-05 18:43       ` Marc Zyngier
@ 2021-07-05 20:36         ` Guenter Roeck
  2021-07-06  9:24           ` Marc Zyngier
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2021-07-05 20:36 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-kernel, tglx

On 7/5/21 11:43 AM, Marc Zyngier wrote:
> On Mon, 05 Jul 2021 19:23:27 +0100,
> Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Hi Marc,
>>
>> On 7/5/21 11:01 AM, Marc Zyngier wrote:
>>> Hi Guenter,
>>>
>>> On Mon, 05 Jul 2021 18:23:52 +0100,
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Fri, Jun 11, 2021 at 01:54:36PM -0000, irqchip-bot for Marc Zyngier wrote:
>>>>> The following commit has been merged into the irq/irqchip-next branch of irqchip:
>>>>>
>>>>> Commit-ID:     d4a45c68dc81f9117ceaff9f058d5fae674181b9
>>>>> Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9
>>>>> Author:        Marc Zyngier <maz@kernel.org>
>>>>> AuthorDate:    Mon, 05 Apr 2021 12:57:27 +01:00
>>>>> Committer:     Marc Zyngier <maz@kernel.org>
>>>>> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00
>>>>>
>>>>> irqdomain: Protect the linear revmap with RCU
>>>>>
>>>>> It is pretty odd that the radix tree uses RCU while the linear
>>>>> portion doesn't, leading to potential surprises for the users,
>>>>> depending on how the irqdomain has been created.
>>>>>
>>>>> Fix this by moving the update of the linear revmap under
>>>>> the mutex, and the lookup under the RCU read-side lock.
>>>>>
>>>>> The mutex name is updated to reflect that it doesn't only
>>>>> cover the radix-tree anymore.
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>
>>>> This patch results in various RCU warnings when booting mipsel images
>>>> in qemu. I can not revert the patch due to subsequent changes, so I
>>>> don't know if a simple revert fixes the problem. Log messages and
>>>> bisect log see below.
>>>
>>> Thanks for the heads up. Do you have a config file I can use to
>>> reproduce this? The QEMU invocation runes would certainly help too.
>>>
>>> It strikes me that in drivers/irqchip/irq-mips-cpu.c,
>>> plat_irq_dispatch() now uses the irqdomain resolution before
>>> irq_enter() took place. That's certainly a latent bug. I'll fix that
>>> regardless, but I'd like to make sure this is what you are seeing too.
>>>
>>
>> See http://server.roeck-us.net/qemu/mipsel/
>>
>> config		Complete configuration file
>> defconfig	Shortened configuration file
>> rootfs.cpio	root file system (initrd)
>> run.sh		qemu run script (tested with qemu 4.2.1 and 6.0.0)
>> vmlinux		Kernel image experiencing the problem (v5.13-9883-gaf9efb8b661)
>>
>> Hope this helps,
> 
> It definitely helps, and confirms my hunch. With the patch below, I'm
> not getting the warnings anymore. I'm pretty sure a number of other
> MIPS systems suffer from similar issues, which I'll address similarly.
> 
> Please let me know if that addresses the issue on your end.
> 

Yes, it does. Feel free to add

Tested-by: Guenter Roeck <linux@roeck-us.net>

to the real patch.

Now the big question: Why does this only affect 32-bit little endian
mips images, but not the matching big endian images, nor 64-bit images ?

Thanks,
Guenter

> Thanks,
> 
> 	M.
> 
> diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
> index d1477ecb1af9..57561e0e6e8d 100644
> --- a/arch/mips/include/asm/irq.h
> +++ b/arch/mips/include/asm/irq.h
> @@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);
>   
>   extern void do_IRQ(unsigned int irq);
>   
> +struct irq_domain;
> +extern void do_domain_IRQ(struct irq_domain *domain, unsigned int irq);
> +
>   extern void arch_init_irq(void);
>   extern void spurious_interrupt(void);
>   
> diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
> index 85b6c60f285d..c76005cd3b79 100644
> --- a/arch/mips/kernel/irq.c
> +++ b/arch/mips/kernel/irq.c
> @@ -21,6 +21,7 @@
>   #include <linux/kallsyms.h>
>   #include <linux/kgdb.h>
>   #include <linux/ftrace.h>
> +#include <linux/irqdomain.h>
>   
>   #include <linux/atomic.h>
>   #include <linux/uaccess.h>
> @@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq)
>   	irq_exit();
>   }
>   
> +void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq)
> +{
> +	struct irq_desc *desc;
> +
> +	irq_enter();
> +	check_stack_overflow();
> +
> +	desc = irq_resolve_mapping(domain, hwirq);
> +	if (likely(desc))
> +		handle_irq_desc(desc);
> +
> +	irq_exit();
> +}
> diff --git a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
> index 0bbb0b2d0dd5..0c7ae71a0af0 100644
> --- a/drivers/irqchip/irq-mips-cpu.c
> +++ b/drivers/irqchip/irq-mips-cpu.c
> @@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = {
>   asmlinkage void __weak plat_irq_dispatch(void)
>   {
>   	unsigned long pending = read_c0_cause() & read_c0_status() & ST0_IM;
> -	unsigned int virq;
>   	int irq;
>   
>   	if (!pending) {
> @@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void)
>   
>   	pending >>= CAUSEB_IP;
>   	while (pending) {
> +		struct irq_domain *d;
> +
>   		irq = fls(pending) - 1;
>   		if (IS_ENABLED(CONFIG_GENERIC_IRQ_IPI) && irq < 2)
> -			virq = irq_linear_revmap(ipi_domain, irq);
> +			d = ipi_domain;
>   		else
> -			virq = irq_linear_revmap(irq_domain, irq);
> -		do_IRQ(virq);
> +			d = irq_domain;
> +
> +		do_domain_IRQ(d, irq);
>   		pending &= ~BIT(irq);
>   	}
>   }
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
  2021-07-05 20:36         ` Guenter Roeck
@ 2021-07-06  9:24           ` Marc Zyngier
  2021-07-06 14:23             ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2021-07-06  9:24 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-kernel, tglx

On Mon, 05 Jul 2021 21:36:36 +0100,
Guenter Roeck <linux@roeck-us.net> wrote:
> 
> On 7/5/21 11:43 AM, Marc Zyngier wrote:

> > It definitely helps, and confirms my hunch. With the patch below, I'm
> > not getting the warnings anymore. I'm pretty sure a number of other
> > MIPS systems suffer from similar issues, which I'll address similarly.
> > 
> > Please let me know if that addresses the issue on your end.
> > 
> 
> Yes, it does. Feel free to add
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> to the real patch.

Thanks.

> Now the big question: Why does this only affect 32-bit little endian
> mips images, but not the matching big endian images, nor 64-bit images ?

Are you sure these images are using the exact same HW? A bunch (most?)
of MIPS systems do not use irqdomains in their root interrupt
handling, so this issue wouldn't be visible (irq_enter() will already
have been done for the chained interrupt handling).

FWIW, I can reproduce the problem by switching your mipsel config to
BE, and adding this patch fixes it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
  2021-07-06  9:24           ` Marc Zyngier
@ 2021-07-06 14:23             ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2021-07-06 14:23 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-kernel, tglx

On 7/6/21 2:24 AM, Marc Zyngier wrote:
> On Mon, 05 Jul 2021 21:36:36 +0100,
> Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 7/5/21 11:43 AM, Marc Zyngier wrote:
> 
>>> It definitely helps, and confirms my hunch. With the patch below, I'm
>>> not getting the warnings anymore. I'm pretty sure a number of other
>>> MIPS systems suffer from similar issues, which I'll address similarly.
>>>
>>> Please let me know if that addresses the issue on your end.
>>>
>>
>> Yes, it does. Feel free to add
>>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>
>> to the real patch.
> 
> Thanks.
> 
>> Now the big question: Why does this only affect 32-bit little endian
>> mips images, but not the matching big endian images, nor 64-bit images ?
> 

Actually they do.

> Are you sure these images are using the exact same HW? A bunch (most?)
> of MIPS systems do not use irqdomains in their root interrupt
> handling, so this issue wouldn't be visible (irq_enter() will already
> have been done for the chained interrupt handling).
> 
> FWIW, I can reproduce the problem by switching your mipsel config to
> BE, and adding this patch fixes it.
> 

Yes, turns out I did not have the necessary debugging options enabled
in my other mips tests.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [irqchip: irq/irqchip-fixes] irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry
  2021-07-05 17:23 ` Guenter Roeck
  2021-07-05 18:01   ` Marc Zyngier
@ 2021-07-08  8:00   ` irqchip-bot for Marc Zyngier
  2021-07-09  9:23   ` irqchip-bot for Marc Zyngier
  2 siblings, 0 replies; 11+ messages in thread
From: irqchip-bot for Marc Zyngier @ 2021-07-08  8:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: Guenter Roeck, Marc Zyngier, Thomas Bogendoerfer, Serge Semin, tglx

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID:     f333d6bc4a8b2ef6c999e46c508d50277baddcd6
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/f333d6bc4a8b2ef6c999e46c508d50277baddcd6
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Tue, 06 Jul 2021 11:38:59 +01:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Tue, 06 Jul 2021 13:55:57 +01:00

irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry

Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"),
any irqdomain lookup requires the RCU read lock to be held.

This assumes that the architecture code will be structured such as
irq_enter() will be called *before* the interrupt is looked up
in the irq domain. However, this isn't the case for MIPS, and a number
of drivers are structured to do it the other way around when handling
an interrupt in their root irqchip (secondary irqchips are OK by
construction).

This results in a RCU splat on a lockdep-enabled kernel when the kernel
takes an interrupt from idle, as reported by Guenter Roeck.

Note that this could have fired previously if any driver had used
tree-based irqdomain, which always had the RCU requirement.

To solve this, provide a MIPS-specific helper (do_domain_IRQ())
as the pendent of do_IRQ() that will do thing in the right order
(and maybe save some cycles in the process).

Ideally, MIPS would be moved over to using handle_domain_irq(),
but that's much more ambitious.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Serge Semin <fancer.lancer@gmail.com>
Link: https://lore.kernel.org/r/20210705172352.GA56304@roeck-us.net
Link: https://lore.kernel.org/r/20210706110647.3979002-1-maz@kernel.org
---
 arch/mips/include/asm/irq.h      |  3 +++
 arch/mips/kernel/irq.c           | 14 ++++++++++++++
 drivers/irqchip/irq-mips-cpu.c   | 10 ++++++----
 drivers/irqchip/irq-mips-gic.c   |  8 ++++----
 drivers/irqchip/irq-pic32-evic.c |  5 ++---
 5 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
index d1477ec..57561e0 100644
--- a/arch/mips/include/asm/irq.h
+++ b/arch/mips/include/asm/irq.h
@@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);
 
 extern void do_IRQ(unsigned int irq);
 
+struct irq_domain;
+extern void do_domain_IRQ(struct irq_domain *domain, unsigned int irq);
+
 extern void arch_init_irq(void);
 extern void spurious_interrupt(void);
 
diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
index 85b6c60..c76005c 100644
--- a/arch/mips/kernel/irq.c
+++ b/arch/mips/kernel/irq.c
@@ -21,6 +21,7 @@
 #include <linux/kallsyms.h>
 #include <linux/kgdb.h>
 #include <linux/ftrace.h>
+#include <linux/irqdomain.h>
 
 #include <linux/atomic.h>
 #include <linux/uaccess.h>
@@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq)
 	irq_exit();
 }
 
+void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq)
+{
+	struct irq_desc *desc;
+
+	irq_enter();
+	check_stack_overflow();
+
+	desc = irq_resolve_mapping(domain, hwirq);
+	if (likely(desc))
+		handle_irq_desc(desc);
+
+	irq_exit();
+}
diff --git a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
index 0bbb0b2..0c7ae71 100644
--- a/drivers/irqchip/irq-mips-cpu.c
+++ b/drivers/irqchip/irq-mips-cpu.c
@@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = {
 asmlinkage void __weak plat_irq_dispatch(void)
 {
 	unsigned long pending = read_c0_cause() & read_c0_status() & ST0_IM;
-	unsigned int virq;
 	int irq;
 
 	if (!pending) {
@@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void)
 
 	pending >>= CAUSEB_IP;
 	while (pending) {
+		struct irq_domain *d;
+
 		irq = fls(pending) - 1;
 		if (IS_ENABLED(CONFIG_GENERIC_IRQ_IPI) && irq < 2)
-			virq = irq_linear_revmap(ipi_domain, irq);
+			d = ipi_domain;
 		else
-			virq = irq_linear_revmap(irq_domain, irq);
-		do_IRQ(virq);
+			d = irq_domain;
+
+		do_domain_IRQ(d, irq);
 		pending &= ~BIT(irq);
 	}
 }
diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index b146e06..54c7092 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -169,8 +169,8 @@ static void gic_handle_shared_int(bool chained)
 			generic_handle_domain_irq(gic_irq_domain,
 						  GIC_SHARED_TO_HWIRQ(intr));
 		else
-			do_IRQ(irq_find_mapping(gic_irq_domain,
-						GIC_SHARED_TO_HWIRQ(intr)));
+			do_domain_IRQ(gic_irq_domain,
+				      GIC_SHARED_TO_HWIRQ(intr));
 	}
 }
 
@@ -320,8 +320,8 @@ static void gic_handle_local_int(bool chained)
 			generic_handle_domain_irq(gic_irq_domain,
 						  GIC_LOCAL_TO_HWIRQ(intr));
 		else
-			do_IRQ(irq_find_mapping(gic_irq_domain,
-						GIC_LOCAL_TO_HWIRQ(intr)));
+			do_domain_IRQ(gic_irq_domain,
+				      GIC_LOCAL_TO_HWIRQ(intr));
 	}
 }
 
diff --git a/drivers/irqchip/irq-pic32-evic.c b/drivers/irqchip/irq-pic32-evic.c
index 34c4b4f..1d9bb28 100644
--- a/drivers/irqchip/irq-pic32-evic.c
+++ b/drivers/irqchip/irq-pic32-evic.c
@@ -42,11 +42,10 @@ static void __iomem *evic_base;
 
 asmlinkage void __weak plat_irq_dispatch(void)
 {
-	unsigned int irq, hwirq;
+	unsigned int hwirq;
 
 	hwirq = readl(evic_base + REG_INTSTAT) & 0xFF;
-	irq = irq_linear_revmap(evic_irq_domain, hwirq);
-	do_IRQ(irq);
+	do_domain_IRQ(evic_irq_domain, hwirq);
 }
 
 static struct evic_chip_data *irqd_to_priv(struct irq_data *data)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [irqchip: irq/irqchip-fixes] irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry
  2021-07-05 17:23 ` Guenter Roeck
  2021-07-05 18:01   ` Marc Zyngier
  2021-07-08  8:00   ` [irqchip: irq/irqchip-fixes] irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry irqchip-bot for Marc Zyngier
@ 2021-07-09  9:23   ` irqchip-bot for Marc Zyngier
  2 siblings, 0 replies; 11+ messages in thread
From: irqchip-bot for Marc Zyngier @ 2021-07-09  9:23 UTC (permalink / raw)
  To: linux-kernel
  Cc: Guenter Roeck, Marc Zyngier, Thomas Bogendoerfer, Serge Semin, tglx

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID:     1fee9db9b42d821e8007289d4eea74bdf85b1543
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/1fee9db9b42d821e8007289d4eea74bdf85b1543
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Tue, 06 Jul 2021 11:38:59 +01:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Fri, 09 Jul 2021 10:18:58 +01:00

irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry

Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"),
any irqdomain lookup requires the RCU read lock to be held.

This assumes that the architecture code will be structured such as
irq_enter() will be called *before* the interrupt is looked up
in the irq domain. However, this isn't the case for MIPS, and a number
of drivers are structured to do it the other way around when handling
an interrupt in their root irqchip (secondary irqchips are OK by
construction).

This results in a RCU splat on a lockdep-enabled kernel when the kernel
takes an interrupt from idle, as reported by Guenter Roeck.

Note that this could have fired previously if any driver had used
tree-based irqdomain, which always had the RCU requirement.

To solve this, provide a MIPS-specific helper (do_domain_IRQ())
as the pendent of do_IRQ() that will do thing in the right order
(and maybe save some cycles in the process).

Ideally, MIPS would be moved over to using handle_domain_irq(),
but that's much more ambitious.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
[maz: add dependency on CONFIG_IRQ_DOMAIN after report from the kernelci bot]
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Serge Semin <fancer.lancer@gmail.com>
Link: https://lore.kernel.org/r/20210705172352.GA56304@roeck-us.net
Link: https://lore.kernel.org/r/20210706110647.3979002-1-maz@kernel.org
---
 arch/mips/include/asm/irq.h      |  3 +++
 arch/mips/kernel/irq.c           | 16 ++++++++++++++++
 drivers/irqchip/irq-mips-cpu.c   | 10 ++++++----
 drivers/irqchip/irq-mips-gic.c   |  8 ++++----
 drivers/irqchip/irq-pic32-evic.c |  5 ++---
 5 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
index d1477ec..57561e0 100644
--- a/arch/mips/include/asm/irq.h
+++ b/arch/mips/include/asm/irq.h
@@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);
 
 extern void do_IRQ(unsigned int irq);
 
+struct irq_domain;
+extern void do_domain_IRQ(struct irq_domain *domain, unsigned int irq);
+
 extern void arch_init_irq(void);
 extern void spurious_interrupt(void);
 
diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
index 85b6c60..d20e002 100644
--- a/arch/mips/kernel/irq.c
+++ b/arch/mips/kernel/irq.c
@@ -21,6 +21,7 @@
 #include <linux/kallsyms.h>
 #include <linux/kgdb.h>
 #include <linux/ftrace.h>
+#include <linux/irqdomain.h>
 
 #include <linux/atomic.h>
 #include <linux/uaccess.h>
@@ -107,3 +108,18 @@ void __irq_entry do_IRQ(unsigned int irq)
 	irq_exit();
 }
 
+#ifdef CONFIG_IRQ_DOMAIN
+void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq)
+{
+	struct irq_desc *desc;
+
+	irq_enter();
+	check_stack_overflow();
+
+	desc = irq_resolve_mapping(domain, hwirq);
+	if (likely(desc))
+		handle_irq_desc(desc);
+
+	irq_exit();
+}
+#endif
diff --git a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c
index 0bbb0b2..0c7ae71 100644
--- a/drivers/irqchip/irq-mips-cpu.c
+++ b/drivers/irqchip/irq-mips-cpu.c
@@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = {
 asmlinkage void __weak plat_irq_dispatch(void)
 {
 	unsigned long pending = read_c0_cause() & read_c0_status() & ST0_IM;
-	unsigned int virq;
 	int irq;
 
 	if (!pending) {
@@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void)
 
 	pending >>= CAUSEB_IP;
 	while (pending) {
+		struct irq_domain *d;
+
 		irq = fls(pending) - 1;
 		if (IS_ENABLED(CONFIG_GENERIC_IRQ_IPI) && irq < 2)
-			virq = irq_linear_revmap(ipi_domain, irq);
+			d = ipi_domain;
 		else
-			virq = irq_linear_revmap(irq_domain, irq);
-		do_IRQ(virq);
+			d = irq_domain;
+
+		do_domain_IRQ(d, irq);
 		pending &= ~BIT(irq);
 	}
 }
diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index b146e06..54c7092 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -169,8 +169,8 @@ static void gic_handle_shared_int(bool chained)
 			generic_handle_domain_irq(gic_irq_domain,
 						  GIC_SHARED_TO_HWIRQ(intr));
 		else
-			do_IRQ(irq_find_mapping(gic_irq_domain,
-						GIC_SHARED_TO_HWIRQ(intr)));
+			do_domain_IRQ(gic_irq_domain,
+				      GIC_SHARED_TO_HWIRQ(intr));
 	}
 }
 
@@ -320,8 +320,8 @@ static void gic_handle_local_int(bool chained)
 			generic_handle_domain_irq(gic_irq_domain,
 						  GIC_LOCAL_TO_HWIRQ(intr));
 		else
-			do_IRQ(irq_find_mapping(gic_irq_domain,
-						GIC_LOCAL_TO_HWIRQ(intr)));
+			do_domain_IRQ(gic_irq_domain,
+				      GIC_LOCAL_TO_HWIRQ(intr));
 	}
 }
 
diff --git a/drivers/irqchip/irq-pic32-evic.c b/drivers/irqchip/irq-pic32-evic.c
index 34c4b4f..1d9bb28 100644
--- a/drivers/irqchip/irq-pic32-evic.c
+++ b/drivers/irqchip/irq-pic32-evic.c
@@ -42,11 +42,10 @@ static void __iomem *evic_base;
 
 asmlinkage void __weak plat_irq_dispatch(void)
 {
-	unsigned int irq, hwirq;
+	unsigned int hwirq;
 
 	hwirq = readl(evic_base + REG_INTSTAT) & 0xFF;
-	irq = irq_linear_revmap(evic_irq_domain, hwirq);
-	do_IRQ(irq);
+	do_domain_IRQ(evic_irq_domain, hwirq);
 }
 
 static struct evic_chip_data *irqd_to_priv(struct irq_data *data)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU
@ 2021-06-06 12:43 irqchip-bot for Marc Zyngier
  0 siblings, 0 replies; 11+ messages in thread
From: irqchip-bot for Marc Zyngier @ 2021-06-06 12:43 UTC (permalink / raw)
  To: linux-kernel; +Cc: Marc Zyngier, tglx

The following commit has been merged into the irq/irqchip-next branch of irqchip:

Commit-ID:     e0f5b5fa10f5bf9cfa547dce21c92dd323daf584
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/e0f5b5fa10f5bf9cfa547dce21c92dd323daf584
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Mon, 05 Apr 2021 12:57:27 +01:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Wed, 02 Jun 2021 14:34:49 +01:00

irqdomain: Protect the linear revmap with RCU

It is pretty odd that the radix tree uses RCU while the linear
portion doesn't, leading to potential surprises for the users,
depending on how the irqdomain has been created.

Fix this by moving the update of the linear revmap under
the mutex, and the lookup under the RCU read-side lock.

The mutex name is updated to reflect that it doesn't only
cover the radix-tree anymore.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/irqdomain.h |  5 +++--
 kernel/irq/irqdomain.c    | 38 ++++++++++++++++++--------------------
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 340cc04..2b696c9 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -151,6 +151,7 @@ struct irq_domain_chip_generic;
  * Revmap data, used internally by irq_domain
  * @revmap_size: Size of the linear map table @revmap[]
  * @revmap_tree: Radix map tree for hwirqs that don't fit in the linear map
+ * @revmap_mutex: Lock for the revmap
  * @revmap: Linear table of irq_data pointers
  */
 struct irq_domain {
@@ -173,8 +174,8 @@ struct irq_domain {
 	irq_hw_number_t hwirq_max;
 	unsigned int revmap_size;
 	struct radix_tree_root revmap_tree;
-	struct mutex revmap_tree_mutex;
-	struct irq_data *revmap[];
+	struct mutex revmap_mutex;
+	struct irq_data __rcu *revmap[];
 };
 
 /* Irq domain flags */
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index ed2ffff..8e55bb8 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -213,7 +213,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size,
 
 	/* Fill structure */
 	INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL);
-	mutex_init(&domain->revmap_tree_mutex);
+	mutex_init(&domain->revmap_mutex);
 	domain->ops = ops;
 	domain->host_data = host_data;
 	domain->hwirq_max = hwirq_max;
@@ -505,13 +505,12 @@ static void irq_domain_set_mapping(struct irq_domain *domain,
 	if (irq_domain_is_nomap(domain))
 		return;
 
-	if (hwirq < domain->revmap_size) {
-		domain->revmap[hwirq] = irq_data;
-	} else {
-		mutex_lock(&domain->revmap_tree_mutex);
+	mutex_lock(&domain->revmap_mutex);
+	if (hwirq < domain->revmap_size)
+		rcu_assign_pointer(domain->revmap[hwirq], irq_data);
+	else
 		radix_tree_insert(&domain->revmap_tree, hwirq, irq_data);
-		mutex_unlock(&domain->revmap_tree_mutex);
-	}
+	mutex_unlock(&domain->revmap_mutex);
 }
 
 static void irq_domain_clear_mapping(struct irq_domain *domain,
@@ -902,12 +901,12 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
 		return 0;
 	}
 
+	rcu_read_lock();
 	/* Check if the hwirq is in the linear revmap. */
 	if (hwirq < domain->revmap_size)
-		return domain->revmap[hwirq]->irq;
-
-	rcu_read_lock();
-	data = radix_tree_lookup(&domain->revmap_tree, hwirq);
+		data = rcu_dereference(domain->revmap[hwirq]);
+	else
+		data = radix_tree_lookup(&domain->revmap_tree, hwirq);
 	rcu_read_unlock();
 	return data ? data->irq : 0;
 }
@@ -1490,18 +1489,17 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 	if (irq_domain_is_nomap(d->domain))
 		return;
 
+	/* Fix up the revmap. */
+	mutex_lock(&d->domain->revmap_mutex);
 	if (d->hwirq < d->domain->revmap_size) {
 		/* Not using radix tree */
-		d->domain->revmap[d->hwirq] = d;
-		return;
+		rcu_assign_pointer(d->domain->revmap[d->hwirq], d);
+	} else {
+		slot = radix_tree_lookup_slot(&d->domain->revmap_tree, d->hwirq);
+		if (slot)
+			radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
 	}
-
-	/* Fix up the revmap. */
-	mutex_lock(&d->domain->revmap_tree_mutex);
-	slot = radix_tree_lookup_slot(&d->domain->revmap_tree, d->hwirq);
-	if (slot)
-		radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
-	mutex_unlock(&d->domain->revmap_tree_mutex);
+	mutex_unlock(&d->domain->revmap_mutex);
 }
 
 /**

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-07-09  9:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-11 13:54 [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU irqchip-bot for Marc Zyngier
2021-07-05 17:23 ` Guenter Roeck
2021-07-05 18:01   ` Marc Zyngier
2021-07-05 18:23     ` Guenter Roeck
2021-07-05 18:43       ` Marc Zyngier
2021-07-05 20:36         ` Guenter Roeck
2021-07-06  9:24           ` Marc Zyngier
2021-07-06 14:23             ` Guenter Roeck
2021-07-08  8:00   ` [irqchip: irq/irqchip-fixes] irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry irqchip-bot for Marc Zyngier
2021-07-09  9:23   ` irqchip-bot for Marc Zyngier
  -- strict thread matches above, loose matches on Subject: below --
2021-06-06 12:43 [irqchip: irq/irqchip-next] irqdomain: Protect the linear revmap with RCU irqchip-bot for Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).