[PATCH 3/3] do_exit(): do not panic if exiting thread is not serving an interrupt

[PATCH 3/3] do_exit(): do not panic if exiting thread is not serving an interrupt

[Alexander Gordeev]

Currently a crashed and killed forced oneshot threaded handler hits
in_interrupt() check in do_exit() and panics. As result, the code that
cleans up IRQ descriptor never not get called and IRQ line stays masked.

Similarly non-forced oneshot threaded handlers that crashed while holding
bh lock leave a IRQ line masked.

Regular threaded handlers that crashed while holding bh simply panic,
although they could have just terminate loudly.

This fix allows IRQ threaded handlers get killed gracefully instead of
panicking.

Since introduction of SOFTIRQ_DISABLE_OFFSET in 75e1056 we can differ
between bh being serviced and bh being disabled. Use this ability to
avoid unnecessary crashes when a exiting thread explicitly disabled bh
and is not serving any softirq. Still we will get the regular warning
that exiting thread is in atomic context.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 include/linux/hardirq.h |    4 ++++
 kernel/exit.c           |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index bb7f309..93aca12 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -82,11 +82,15 @@
  * Are we in a softirq context? Interrupt context?
  * in_softirq - Are we currently processing softirq or have bh disabled?
  * in_serving_softirq - Are we currently processing softirq?
+ * in_serving_interrupt - Are we currently processing softirq, nmi or
+ *                        hardware interrupt?
  */
 #define in_irq()		(hardirq_count())
 #define in_softirq()		(softirq_count())
 #define in_interrupt()		(irq_count())
 #define in_serving_softirq()	(softirq_count() & SOFTIRQ_OFFSET)
+#define in_serving_interrupt()	(preempt_count() & (HARDIRQ_MASK \
+					 | SOFTIRQ_OFFSET | NMI_MASK))
 
 /*
  * Are we in NMI context?
diff --git a/kernel/exit.c b/kernel/exit.c
index 752d2c0..0c78ae6 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -896,7 +896,7 @@ void do_exit(long code)
 
 	WARN_ON(blk_needs_flush_plug(tsk));
 
-	if (unlikely(in_interrupt()))
+	if (unlikely(in_serving_interrupt()))
 		panic("Aiee, killing interrupt handler!");
 	if (unlikely(!tsk->pid))
 		panic("Attempted to kill the idle task!");
-- 
1.7.7.6

Re: [PATCH 3/3] do_exit(): do not panic if exiting thread is not serving an interrupt

[Thomas Gleixner]

On Mon, 19 Mar 2012, Alexander Gordeev wrote:

> Currently a crashed and killed forced oneshot threaded handler hits
> in_interrupt() check in do_exit() and panics. As result, the code that
> cleans up IRQ descriptor never not get called and IRQ line stays masked.
> 
> Similarly non-forced oneshot threaded handlers that crashed while holding
> bh lock leave a IRQ line masked.
> 
> Regular threaded handlers that crashed while holding bh simply panic,
> although they could have just terminate loudly.
> 
> This fix allows IRQ threaded handlers get killed gracefully instead of
> panicking.
> 
> Since introduction of SOFTIRQ_DISABLE_OFFSET in 75e1056 we can differ
> between bh being serviced and bh being disabled. Use this ability to
> avoid unnecessary crashes when a exiting thread explicitly disabled bh
> and is not serving any softirq. Still we will get the regular warning
> that exiting thread is in atomic context.

Hmm, this applies for all threads which exit with bh disabled. We risk
data corruption this way as the crash of a task might happen within a
data set manipulation protected by bh_disable.

Not sure whether the chance to get debug information from the machine
is worth the risk of data corruption causes follow up problems.
 
Thanks,

	tglx


> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  include/linux/hardirq.h |    4 ++++
>  kernel/exit.c           |    2 +-
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index bb7f309..93aca12 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -82,11 +82,15 @@
>   * Are we in a softirq context? Interrupt context?
>   * in_softirq - Are we currently processing softirq or have bh disabled?
>   * in_serving_softirq - Are we currently processing softirq?
> + * in_serving_interrupt - Are we currently processing softirq, nmi or
> + *                        hardware interrupt?
>   */
>  #define in_irq()		(hardirq_count())
>  #define in_softirq()		(softirq_count())
>  #define in_interrupt()		(irq_count())
>  #define in_serving_softirq()	(softirq_count() & SOFTIRQ_OFFSET)
> +#define in_serving_interrupt()	(preempt_count() & (HARDIRQ_MASK \
> +					 | SOFTIRQ_OFFSET | NMI_MASK))
>  
>  /*
>   * Are we in NMI context?
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 752d2c0..0c78ae6 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -896,7 +896,7 @@ void do_exit(long code)
>  
>  	WARN_ON(blk_needs_flush_plug(tsk));
>  
> -	if (unlikely(in_interrupt()))
> +	if (unlikely(in_serving_interrupt()))
>  		panic("Aiee, killing interrupt handler!");
>  	if (unlikely(!tsk->pid))
>  		panic("Attempted to kill the idle task!");
> -- 
> 1.7.7.6
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 3/3] do_exit(): do not panic if exiting thread is not serving an interrupt

[Alexander Gordeev]

On Thu, Mar 22, 2012 at 12:56:55PM +0100, Thomas Gleixner wrote:
> On Mon, 19 Mar 2012, Alexander Gordeev wrote:
> 
> > Currently a crashed and killed forced oneshot threaded handler hits
> > in_interrupt() check in do_exit() and panics. As result, the code that
> > cleans up IRQ descriptor never not get called and IRQ line stays masked.
> > 
> > Similarly non-forced oneshot threaded handlers that crashed while holding
> > bh lock leave a IRQ line masked.
> > 
> > Regular threaded handlers that crashed while holding bh simply panic,
> > although they could have just terminate loudly.
> > 
> > This fix allows IRQ threaded handlers get killed gracefully instead of
> > panicking.
> > 
> > Since introduction of SOFTIRQ_DISABLE_OFFSET in 75e1056 we can differ
> > between bh being serviced and bh being disabled. Use this ability to
> > avoid unnecessary crashes when a exiting thread explicitly disabled bh
> > and is not serving any softirq. Still we will get the regular warning
> > that exiting thread is in atomic context.
> 
> Hmm, this applies for all threads which exit with bh disabled. We risk
> data corruption this way as the crash of a task might happen within a
> data set manipulation protected by bh_disable.

True. But we live with this as we do exit with preemption disabled. Are bh are
terribly different in this regard?

Anyway, I do not have strong opinion here. My point is letting innocent devices
on the shared irq line to go on worth considering.

> Not sure whether the chance to get debug information from the machine
> is worth the risk of data corruption causes follow up problems.

I would judge: no ;)

>  
> Thanks,
> 
> 	tglx
> 
> 
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  include/linux/hardirq.h |    4 ++++
> >  kernel/exit.c           |    2 +-
> >  2 files changed, 5 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> > index bb7f309..93aca12 100644
> > --- a/include/linux/hardirq.h
> > +++ b/include/linux/hardirq.h
> > @@ -82,11 +82,15 @@
> >   * Are we in a softirq context? Interrupt context?
> >   * in_softirq - Are we currently processing softirq or have bh disabled?
> >   * in_serving_softirq - Are we currently processing softirq?
> > + * in_serving_interrupt - Are we currently processing softirq, nmi or
> > + *                        hardware interrupt?
> >   */
> >  #define in_irq()		(hardirq_count())
> >  #define in_softirq()		(softirq_count())
> >  #define in_interrupt()		(irq_count())
> >  #define in_serving_softirq()	(softirq_count() & SOFTIRQ_OFFSET)
> > +#define in_serving_interrupt()	(preempt_count() & (HARDIRQ_MASK \
> > +					 | SOFTIRQ_OFFSET | NMI_MASK))
> >  
> >  /*
> >   * Are we in NMI context?
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index 752d2c0..0c78ae6 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -896,7 +896,7 @@ void do_exit(long code)
> >  
> >  	WARN_ON(blk_needs_flush_plug(tsk));
> >  
> > -	if (unlikely(in_interrupt()))
> > +	if (unlikely(in_serving_interrupt()))
> >  		panic("Aiee, killing interrupt handler!");
> >  	if (unlikely(!tsk->pid))
> >  		panic("Attempted to kill the idle task!");
> > -- 
> > 1.7.7.6
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com