linux-kernel - Re: [PATCH 2/2] irq: Cleanup context state transitions in 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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 22 Feb 2013 15:33:51 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Frederic Weisbecker <fweisbec@...il.com>
cc:	LKML <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH 2/2] irq: Cleanup context state transitions in
 irq_exit()

On Fri, 22 Feb 2013, Frederic Weisbecker wrote:
> The irq code is run under HARDIRQ_OFFSET preempt offset until
> we reach the softirq code. Then it's substracted, leaving the
> preempt count to 0, whether we have pending softirqs or not.
> 
> Afterward, if we have softirqs to run, we'll run them under
> the SOFTIRQ_OFFSET then set the preempt offset back to 0
> after softirqs completion.
> 
> The rest of the code in irq_exit(), mainly tick_nohz_irq_exit()
> and rcu_irq_exit(), are executed with this NULL preempt offset.
> 
> The semantics here are not very intuitive. They leave several portions
> of the code into some half-defined context state, where in_interrupt()
> returns false while we actually are in an interrupt.
> 
> In order to make the context definition less confusing, let's
> cover the whole code in irq_exit() under HARDIRQ_OFFSET, except
> for the softirq processing where we switch back and forth
> from HARDIRQ_OFFSET to SOFTIRQ_OFFSET without leaving a gap
> in the context definition.
> 
> There is a drawback though: raise_softirq() relies on the previous
> semantics considering that as long as we are in_interrupt(), the
> pending softirq will be handled in the end of the interrupt. Otherwise
> it kicks ksoftirqd so the softirq is always processed.
> 
> Now tick_nohz_irq_exit() and rcu_irq_exit() can raise softirqs
> themselves. Since these functions were not under the HARDIRQ_OFFSET,
> calling raise_softirq() resulted in waking up the ksoftirqd thread.
> This is correct because invoke_softirq() has already been invoked at
> this stage. But with this patch they are now under HARDIRQ_OFFSET and
> raise_softirq() wrongly thinks invoke_softirq() has yet to be called.
> In the end, this could leave the softirq unprocessed for a while.
> 
> So as Thomas suggested me, this also brings a check in the end of
> irq_exit() that looks for pending softirqs raised after invoke_softirq()
> and wake up ksoftirqd if needed.
> 
> Given that the cleanup on the contexts transition involves that
> new unpretty workaround, I have mixed feelings about this patch so I
> tagged it as "RFC" and I wait for your opinion.

Of course, I'm all for it because I suggested that solution :)

Seriously, I prefer to have in_interrupt() and in_irq() working in the
functions which are called from irq_exit() rather than having special
case workarounds inside of them. We are in interrupt context at this
point and not in some magic virtual place. 

The minimal extra check at the end of irq_exit() is way better than
any other special cased workaround and the softirq stuff is really the
only thing which needs to be taken care of. Anything else just works.

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/