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:   Wed, 5 Aug 2020 08:17:53 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Ahmad Fatoum <a.fatoum@...gutronix.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     Thorsten Scherer <t.scherer@...elmann.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] gpio: siox: indicate exclusive support of threaded IRQs

Hello,

[adding tglx for irq expertise to Cc]

On Tue, Aug 04, 2020 at 11:16:03AM +0200, Ahmad Fatoum wrote:
> Generic GPIO consumers like gpio-keys use request_any_context_irq()
> to request a threaded handler if irq_settings_is_nested_thread() ==
> true or a hardirq handler otherwise.
> 
> Drivers using handle_nested_irq() must be sure that the nested
> IRQs were requested with threaded handlers, because the IRQ
> is handled by calling action->thread_fn().
> 
> The gpio-siox driver dispatches IRQs via handle_nested_irq,
> but has irq_settings_is_nested_thread() == false.
> 
> Set gpio_irq_chip::threaded to remedy this.

Sounds reasonable, but I have to keep this for others to judge if this
is indeed how the irq stuff works.

> ---
> I am writing a driver similar to gpio-siox and I ran into a null pointer
> dereference, because ->threaded wasn't set. I didn't test this on actual
> SIOX hardware.
> 
> This patch doesn't fix the case were are driver explicitly calls
> request_irq and is combined with a driver that does handle_nested_irq.
> 
> Is there a flag, such drivers should additionally set or should we
> check action->thread_fn before calling it inside handle_nested_irq?

If gpio_irq_chip::threaded being set means that this problem happens
IMHO the driver is fine and something in the generic gpio code should do
the right thing here.

Best regards and thanks for caring,
Uwe

> ---
>  drivers/gpio/gpio-siox.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
> index 26e1fe092304..f8c5e9fc4bac 100644
> --- a/drivers/gpio/gpio-siox.c
> +++ b/drivers/gpio/gpio-siox.c
> @@ -245,6 +245,7 @@ static int gpio_siox_probe(struct siox_device *sdevice)
>  	girq->chip = &ddata->ichip;
>  	girq->default_type = IRQ_TYPE_NONE;
>  	girq->handler = handle_level_irq;
> +	girq->threaded = true;
>  
>  	ret = devm_gpiochip_add_data(dev, &ddata->gchip, NULL);
>  	if (ret)
> -- 
> 2.28.0
> 
> 
> 

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