linux-kernel - Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

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-next>] [day] [month] [year] [list]

Date:	Mon, 29 Feb 2016 11:32:21 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ben Maurer <bmaurer@...com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Russell King <linux@....linux.org.uk>,
	linux-api <linux-api@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Dave Watson <davejwatson@...com>,
	rostedt <rostedt@...dmis.org>,
	Andy Lutomirski <luto@...capital.net>,
	Will Deacon <will.deacon@....com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Chris Lameter <cl@...ux.com>, Andi Kleen <andi@...stfloor.org>,
	Josh Triplett <josh@...htriplett.org>,
	Paul Turner <pjt@...gle.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Andrew Hunter <ahh@...gle.com>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of
 running thread

On Sun, Feb 28, 2016 at 12:39:54AM +0000, Mathieu Desnoyers wrote:

> /* This structure needs to be aligned cache line size. */
> struct thread_local_abi {
>   int32_t  cpu_id;
>   uint32_t rseq_seqnum;
>   uint64_t rseq_post_commit_ip;
>   /* Add new fields at the end. */ 
> } __attribute__((packed));

I would really not use packed; that can lead to horrible layout.

Suppose someone would add:

	uint32_t foo;
	uint64_t bar;

With packed, you get an unaligned uint64_t in there, which is horrible.
Without packed, you get a hole, which you can later fill.

> /* Thread local ABI system calls. */ 
> 
> int thread_local_abi_len(size_t *features_mask_len, size_t *tlabi_len); 

See below; maybe we can fudge the register call to return the size when
called 'right', maybe that'll end up too ugly, dunno. But I don't think
we need the feature mask bits.

Maybe: TLA_FLAG_GETSIZE ?

> int thread_local_abi_features(uint8_t *mask); 

Not sure you need this; see below. Either you know about a
TLA_ENABLE_feat flag and you can attempt enabling it (failing if the
kernel doesn't support it), or you don't, in which case you won't
attempt use.

> int thread_local_abi_register(struct thread_local_abi *tlabi); 

This has the problem that the moment you register for this, we must have
all features enabled. And esp. the rseq stuff has non-trivial overhead.

I would much rather have something where we only enable the features
actually used by the program at hand.

Also, every syscall should have a flags argument, so maybe we can do
something like:

	#define TLA_ENABLE_CPU		0x01
	#define TLA_ENABLE_RSEQ		0x03 /* RSEQ must imply CPU */

	int thread_local_abi_register(struct tla *tla, unsigned int enable, unsigned int flags);

Where (g)libc would unconditionally set up the structure with
.enabled=0, .flags=0, and anybody actually wanting to make use of the
thing do:

	thread_local_abi_register(NULL, TLA_ENABLE_CPU, 0);

Obviously calling register with !NULL address twice will error (you
already registered), calling with NULL before !NULL will also error.

And if you really worry about running out of feature bits, we could of
course pass it in a mask, but I'm not sure I can see 30 other features
we would want to cram into this (yes, yes, famous last words etc.. 640kb
anyone?).