linux-kernel - Re: [PATCH] pinctrl: sunxi: Use unique lockdep classes for 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]

Message-ID: <20220216042637.GB63304@roeck-us.net>
Date:   Tue, 15 Feb 2022 20:26:37 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        linux-sunxi@...ts.linux.dev, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pinctrl: sunxi: Use unique lockdep classes for IRQs

On Tue, Feb 15, 2022 at 10:00:36PM -0600, Samuel Holland wrote:
> This driver, like several others, uses a chained IRQ for each GPIO bank,
> and forwards .irq_set_wake to the GPIO bank's upstream IRQ. As a result,
> a call to irq_set_irq_wake() needs to lock both the upstream and
> downstream irq_desc's. Lockdep considers this to be a possible deadlock
> when the irq_desc's share lockdep classes, which they do by default:
> 
>  ============================================
>  WARNING: possible recursive locking detected
>  5.17.0-rc3-00394-gc849047c2473 #1 Not tainted
>  --------------------------------------------
>  init/307 is trying to acquire lock:
>  c2dfe27c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> 
>  but task is already holding lock:
>  c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> 
>  other info that might help us debug this:
>   Possible unsafe locking scenario:
> 
>         CPU0
>         ----
>    lock(&irq_desc_lock_class);
>    lock(&irq_desc_lock_class);
> 
>   *** DEADLOCK ***
> 
>   May be due to missing lock nesting notation
> 
>  4 locks held by init/307:
>   #0: c1f29f18 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x90/0x23c
>   #1: c20f7760 (&dev->mutex){....}-{3:3}, at: device_shutdown+0xf4/0x224
>   #2: c2e804d8 (&dev->mutex){....}-{3:3}, at: device_shutdown+0x104/0x224
>   #3: c3c0ac7c (&irq_desc_lock_class){-.-.}-{2:2}, at: __irq_get_desc_lock+0x58/0xa0
> 
>  stack backtrace:
>  CPU: 0 PID: 307 Comm: init Not tainted 5.17.0-rc3-00394-gc849047c2473 #1
>  Hardware name: Allwinner sun8i Family
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x68/0x90
>   dump_stack_lvl from __lock_acquire+0x1680/0x31a0
>   __lock_acquire from lock_acquire+0x148/0x3dc
>   lock_acquire from _raw_spin_lock_irqsave+0x50/0x6c
>   _raw_spin_lock_irqsave from __irq_get_desc_lock+0x58/0xa0
>   __irq_get_desc_lock from irq_set_irq_wake+0x2c/0x19c
>   irq_set_irq_wake from irq_set_irq_wake+0x13c/0x19c
>     [tail call from sunxi_pinctrl_irq_set_wake]
>   irq_set_irq_wake from gpio_keys_suspend+0x80/0x1a4
>   gpio_keys_suspend from gpio_keys_shutdown+0x10/0x2c
>   gpio_keys_shutdown from device_shutdown+0x180/0x224
>   device_shutdown from __do_sys_reboot+0x134/0x23c
>   __do_sys_reboot from ret_fast_syscall+0x0/0x1c
> 
> However, this can never deadlock because the upstream and downstream
> IRQs are never the same (nor do they even involve the same irqchip).
> 
> Silence this erroneous lockdep splat by applying what appears to be the
> usual fix of moving the GPIO IRQs to separate lockdep classes.
> 
> Fixes: a59c99d9eaf9 ("pinctrl: sunxi: Forward calls to irq_set_irq_wake")
> Reported-by: Guenter Roeck <linux@...ck-us.net>
> Signed-off-by: Samuel Holland <samuel@...lland.org>

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

Guenter

> ---
> 
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index 80d6750c74a6..9e6ed1175db3 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -36,6 +36,13 @@
>  #include "../core.h"
>  #include "pinctrl-sunxi.h"
>  
> +/*
> + * These lock classes tell lockdep that GPIO IRQs are in a different
> + * category than their parents, so it won't report false recursion.
> + */
> +static struct lock_class_key sunxi_pinctrl_irq_lock_class;
> +static struct lock_class_key sunxi_pinctrl_irq_request_class;
> +
>  static struct irq_chip sunxi_pinctrl_edge_irq_chip;
>  static struct irq_chip sunxi_pinctrl_level_irq_chip;
>  
> @@ -1555,6 +1562,8 @@ int sunxi_pinctrl_init_with_variant(struct platform_device *pdev,
>  	for (i = 0; i < (pctl->desc->irq_banks * IRQ_PER_BANK); i++) {
>  		int irqno = irq_create_mapping(pctl->domain, i);
>  
> +		irq_set_lockdep_class(irqno, &sunxi_pinctrl_irq_lock_class,
> +				      &sunxi_pinctrl_irq_request_class);
>  		irq_set_chip_and_handler(irqno, &sunxi_pinctrl_edge_irq_chip,
>  					 handle_edge_irq);
>  		irq_set_chip_data(irqno, pctl);
> -- 
> 2.33.1
>