linux-kernel - Re: current_thread_info() not respecting program order with gcc 4.8.x

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, 19 Nov 2013 17:21:01 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Jakub Jelinek <jakub@...hat.com>
Cc:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	linux-kernel@...r.kernel.org, Will Deacon <will.deacon@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	lttng-dev@...ts.lttng.org, Nathan Lynch <Nathan_Lynch@...tor.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Richard Henderson <rth@...ddle.net>
Subject: Re: current_thread_info() not respecting program order with gcc 4.8.x

On Tue, Nov 19, 2013 at 05:13:22PM +0100, Jakub Jelinek wrote:
> On Tue, Nov 19, 2013 at 04:57:49PM +0100, Peter Zijlstra wrote:
> > On Tue, Nov 19, 2013 at 03:29:12PM +0000, Mathieu Desnoyers wrote:
> > > However, looking at ARM arch/arm/include/asm/thread_info.h:
> > > 
> > > static inline struct thread_info *current_thread_info(void) { register
> > > unsigned long sp asm ("sp"); return (struct thread_info *)(sp &
> > > ~(THREAD_SIZE - 1)); }
> > > 
> > > The inline assembly has no clobber and is not volatile. (this is also
> > > true for all other architectures I've looked at so far, which includes
> > > x86 and powerpc)
> 
> The above is not inline assembly, it is a local register variable extension,
> see
> http://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/Local-Reg-Vars.html#Local-Reg-Vars
> 
> > > Since each current_thread_info() is a different asm ("sp") without
> > > clobber nor volatile, AFAIU, the compiler is within its right to
> > > reorder them.
> 
> Sure.
> 
> > > One possible solution to this might be to add "memory" clobber and
> > > volatile to this inline asm, but I fear it would put way too much
> > > constraints on the compiler optimizations (too heavyweight).
> 
> As it is not inline asm extension, you can't.
> Of course you could add asm volatile ("" : "=r" (ret) : "0" (sp));
> or similar and thus make it a barrier, but I think the current
> definition of current_thread_info meant to avoid all that extra
> overhead.  Why does it matter when sp is different between the
> current_thread_info () calls?  As long as sp & ~(THREAD_SIZE - 1) is
> the same, it shouldn't make a difference.  Or is the call in between those
> changing sp to something else?

So what appears to be happening is that:

    preempt_enable()
 	barrier();
 	current_thread_info()->preempt_count--;

    (1)

    might_sleep()
 	if (current_thread_info()->preempt_count)
		/* raise all bloody hell */

Gets re-ordered like:

	barrier();
	if (current_thread_info()->preempt_count)
		/* raise hell */
	current_thread_info()->preempt_count--;


And by inserting a barrier() at (1) things revert to normal and work
again.

The "sp" reg value itself doesn't change here.
--
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/