linux-kernel - Re: [PATCH] pinctrl: mcp23s08: Fix sleeping in atomic context due to regmap locking

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] [day] [month] [year] [list]

Message-ID: <CACRpkda4UGxAjXFOAu5SA9GX=9eNpXcxqUzkabmtA_sCqUG7=Q@mail.gmail.com>
Date: Tue, 17 Dec 2024 15:11:10 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Evgenii Shatokhin <e.shatokhin@...ro.com>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Nikita Shubin <nikita.shubin@...uefel.me>, linux@...ro.com, stable@...r.kernel.org
Subject: Re: [PATCH] pinctrl: mcp23s08: Fix sleeping in atomic context due to
 regmap locking

On Mon, Dec 9, 2024 at 8:47 AM Evgenii Shatokhin <e.shatokhin@...ro.com> wrote:

> If a device uses MCP23xxx IO expander to receive IRQs, the following
> bug can happen:
>
>   BUG: sleeping function called from invalid context
>     at kernel/locking/mutex.c:283
>   in_atomic(): 1, irqs_disabled(): 1, non_block: 0, ...
>   preempt_count: 1, expected: 0
>   ...
>   Call Trace:
>   ...
>   __might_resched+0x104/0x10e
>   __might_sleep+0x3e/0x62
>   mutex_lock+0x20/0x4c
>   regmap_lock_mutex+0x10/0x18
>   regmap_update_bits_base+0x2c/0x66
>   mcp23s08_irq_set_type+0x1ae/0x1d6
>   __irq_set_trigger+0x56/0x172
>   __setup_irq+0x1e6/0x646
>   request_threaded_irq+0xb6/0x160
>   ...
>
> We observed the problem while experimenting with a touchscreen driver which
> used MCP23017 IO expander (I2C).
>
> The regmap in the pinctrl-mcp23s08 driver uses a mutex for protection from
> concurrent accesses, which is the default for regmaps without .fast_io,
> .disable_locking, etc.
>
> mcp23s08_irq_set_type() calls regmap_update_bits_base(), and the latter
> locks the mutex.
>
> However, __setup_irq() locks desc->lock spinlock before calling these
> functions. As a result, the system tries to lock the mutex whole holding
> the spinlock.
>
> It seems, the internal regmap locks are not needed in this driver at all.
> mcp->lock seems to protect the regmap from concurrent accesses already,
> except, probably, in mcp_pinconf_get/set.
>
> mcp23s08_irq_set_type() and mcp23s08_irq_mask/unmask() are called under
> chip_bus_lock(), which calls mcp23s08_irq_bus_lock(). The latter takes
> mcp->lock and enables regmap caching, so that the potentially slow I2C
> accesses are deferred until chip_bus_unlock().
>
> The accesses to the regmap from mcp23s08_probe_one() do not need additional
> locking.
>
> In all remaining places where the regmap is accessed, except
> mcp_pinconf_get/set(), the driver already takes mcp->lock.
>
> This patch adds locking in mcp_pinconf_get/set() and disables internal
> locking in the regmap config. Among other things, it fixes the sleeping
> in atomic context described above.
>
> Fixes: 8f38910ba4f6 ("pinctrl: mcp23s08: switch to regmap caching")
> Cc: stable@...r.kernel.org
> Signed-off-by: Evgenii Shatokhin <e.shatokhin@...ro.com>

Excellent analysis, patch applied!

Yours,
Linus Walleij