linux-kernel - Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

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: <ZJo+wBMsyHvu/7Mj@sol>
Date:   Tue, 27 Jun 2023 09:43:28 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     YE Chengfeng <cyeaa@...nect.ust.hk>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andy@...nel.org" <andy@...nel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock
 deadlock issue

On Mon, Jun 26, 2023 at 05:50:47PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 26, 2023 at 9:23 AM Kent Gibson <warthog618@...il.com> wrote:
> >
> >
> > spin_lock_bh() should be sufficient, given that edge_irq_thread() is run
> > in a softirq?  That is faster and would allow the hard irq handlers to
> > still run, and timestamp the event, but inhibit the edge_irq_thread()
> > from being called on that CPU until the lock is released.
> > (hmmm, gpio_desc_to_lineinfo() also uses spin_lock_irqsave() but it is
> > never called from hard irq context, so there is a good chance I'm missing
> > something here??)
> > More on spin_lock choice below.
> 
> Again: this is incorrect - edge_irq_thread() doesn't execute in
> softirq context which can be verified by calling in_softirq() from it.
> 

Ok, that matches what I had initially thought.  Wading through the kernel
doc got me thinking the secondary handler was run as a softirq.
But it is a threaded irq used here, so the thread handler runs in a
kernel thread, as does the debounce_work_func() and hte thread handler
process_hw_ts_thread().
That's a relief.

While we are on the subject of spin_locks, why does
gpio_desc_to_lineinfo() use spin_lock_irqsave()?
I assume the _irq is necessary as the desc could be updated at interrupt
level, but AFAICT gpio_desc_to_lineinfo() is only ever called from process
context, so why not just spin_lock_irq()?

Cheers,
Kent.