[v2] irq/core: synchronize irq_thread startup

From: Thomas Pfaff <tpfaff@pcs.com>

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

Signed-off-by: Thomas Pfaff <tpfaff@pcs.com>
---
v1-v2:
  - use already existing resources

Hi Thomas,

Thanks for this, a few comments below.

On Fri, 29 Apr 2022 12:52:48 +0100,
Thomas Pfaff <tpfaff@pcs.com> wrote:
> 
> From: Thomas Pfaff <tpfaff@pcs.com>
> 
> 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().
> 
> Signed-off-by: Thomas Pfaff <tpfaff@pcs.com>
> ---
> v1-v2:
>   - use already existing resources
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index 99cbdf55a8bd..dca57bed0d96 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -29,12 +29,14 @@ extern struct irqaction chained_action;
>   * IRQTF_WARNED    - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed
>   * IRQTF_AFFINITY  - irq thread is requested to adjust affinity
>   * IRQTF_FORCED_THREAD  - irq action is force threaded
> + * IRQTF_UP        - signals that irq thread is ready

nit: Why not call the flag IRQTF_READY then? I find it slightly more
readable than 'UP'.

>   */
>  enum {
>  	IRQTF_RUNTHREAD,
>  	IRQTF_WARNED,
>  	IRQTF_AFFINITY,
>  	IRQTF_FORCED_THREAD,
> +	IRQTF_UP,
>  };
>  
>  /*
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index f1d5a94c6c9f..7efa24629694 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1263,6 +1263,30 @@ static void irq_wake_secondary(struct irq_desc *desc, struct irqaction *action)
>  	raw_spin_unlock_irq(&desc->lock);
>  }
>  
> +/*
> + * Internal function to notify that irq_thread is ready
> + */
> +static void irq_thread_is_up(struct irq_desc *desc,
> +		struct irqaction *action)

nit again: the name of this function makes it look like a predicate.
The rest of the IRQ core uses the 'set' word to... set a bit.
Something like irq_thread_set_ready() would have my preference.

> +{
> +	set_bit(IRQTF_UP, &action->thread_flags);
> +	wake_up(&desc->wait_for_threads);
> +}
> +
> +/*
> + * Internal function to wake up irq_thread
> + * and wait until it is really up
> + */
> +static void wait_for_irq_thread_startup(struct irq_desc *desc,
> +		struct irqaction *action)

and this would be wait_for_irq_thread_ready().

> +{
> +	if (action && action->thread) {
> +		wake_up_process(action->thread);
> +		wait_event(desc->wait_for_threads,
> +			test_bit(IRQTF_UP, &action->thread_flags));
> +	}
> +}
> +
>  /*
>   * Interrupt handler thread
>   */
> @@ -1287,6 +1311,8 @@ static int irq_thread(void *data)
>  
>  	irq_thread_check_affinity(desc, action);
>  
> +	irq_thread_is_up (desc, action);

nit: extra space after the function.

> +
>  	while (!irq_wait_for_interrupt(action)) {
>  		irqreturn_t action_ret;
>  
> @@ -1522,6 +1548,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  		}
>  	}
>  
> +	init_waitqueue_head(&desc->wait_for_threads);
> +

I'm trying to convince myself that this one is safe.

It was so far only done when registering the first handler of a
threaded interrupt, while it is now done on every call to
__setup_irq().  However, this is now done outside of the protection of
any of the locks, meaning that a concurrent __setup_irq() for a shared
interrupt can now barge in and corrupt the wait queue.

So I don't think this is right. You may be able to hoist the
request_lock up, but I haven't checked what could break, if anything.

>  	/*
>  	 * Create a handler thread when a thread function is supplied
>  	 * and the interrupt does not nest into another interrupt
> @@ -1698,8 +1726,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  	}
>  
>  	if (!shared) {
> -		init_waitqueue_head(&desc->wait_for_threads);
> -
>  		/* Setup the type (level, edge polarity) if configured: */
>  		if (new->flags & IRQF_TRIGGER_MASK) {
>  			ret = __irq_set_trigger(desc,
> @@ -1795,14 +1821,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  
>  	irq_setup_timings(desc, new);
>  
> -	/*
> -	 * Strictly no need to wake it up, but hung_task complains
> -	 * when no hard interrupt wakes the thread up.
> -	 */
> -	if (new->thread)
> -		wake_up_process(new->thread);
> -	if (new->secondary)
> -		wake_up_process(new->secondary->thread);
> +	wait_for_irq_thread_startup(desc, new);
> +	wait_for_irq_thread_startup(desc, new->secondary);
>  
>  	register_irq_proc(irq, desc);
>  	new->dir = NULL;

Thanks,

	M.

On Fri, Apr 29 2022 at 17:08, Marc Zyngier wrote:
> On Fri, 29 Apr 2022 12:52:48 +0100,
> Thomas Pfaff <tpfaff@pcs.com> wrote:

 > +static void wait_for_irq_thread_startup(struct irq_desc *desc,
 > +		struct irqaction *action)

 and this would be wait_for_irq_thread_ready().

which is sill a misnomer as this actually wakes and waits.

>> @@ -1522,6 +1548,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>  		}
>>  	}
>>  
>> +	init_waitqueue_head(&desc->wait_for_threads);
>> +
>
> I'm trying to convince myself that this one is safe.
>
> It was so far only done when registering the first handler of a
> threaded interrupt, while it is now done on every call to
> __setup_irq().  However, this is now done outside of the protection of
> any of the locks, meaning that a concurrent __setup_irq() for a shared
> interrupt can now barge in and corrupt the wait queue.
>
> So I don't think this is right. You may be able to hoist the
> request_lock up, but I haven't checked what could break, if anything.

It can't be moved here, but I can see why Thomas wants to move it. With
a spurious wakeup of the irq thread (should not happen), the thread
would try to invoke wake_up() on a non initialize wait queue head.

Something like this should do the trick.

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 939d21cd55c3..0099b87dd853 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -407,6 +407,7 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
 	lockdep_set_class(&desc->lock, &irq_desc_lock_class);
 	mutex_init(&desc->request_mutex);
 	init_rcu_head(&desc->rcu);
+	init_waitqueue_head(&desc->wait_for_threads);
 
 	desc_set_defaults(irq, desc, node, affinity, owner);
 	irqd_set(&desc->irq_data, flags);
@@ -575,6 +576,7 @@ int __init early_irq_init(void)
 		raw_spin_lock_init(&desc[i].lock);
 		lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
 		mutex_init(&desc[i].request_mutex);
+		init_waitqueue_head(&desc[i].wait_for_threads);
 		desc_set_defaults(i, &desc[i], node, NULL, NULL);
 	}
 	return arch_early_irq_init();
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c03f71d5ec10..6a0942f4d068 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1683,8 +1683,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	if (!shared) {
-		init_waitqueue_head(&desc->wait_for_threads);
-
 		/* Setup the type (level, edge polarity) if configured: */
 		if (new->flags & IRQF_TRIGGER_MASK) {
 			ret = __irq_set_trigger(desc,

Thanks,

        tglx

On Fri, 29 Apr 2022 20:40:32 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Fri, Apr 29 2022 at 17:08, Marc Zyngier wrote:
> > On Fri, 29 Apr 2022 12:52:48 +0100,
> > Thomas Pfaff <tpfaff@pcs.com> wrote:
> 
>  > +static void wait_for_irq_thread_startup(struct irq_desc *desc,
>  > +		struct irqaction *action)
> 
>  and this would be wait_for_irq_thread_ready().
> 
> which is sill a misnomer as this actually wakes and waits.

Hey, I didn't say I picked the right color for that shed! ;-)

> 
> >> @@ -1522,6 +1548,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> >>  		}
> >>  	}
> >>  
> >> +	init_waitqueue_head(&desc->wait_for_threads);
> >> +
> >
> > I'm trying to convince myself that this one is safe.
> >
> > It was so far only done when registering the first handler of a
> > threaded interrupt, while it is now done on every call to
> > __setup_irq().  However, this is now done outside of the protection of
> > any of the locks, meaning that a concurrent __setup_irq() for a shared
> > interrupt can now barge in and corrupt the wait queue.
> >
> > So I don't think this is right. You may be able to hoist the
> > request_lock up, but I haven't checked what could break, if anything.
> 
> It can't be moved here, but I can see why Thomas wants to move it. With
> a spurious wakeup of the irq thread (should not happen), the thread
> would try to invoke wake_up() on a non initialize wait queue head.
> 
> Something like this should do the trick.
> 
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 939d21cd55c3..0099b87dd853 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -407,6 +407,7 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
>  	lockdep_set_class(&desc->lock, &irq_desc_lock_class);
>  	mutex_init(&desc->request_mutex);
>  	init_rcu_head(&desc->rcu);
> +	init_waitqueue_head(&desc->wait_for_threads);
>  
>  	desc_set_defaults(irq, desc, node, affinity, owner);
>  	irqd_set(&desc->irq_data, flags);
> @@ -575,6 +576,7 @@ int __init early_irq_init(void)
>  		raw_spin_lock_init(&desc[i].lock);
>  		lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
>  		mutex_init(&desc[i].request_mutex);
> +		init_waitqueue_head(&desc[i].wait_for_threads);
>  		desc_set_defaults(i, &desc[i], node, NULL, NULL);
>  	}
>  	return arch_early_irq_init();
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index c03f71d5ec10..6a0942f4d068 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1683,8 +1683,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  	}
>  
>  	if (!shared) {
> -		init_waitqueue_head(&desc->wait_for_threads);
> -
>  		/* Setup the type (level, edge polarity) if configured: */
>  		if (new->flags & IRQF_TRIGGER_MASK) {
>  			ret = __irq_set_trigger(desc,
> 

Indeed, it makes a lot of sense to fully initialise the irqdesc
structure at the point of allocation, rather than later.

Thanks,

	M.