linux-kernel - Re: [PATCH v5] futex: Remove requirement for lock_page in get_futex

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, 5 Feb 2016 10:44:42 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Davidlohr Bueso <dave@...olabs.net>,
	Mel Gorman <mgorman@...hsingularity.net>,
	Peter Zijlstra <peterz@...radead.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Chris Mason <clm@...com>, Darren Hart <dvhart@...ux.intel.com>,
	Hugh Dickins <hughd@...gle.com>, linux-kernel@...r.kernel.org,
	Davidlohr Bueso <dbueso@...e.de>, Mel Gorman <mgorman@...e.de>
Subject: Re: [PATCH v5] futex: Remove requirement for lock_page in
 get_futex_key


* Thomas Gleixner <tglx@...utronix.de> wrote:

> On Thu, 4 Feb 2016, Davidlohr Bueso wrote:
> 
> > On Thu, 04 Feb 2016, Thomas Gleixner wrote:
> > 
> > > On Wed, 3 Feb 2016, Davidlohr Bueso wrote:
> > > > +		 * We are not calling into get_futex_key_refs() in file-backed
> > > > +		 * cases, therefore a successful atomic_inc return below will
> > > > +		 * guarantee that get_futex_key() will continue to imply MB
> > > > (B).
> > > 
> > > Can you please make that "MB (B)" part a bit more outstanding. I really had
> > > to
> > > search for it.
> > 
> > Hmm as you know this is mostly explained at the begining of the file, and we
> > sprinkle MB (B) around the code based on that description. So I'm a bit
> > confused
> > as to why you don't like like that comment.
> 
> The other "MB (B)" places are more outstanding. It did not spring in my eye 
> immideately. So it's a pure cosmetic issue.

So I too didn't understand that sentence at first, because the capitalization 
really throws off quick parsing of that comment, as 'MB' ususally denotes 
megabytes.

So please change it to "mb(); (A)" or so - and I think all of these comments 
should be changed to use a standard API name for the barrier they imply, as the 
head of futex.c does:

 *   waiters++; (a)
 *   mb(); (A) <-- paired with -.
 *                              |
 *   lock(hash_bucket(futex));  |
 *                              |
 *   uval = *futex;             |
 *                              |        *futex = newval;
 *                              |        sys_futex(WAKE, futex);
 *                              |          futex_wake(futex);
 *                              |
 *                              `------->  mb(); (B)

Btw., pedantic: shouldn't that be smp_mb()? Futexes don't operate on IO spaces, so 
on UP they only need compiler barriers.

Thanks,

	Ingo