linux-kernel - Re: [PATCH] irq: 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] [day] [month] [year] [list]

Message-ID: <87r15huscg.ffs@tglx>
Date:   Thu, 28 Apr 2022 17:42:55 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Thomas Pfaff <tpfaff@....com>, linux-kernel@...r.kernel.org
Cc:     linux-rt-users@...r.kernel.org, Marc Zyngier <maz@...nel.org>
Subject: Re: [PATCH] irq: synchronize irq_thread startup

Thomas,

On Thu, Apr 28 2022 at 12:49, Thomas Pfaff wrote:

please always Cc maintainers when sending patches and please also have a
look at:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

> 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.
>
> If the close comes too fast, the irq_thread does not really start,
> it is terminated immediately without going into irq_thread().
> But an interrupt might already been handled by
> irq_default_primary_handler(), going to __irq_wake_thread() and
> incrementing threads_active.
> If this happens, synchronize_irq() will hang forever, because the
> irq_thread is already dead, and threads_active will never be decremented.
>
> The fix is to make sure that the irq_thread is really started
> during __setup_irq().

Right. Nice detective work.

> @@ -127,6 +129,8 @@ struct irqaction {
>  	unsigned int		flags;
>  	unsigned long		thread_flags;
>  	unsigned long		thread_mask;
> +	atomic_t		thread_is_up;
> +	wait_queue_head_t	wait_for_thread;

I don't think we need any of this extra ballast per irqaction.

irqaction::thread_flags is already there and can be used as indicator.
There are enough bits left.

irq_desc already has a wait queue, which is used for synchronize_irq()
to wait for threads to complete. That wait queue is unused at the point
where an interrupt is set up, so there is no reason to introduce a new
one which is only used once.

This is not a problem in the rare case that the interrupt fires and
wakes the thread which then in consequence might wake the waiter via
wake_threads_waitq().

> -	if (new->thread)
> +	if (new->thread) {
>  		wake_up_process(new->thread);
> -	if (new->secondary)
> +		wait_event(new->wait_for_thread,
> +			   atomic_read(&new->thread_is_up));

Please stick this into a helper function.

Thanks,

        tglx