[PATCH] genirq: check irq_ack callback in handle_edge_irq() before calling

[PATCH] genirq: check irq_ack callback in handle_edge_irq() before calling

[Semen Protsenko]

Direct calling of .irq_ack callback (from struct irq_chip) leads to NULL
pointer dereference in some cases. E.g. this was observed for MAX732X
driver on hibernation:

    Unable to handle kernel NULL pointer dereference at virtual address 0
    Backtrace:
    (handle_edge_irq) from (resend_irqs)
    (resend_irqs) from (tasklet_action)
    (tasklet_action) from (__do_softirq)
    (__do_softirq) from (run_ksoftirqd)
    (run_ksoftirqd) from (smpboot_thread_fn)
    (smpboot_thread_fn) from (kthread)
    (kthread) from (ret_from_fork)

This patch checks if .irq_ack was set, so in case it wasn't (i.e. it's
NULL) we wouldn't have NULL pointer dereference. This check seems to
be pretty common in kernel/irq/chip.c, but it was missed for
handle_edge_irq() function.

Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com>
---
 kernel/irq/chip.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index eb9a4ea..3889b02 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -586,7 +586,8 @@ handle_edge_irq(unsigned int irq, struct irq_desc *desc)
 	kstat_incr_irqs_this_cpu(irq, desc);
 
 	/* Start handling the irq */
-	desc->irq_data.chip->irq_ack(&desc->irq_data);
+	if (desc->irq_data.chip->irq_ack)
+		desc->irq_data.chip->irq_ack(&desc->irq_data);
 
 	do {
 		if (unlikely(!desc->action)) {
-- 
1.7.9.5

Re: [PATCH] genirq: check irq_ack callback in handle_edge_irq() before calling

[Thomas Gleixner]

On Tue, 21 Apr 2015, Semen Protsenko wrote:
> Direct calling of .irq_ack callback (from struct irq_chip) leads to NULL
> pointer dereference in some cases. E.g. this was observed for MAX732X
> driver on hibernation:
> 
>     Unable to handle kernel NULL pointer dereference at virtual address 0
>     Backtrace:
>     (handle_edge_irq) from (resend_irqs)
>     (resend_irqs) from (tasklet_action)
>     (tasklet_action) from (__do_softirq)
>     (__do_softirq) from (run_ksoftirqd)
>     (run_ksoftirqd) from (smpboot_thread_fn)
>     (smpboot_thread_fn) from (kthread)
>     (kthread) from (ret_from_fork)
> 
> This patch checks if .irq_ack was set, so in case it wasn't (i.e. it's
> NULL) we wouldn't have NULL pointer dereference. This check seems to
> be pretty common in kernel/irq/chip.c, but it was missed for
> handle_edge_irq() function.

No, it's not missing by chance. It's missing on purpose. The edge
handler is designed to deal with edge type interrupt chips and those
have an ACK by definition.

You are fixing the wrong place. That GPIO expander is using the wrong
flow handler. It should not use the edge handler at all.

Thanks,

	tglx

Re: [PATCH] genirq: check irq_ack callback in handle_edge_irq() before calling

[Sam Protsenko]

On Tue, Apr 21, 2015 at 5:40 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> No, it's not missing by chance. It's missing on purpose. The edge
> handler is designed to deal with edge type interrupt chips and those
> have an ACK by definition.
>
> You are fixing the wrong place. That GPIO expander is using the wrong
> flow handler. It should not use the edge handler at all.

Just missed comments for handle_edge_irq() function, explaining why
irq_ack is mandatory. So I just need to use handle_level_irq() in expander
driver (instead of handle_edge_irq()).

Thanks!