linux-kernel - Re: [PATCH] gpio: siox: indicate exclusive support of threaded IRQs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Aug 2020 23:07:09 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Ahmad Fatoum <a.fatoum@...gutronix.de>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Thorsten Scherer <t.scherer@...elmann.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>
Subject: Re: [PATCH] gpio: siox: indicate exclusive support of threaded IRQs

Hello Thomas,

On Thu, Aug 06, 2020 at 10:33:06PM +0200, Thomas Gleixner wrote:
> Uwe Kleine-König <u.kleine-koenig@...gutronix.de> writes:
> > On Thu, Aug 06, 2020 at 08:50:45PM +0200, Thomas Gleixner wrote:
> >> handle_nested_irq() does not care. It cares about thread context,
> >> external reentrancy protection for the same nested interrupt and that
> >> the nested interrupt has a thread handler.
> >> 
> >> The latter is what goes belly up because w/o that threaded bit set the
> >> GPIO core fails to set nested thread. So if a consumer requests an
> >> interrupt with request_any_context_irq() then that fails to select
> >> thread mode which means the threaded handler is not set causing
> >> handle_nested_irq() to fail.
> >
> > For a caller of request_threaded_irq() that passes a relevant hardirq
> > handler the hardirq handler is never called but request_threaded_irq()
> > doesn't fail. The handler is just replaced by irq_nested_primary_handler
> > in __setup_irq(). Is that a bug? (I didn't test, just read the code, so I
> > might have missed something.)
> 
> Depends on what the threaded handler expects what the primary handler
> has done. It might just work or not :)

So we need something like:

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 48c38e09c673..31777a0b79df 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1393,12 +1393,18 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			ret = -EINVAL;
 			goto out_mput;
 		}
-		/*
-		 * Replace the primary handler which was provided from
-		 * the driver for non nested interrupt handling by the
-		 * dummy function which warns when called.
-		 */
-		new->handler = irq_nested_primary_handler;
+
+		if (new->handler == NULL) {
+			/* Scream loud if the primary handler gets called */
+			new->handler = irq_nested_primary_handler;
+		} else {
+			/*
+			 * The handler won't be called as the requestor expects,
+			 * so refuse to install the handler
+			 */
+			ret = -EINVAL;
+			goto out_mput;
+		}
 	} else {
 		if (irq_settings_can_thread(desc)) {
 			ret = irq_setup_forced_threading(new);

? Do we need to care for other allowed values of new->handler? Maybe
irq_default_primary_handler?

> > Is this bad enough to justify sending this patch to stable?
> 
> Yes, a Cc: stable and a Fixes: tag is justified.

That would be

Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox")

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)