linux-kernel - Re: [PATCH] Input: gpio-keys - fix a sleep while atomic with PREEMPT

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: <661af124-3072-4dcf-b613-ec3e48549626@foss.st.com>
Date: Tue, 27 May 2025 15:36:37 +0200
From: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
CC: Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Clark Williams
	<clrkwllms@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Paul Cercueil
	<paul@...pouillou.net>, <linux-input@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-rt-devel@...ts.linux.dev>,
        Fabrice
 Gasnier <fabrice.gasnier@...s.st.com>
Subject: Re: [PATCH] Input: gpio-keys - fix a sleep while atomic with
 PREEMPT_RT

Hello Sebastian,

On 5/26/25 16:13, Sebastian Andrzej Siewior wrote:
> On 2025-05-26 15:56:29 [+0200], Gatien Chevallier wrote:
>> From: Fabrice Gasnier <fabrice.gasnier@...s.st.com>
>>
>> When enabling PREEMPT_RT, the gpio_keys_irq_timer() callback runs in
>> hard irq context, but the input_event() takes a spin_lock, which isn't
>> allowed there as it is converted to a rt_spin_lock().
>>
>> [ 4054.289999] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>> [ 4054.290028] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 0, name: swapper/0
>> ...
>> [ 4054.290195]  __might_resched+0x13c/0x1f4
>> [ 4054.290209]  rt_spin_lock+0x54/0x11c
>> [ 4054.290219]  input_event+0x48/0x80
>> [ 4054.290230]  gpio_keys_irq_timer+0x4c/0x78
>> [ 4054.290243]  __hrtimer_run_queues+0x1a4/0x438
>> [ 4054.290257]  hrtimer_interrupt+0xe4/0x240
>> [ 4054.290269]  arch_timer_handler_phys+0x2c/0x44
>> [ 4054.290283]  handle_percpu_devid_irq+0x8c/0x14c
>> [ 4054.290297]  handle_irq_desc+0x40/0x58
>> [ 4054.290307]  generic_handle_domain_irq+0x1c/0x28
>> [ 4054.290316]  gic_handle_irq+0x44/0xcc
>>
>> Considering the gpio_keys_irq_isr() can run in any context, e.g. it can
>> be threaded, it seems there's no point in requesting the timer isr to
>> run in hard irq context.
>>
>> So relax the hrtimer not to use the hard context. This requires the
>> spin_lock to be added back in gpio_keys_irq_timer().
> 
> Why does it? This needs to be explained or it deserves an independent
> patch/ fix. This flag change makes not difference on !PREEMPT_RT and so
> should be the requirements for locking here.
> 

Can you elaborate on "This flag change makes not difference on
!PREEMPT_RT" please? IIUC,this makes the callback not run in hard IRQ
context, even in !PREEMPT_RT, no?

Regarding the need of the spin_lock: gpio_keys_irq_timer() and
gpio_keys_irq_isr() appear to access the same resources. Can't we
have a concurrent access on it from:
HR timer interrupt // GPIO interrupt?

But looking back at the patch, this situation does not depend on
the HRTIMER_MODE_REL_HARD flag. So maybe it should be addressed
separately.

On the other hand, I should use the new
guard(spinlock_irqsave)(&bdata->lock);

>> Fixes: 019002f20cb5 ("Input: gpio-keys - use hrtimer for release timer")
>> Suggested-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...s.st.com>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@...s.st.com>
> 
> Sebastian