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: <87pmklpqod.ffs@tglx>
Date:   Tue, 10 May 2022 13:34:10 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Thomas Pfaff <tpfaff@....com>
Cc:     linux-kernel@...r.kernel.org, linux-rt-users@...r.kernel.org
Subject: Re: [PATCH v3] irq/core: synchronize irq_thread startup

On Tue, May 10 2022 at 10:43, Thomas Pfaff wrote:
> On Mon, 2 May 2022, Thomas Gleixner wrote:
>> On Mon, May 02 2022 at 13:28, Thomas Pfaff wrote:
>> 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.

Well, I'd say pointless.

But it's not the real problem, it's the messenger which unearthed the
underlying issue. Even if you remove that call, the underlying problem
persists because the interrupt descriptor is in inconsistent state.

> 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.

No, it does not really take care about it. It can return with
irq_desc::threads_active > 0 due to the interrupt thread being stopped
before reaching the thread function. Think about shared interrupts.

> If it is called after free_irq(), the context is already lost.

That's correct.

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

Yes, that call is pointless.

Thanks,

        tglx