linux-kernel - Re: [PATCH v3] irq/core: synchronize irq

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: <e64aa9cb-a1ae-7534-8bf1-446d9d8c512@pcs.com>
Date:   Tue, 10 May 2022 10:43:15 +0200
From:   Thomas Pfaff <tpfaff@....com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     <linux-kernel@...r.kernel.org>, <linux-rt-users@...r.kernel.org>
Subject: Re: [PATCH v3] irq/core: synchronize irq_thread startup

On Mon, 2 May 2022, Thomas Gleixner wrote:

> On Mon, May 02 2022 at 13:28, Thomas Pfaff wrote:
> > While running
> > "while /bin/true; do setserial /dev/ttyS0 uart none;
> > setserial /dev/ttyS0 uart 16550A; done"
> > on a kernel with threaded irqs, setserial is hung after some calls.
> >
> > setserial opens the device, this will install an irq handler if the uart is
> > not none, followed by TIOCGSERIAL and TIOCSSERIAL ioctls.
> > Then the device is closed. On close, synchronize_irq() is called by
> > serial_core.
> 
> This comment made me look deeper because I expected that free_irq()
> would hang.
> 
> But free_irq() stopped issuing synchronize_irq() with commit
> 519cc8652b3a ("genirq: Synchronize only with single thread on
> free_irq()"). And that turns out to be the root cause of the problem.
> I should have caught that back then, but in hindsight ....
> 

Sorry for coming back to this again late, but this makes me believe that 
the real problem for the freeze in setserial is that uart_port_shutdown() 
is calling synchronize_irq() after free_irq(), which is illegal in my 
opinion.

It can be done only before the interrupt thread is stopped, and free_irq() 
itself is already taking care about synchronizing, no matter if its done by 
__synchronize_hardirq() or by synchronize_irq(), like it was before commit 
519cc8652b3a.
If it is called after free_irq(), the context is already lost.

I am not sure about all the other drivers, but at least serial_core should 
be fixed if you agree.

Thanks,
    Thomas