linux-kernel - Re: [PATCH] genirq: Sanitize spurious interrupt detection of threaded irqs

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]

Date:	Tue, 12 Mar 2013 14:22:38 +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

OK. Thanks for the explanation.

However - even if not strictly necessary - wouldn't it
be simpler and the code easier to understand if
spurious interrupt detection just always would use the
deferred algorithm?
  - after handling by whatever scheme (hard, threaded, nested)
    the counter is incremented
  - the next hard IRQ calls note_interrupt() which checks
    if the counter has changed since last time.

Just my 2 cents...

Thanks again
- Till

On 03/08/2013 08:41 PM, Thomas Gleixner wrote:
> On Fri, 8 Mar 2013, Till Straumann wrote:
>> On 03/08/2013 05:12 PM, Thomas Gleixner wrote:
>>> On Fri, 8 Mar 2013, Till Straumann wrote:
>>>    
>>>> 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.
>>> atomic_t is going to stay 32 bit otherwise we'll have more horrible
>>> problems than that one.
>> I know. But this means that when the counter overflows 31 bits (2^31 - 1)
>> then it spills into the SPURIOUS_DEFERRED flag, right?
> Gah, yes. /me should stop doing overoptimizations :)
>   
>>>> 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.
>>> That's a different issue. The nested_irq handler is for interrupts
>>> which are demultiplexed by a primary threaded handler. That interrupt
>>> is never handled in hard interrupt context. It's always called from
>>> the context of the demultiplxing thread.
>> So you are saying that there 'handle_nested_irq()' can never be executed
>> from more than one thread for a single interrupt?
>>
>> I find, however, that e.g., the gpio-sx150x.c driver calls
>>
>> request_threaded_irq() with IRQF_SHARED set and it's thread_fn does call
>> handle_nested_irq(). It would thus be possible that multiple drivers
>> could share an interrupt and each driver would call handle_nested_irq()
>> which in-turn executes note_interrupt(). This would again raise the
>> issues we already discussed (note_interrupt() not serialized and thinking
>> that an interrupt was not handled because it was handled by a different
>> thread).
>>
>> Probably I'm missing something regarding the use of nested interrupts
>> - I would really appreciate if you could help me understand why
>> it should be OK for handle_nested_irq() to call note_interrupt().
> The thing about nested irqs is:
>
> main irq is threaded (requested by the driver for stuff like i2c)
>
> The handler of this irq reads a pending irq register in the chip and
> then invokes handle_nested_irq() for each of the pending bits.
>
> Those interrupts cannot be shared even if the driver request them as
> shared:
>
>          irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>          raw_spin_unlock_irq(&desc->lock);
>
>          action_ret = action->thread_fn(action->irq, action->dev_id);
>          if (!noirqdebug)
>                  note_interrupt(irq, desc, action_ret);
>
>          raw_spin_lock_irq(&desc->lock);
>          irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>
> So there is no loop over action->next. And even if that code would
> loop over action next, then it still would be serialized
>
> main irq is raised
>
>       -> wake thread
>
> thread runs
>
>         read pending reg()
>
>         for each pending bit {
>
>         	   handle_nested_irq();
> 		action = desc->action;
>
> 		while (action) {
> 		      action->thread_fn()
> 		      action = action->next)
> 		}
> 		note_interrupt();
>         }
>
> thread done
>
> Hope that helps. Thanks,
>
>       tglx
>
>        
>
>
>

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