* [PATCH 2/2] gpio: gpio-grgpio: fix possible sleep-in-atomic-context bugs in grgpio_irq_map/unmap()
@ 2019-12-18 13:26 Jia-Ju Bai
2020-01-07 9:37 ` Linus Walleij
0 siblings, 1 reply; 3+ messages in thread
From: Jia-Ju Bai @ 2019-12-18 13:26 UTC (permalink / raw)
To: linus.walleij, bgolaszewski; +Cc: linux-gpio, linux-kernel, Jia-Ju Bai
The driver may sleep while holding a spinlock.
The function call path (from bottom to top) in Linux 4.19 is:
drivers/gpio/gpio-grgpio.c, 261:
request_irq in grgpio_irq_map
drivers/gpio/gpio-grgpio.c, 255:
_raw_spin_lock_irqsave in grgpio_irq_map
drivers/gpio/gpio-grgpio.c, 318:
free_irq in grgpio_irq_unmap
drivers/gpio/gpio-grgpio.c, 299:
_raw_spin_lock_irqsave in grgpio_irq_unmap
request_irq() and free_irq() can sleep at runtime.
To fix these bugs, request_irq() and free_irq() are called without
holding the spinlock.
These bugs are found by a static analysis tool STCheck written by myself.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/gpio/gpio-grgpio.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-grgpio.c b/drivers/gpio/gpio-grgpio.c
index 08234e64993a..3224933f4c8f 100644
--- a/drivers/gpio/gpio-grgpio.c
+++ b/drivers/gpio/gpio-grgpio.c
@@ -253,17 +253,16 @@ static int grgpio_irq_map(struct irq_domain *d, unsigned int irq,
lirq->irq = irq;
uirq = &priv->uirqs[lirq->index];
if (uirq->refcnt == 0) {
+ spin_unlock_irqrestore(&priv->gc.bgpio_lock, flags);
ret = request_irq(uirq->uirq, grgpio_irq_handler, 0,
dev_name(priv->dev), priv);
if (ret) {
dev_err(priv->dev,
"Could not request underlying irq %d\n",
uirq->uirq);
-
- spin_unlock_irqrestore(&priv->gc.bgpio_lock, flags);
-
return ret;
}
+ spin_lock_irqsave(&priv->gc.bgpio_lock, flags);
}
uirq->refcnt++;
@@ -309,8 +308,11 @@ static void grgpio_irq_unmap(struct irq_domain *d, unsigned int irq)
if (index >= 0) {
uirq = &priv->uirqs[lirq->index];
uirq->refcnt--;
- if (uirq->refcnt == 0)
+ if (uirq->refcnt == 0) {
+ spin_unlock_irqrestore(&priv->gc.bgpio_lock, flags);
free_irq(uirq->uirq, priv);
+ return;
+ }
}
spin_unlock_irqrestore(&priv->gc.bgpio_lock, flags);
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] gpio: gpio-grgpio: fix possible sleep-in-atomic-context bugs in grgpio_irq_map/unmap()
2019-12-18 13:26 [PATCH 2/2] gpio: gpio-grgpio: fix possible sleep-in-atomic-context bugs in grgpio_irq_map/unmap() Jia-Ju Bai
@ 2020-01-07 9:37 ` Linus Walleij
2020-01-08 14:41 ` Andreas Larsson
0 siblings, 1 reply; 3+ messages in thread
From: Linus Walleij @ 2020-01-07 9:37 UTC (permalink / raw)
To: Jia-Ju Bai, Andreas Larsson
Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel
On Wed, Dec 18, 2019 at 2:26 PM Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
> The driver may sleep while holding a spinlock.
> The function call path (from bottom to top) in Linux 4.19 is:
>
> drivers/gpio/gpio-grgpio.c, 261:
> request_irq in grgpio_irq_map
> drivers/gpio/gpio-grgpio.c, 255:
> _raw_spin_lock_irqsave in grgpio_irq_map
>
> drivers/gpio/gpio-grgpio.c, 318:
> free_irq in grgpio_irq_unmap
> drivers/gpio/gpio-grgpio.c, 299:
> _raw_spin_lock_irqsave in grgpio_irq_unmap
>
> request_irq() and free_irq() can sleep at runtime.
>
> To fix these bugs, request_irq() and free_irq() are called without
> holding the spinlock.
>
> These bugs are found by a static analysis tool STCheck written by myself.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
I suppose this is correct, so patch applied.
However there is a deeper problem, this code was added by Andreas
Larsson in 2013 and at the time this was a hacky way to deal with
an interrupt that is actually hierarchical.
Since 2013 we have gained:
- Hierarchical interrupt controllers
- Hierarchical interrupt chip helpers in gpiolib
So this code really needs to be modernized using a hierarchical
irqchip.
See for example commit:
aa7d618ac65f ("gpio: ixp4xx: Convert to hierarchical GPIOLIB_IRQCHIP")
for an example.
Who is using grgpio these days and could work on fixing this up?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] gpio: gpio-grgpio: fix possible sleep-in-atomic-context bugs in grgpio_irq_map/unmap()
2020-01-07 9:37 ` Linus Walleij
@ 2020-01-08 14:41 ` Andreas Larsson
0 siblings, 0 replies; 3+ messages in thread
From: Andreas Larsson @ 2020-01-08 14:41 UTC (permalink / raw)
To: Linus Walleij, Jia-Ju Bai
Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, linux-kernel
On 2020-01-07 10:37, Linus Walleij wrote:
> However there is a deeper problem, this code was added by Andreas
> Larsson in 2013 and at the time this was a hacky way to deal with
> an interrupt that is actually hierarchical.
>
> Since 2013 we have gained:
> - Hierarchical interrupt controllers
> - Hierarchical interrupt chip helpers in gpiolib
>
> So this code really needs to be modernized using a hierarchical
> irqchip.
>
> See for example commit:
> aa7d618ac65f ("gpio: ixp4xx: Convert to hierarchical GPIOLIB_IRQCHIP")
> for an example.
>
> Who is using grgpio these days and could work on fixing this up?
I will put on my list to look into this. GRGPIO is used in all our
chips, and in most designs made by our customers.
The main hurdle with the interrupt handling in the current driver was to
both allow several lines to generate the same system interrupt and at
the same time make sure to not register any system interrupts for any
lines until the user actually requests it (as in the general case all
interrupts would be registered leading to clashes with interrupts that
cannot necessarily be shared). Hopefully, the hierarchical interrupt
controller and chip helper functionalities can cater for these requirements.
Best regards,
Andreas Larsson
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-08 14:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 13:26 [PATCH 2/2] gpio: gpio-grgpio: fix possible sleep-in-atomic-context bugs in grgpio_irq_map/unmap() Jia-Ju Bai
2020-01-07 9:37 ` Linus Walleij
2020-01-08 14:41 ` Andreas Larsson
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).