| 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
| ||
|
Message-ID: <5139F9E8.8090402@slac.stanford.edu>
Date: Fri, 08 Mar 2013 15:47:04 +0100
From: Till Straumann <strauman@...c.stanford.edu>
To: Thomas Gleixner <tglx@...utronix.de>
CC: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] genirq: Sanitize spurious interrupt detection of threaded
irqs
Thomas.
Thanks for your quick reply. I had a look at the patch and I have a few
comments.
1) I'm not sure adding the SPURIOUS_DEFERRED flag into
threads_handled_last is OK
- what happens if the atomic_t counter can hold more than 31 bits?
In this case,
when thread handlers increment the counter there is interference
with the flag.
If this is not harmful then it is at least ugly.
I'm not as familiar with the code as you are but wouldn't it be
simpler to always
defer spurious detection thus avoiding to have to keep track of the
state (deferral
active/inactive)? I.e., if any primary handler returns IRQ_HANDLED then
we simply increment the counter. note_interrupt() could then always
compare the
previous count to the current count and if they are equal conclude
that the interrupt
was not handled:
handle_irq_event_percpu()
{
...
if (!noirqdebug)
note_interrupt(irq, desc, retval);
if ( (retval & IRQ_HANDLED) )
atomic_inc(&desc->threads_handled);
}
and in 'note_interrupt()'
handled = atomic_read(&desc->threads_handled);
if ( desc->threads_handled_last == handled ) {
action_ret = IRQ_NONE;
} else {
action_ret = IRQ_HANDLED;
desc->threads_handled_last = handled;
}
Either way - I'm not sure what deferral does to the part of the
algorithm
in note_interrupt() which deals with misrouted interrupts since the
'action_ret' that goes into try_misrouted_irq() is delayed by one
interrupt
cycle.
2) note_interrupt is also called from irq/chip.c:handle_nested_irq() and
I believe
this routine would also need to increment the 'threads_handled'
counter rather
than calling note_interrupt.
Cheers
- Till
On 03/07/2013 02:53 PM, Thomas Gleixner wrote:
> Till reported that he spurious interrupt detection of threaded
> interrupts is broken in two ways:
>
> - note_interrupt() is called for each action thread of a shared
> interrupt line. That's wrong as we are only interested whether none
> of the device drivers felt responsible for the interrupt, but by
> calling multiple times for a single interrupt line we account
> IRQ_NONE even if one of the drivers felt responsible.
>
> - note_interrupt() when called from the thread handler is not
> serialized. That leaves the members of irq_desc which are used for
> the spurious detection unprotected.
>
> To solve this we need to defer the spurious detection of a threaded
> interrupt to the next hardware interrupt context where we have
> implicit serialization.
>
> If note_interrupt is called with action_ret == IRQ_WAKE_THREAD, we
> check whether the previous interrupt requested a deferred check. If
> not, we request a deferred check for the next hardware interrupt and
> return.
>
> If set, we check whether one of the interrupt threads signaled
> success. Depending on this information we feed the result into the
> spurious detector.
>
> If one primary handler of a shared interrupt returns IRQ_HANDLED we
> disable the deferred check of irq threads on the same line, as we have
> found at least one device driver who cared.
>
> Reported-by: Till Straumann <strauman@...c.stanford.edu>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> include/linux/irqdesc.h | 4 +++
> kernel/irq/internals.h | 4 +++
> kernel/irq/manage.c | 4 +--
> kernel/irq/spurious.c | 57 ++++++++++++++++++++++++++++++++++++++++++++----
> 4 files changed, 63 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/include/linux/irqdesc.h
> ===================================================================
> --- linux-2.6.orig/include/linux/irqdesc.h
> +++ linux-2.6/include/linux/irqdesc.h
> @@ -27,6 +27,8 @@ struct irq_desc;
> * @irq_count: stats field to detect stalled irqs
> * @last_unhandled: aging timer for unhandled count
> * @irqs_unhandled: stats field for spurious unhandled interrupts
> + * @threads_handled: stats field for deferred spurious detection of threaded handlers
> + * @threads_handled_last: comparator field for deferred spurious detection of theraded handlers
> * @lock: locking for SMP
> * @affinity_hint: hint to user space for preferred irq affinity
> * @affinity_notify: context for notification of affinity changes
> @@ -52,6 +54,8 @@ struct irq_desc {
> unsigned int irq_count; /* For detecting broken IRQs */
> unsigned long last_unhandled; /* Aging timer for unhandled count */
> unsigned int irqs_unhandled;
> + atomic_t threads_handled;
> + int threads_handled_last;
> raw_spinlock_t lock;
> struct cpumask *percpu_enabled;
> #ifdef CONFIG_SMP
> Index: linux-2.6/kernel/irq/internals.h
> ===================================================================
> --- linux-2.6.orig/kernel/irq/internals.h
> +++ linux-2.6/kernel/irq/internals.h
> @@ -55,6 +55,10 @@ enum {
> IRQS_SUSPENDED = 0x00000800,
> };
>
> +/* Bits for spurious detection of threaded handlers */
> +#define SPURIOUS_MASK 0x7fffffff
> +#define SPURIOUS_DEFERRED 0x80000000
> +
> #include "debug.h"
> #include "settings.h"
>
> Index: linux-2.6/kernel/irq/manage.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/manage.c
> +++ linux-2.6/kernel/irq/manage.c
> @@ -867,8 +867,8 @@ static int irq_thread(void *data)
> irq_thread_check_affinity(desc, action);
>
> action_ret = handler_fn(desc, action);
> - if (!noirqdebug)
> - note_interrupt(action->irq, desc, action_ret);
> + if (!noirqdebug && action_ret == IRQ_HANDLED)
> + atomic_inc(&desc->threads_handled);
>
> wake_threads_waitq(desc);
> }
> Index: linux-2.6/kernel/irq/spurious.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/spurious.c
> +++ linux-2.6/kernel/irq/spurious.c
> @@ -271,15 +271,64 @@ void note_interrupt(unsigned int irq, st
> if (desc->istate & IRQS_POLL_INPROGRESS)
> return;
>
> - /* we get here again via the threaded handler */
> - if (action_ret == IRQ_WAKE_THREAD)
> - return;
> -
> if (bad_action_ret(action_ret)) {
> report_bad_irq(irq, desc, action_ret);
> return;
> }
>
> + /*
> + * We cannot call note_interrupt from the threaded handler. So
> + * the threaded handlers store whether they sucessfully
> + * handled an interrupt. Here is the right place to take care
> + * of this.
> + */
> + if (action_ret & IRQ_WAKE_THREAD) {
> + /*
> + * There is a thread woken. Check whether one of the
> + * shared primary handlers returned IRQ_HANDLED. If
> + * not we defer the spurious detection to the next
> + * interrupt.
> + */
> + if (action_ret == IRQ_WAKE_THREAD) {
> + int handled;
> +
> + /*
> + * We use bit 31 of thread_handled_last to
> + * note the deferred spurious detection
> + * active. No locking necessary as
> + * thread_handled_last is only accessed here
> + * and we have the guarantee that hard
> + * interrupts are not reentrant.
> + */
> + if (!(desc->threads_handled_last & SPURIOUS_DEFERRED)) {
> + desc->threads_handled_last |= SPURIOUS_DEFERRED;
> + return;
> + }
> + /*
> + * Check whether one of the threaded handlers
> + * returned IRQ_HANDLED since the last
> + * interrupt happened. Or the spurious
> + * deferred flag for ease of comparison and
> + * preservation in the writeback.
> + */
> + handled = atomic_read(&desc->threads_handled);
> + handled |= SPURIOUS_DEFERRED;
> + if (handled != desc->threads_handled_last) {
> + action_ret = IRQ_HANDLED;
> + desc->threads_handled_last = handled;
> + } else {
> + action_ret = IRQ_NONE;
> + }
> + } else {
> + /*
> + * One of the primary handlers returned
> + * IRQ_HANDLED. So we don't care about the
> + * threaded handlers on the same line.
> + */
> + desc->threads_handled_last &= ~SPURIOUS_DEFERRED;
> + }
> + }
> +
> if (unlikely(action_ret == IRQ_NONE)) {
> /*
> * If we are seeing only the odd spurious IRQ caused by
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists