linux-kernel - [PATCH v3] futex: replace bare barrier() with more lightweight READ

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:	Mon,  7 Mar 2016 09:32:24 +0800
From:	Jianyu Zhan <nasa4836@...il.com>
To:	linux-kernel@...r.kernel.org, peterz@...radead.org,
	tglx@...utronix.de, dave@...olabs.net, akpm@...ux-foundation.org,
	mingo@...nel.org, linux@...musvillemoes.dk, dvhart@...ux.intel.com,
	borntraeger@...ibm.com, fengguang.wu@...el.com,
	bigeasy@...utronix.de
Cc:	nasa4836@...il.com
Subject: [PATCH v3] futex: replace bare barrier() with more lightweight READ_ONCE()

Commit e91467ecd1ef ("bug in futex unqueue_me") introduces a barrier()
in unqueue_me(), to address below problem.

The scenario is like this:

====================
original code:

retry:
       lock_ptr = q->lock_ptr;
       if (lock_ptr != 0)  {
                  spin_lock(lock_ptr)
                  if (unlikely(lock_ptr != q->lock_ptr)) {
                        spin_unlock(lock_ptr);
                         goto retry;
                  }
                   ...
       }

====================
It was observed that compiler generates code that is equivalent to:

retry:
       if (q->lock_ptr != 0)  {
                  spin_lock(q->lock_ptr)
                  if (unlikely(lock_ptr != q->lock_ptr)) {
                        spin_unlock(lock_ptr);
                         goto retry;
                  }
                   ...
       }

since q->lock_ptr might change between the test of non-nullness and spin_lock(),
the double load will cause trouble. So that commit uses a barrier() to prevent this.

This patch replaces this bare barrier() with a READ_ONCE().

The reasons are:

1) READ_ONCE() is a more weak form of barrier() that affect only the specific
   accesses, while barrier() is a more general compiler level memroy barrier.
   READ_ONCE() was not available at that time when that patch was written.

2) READ_ONCE() which could be more informative by its name, while a bare barrier()
   without comment leads to quite a bit of perplexity.

Assembly code before(barrier version) and after this patch(READ_ONCE version) are the same:

====================
Before(barrier version):

unqueue_me():
linux/kernel/futex.c:1930
    1df6:       4c 8b bd 28 ff ff ff    mov    -0xd8(%rbp),%r15
linux/kernel/futex.c:1932
    1dfd:       4d 85 ff                test   %r15,%r15
    1e00:       0f 84 5c 01 00 00       je     1f62 <futex_wait+0x292>
spin_lock():
linux/include/linux/spinlock.h:302
    1e06:       4c 89 ff                mov    %r15,%rdi
    1e09:       e8 00 00 00 00          callq  1e0e <futex_wait+0x13e>

====================
After(READ_ONCE version):

__read_once_size():
linux/include/linux/compiler.h:218
    1df6:       4c 8b bd 28 ff ff ff    mov    -0xd8(%rbp),%r15
unqueue_me():
linux/kernel/futex.c:1935
    1dfd:       4d 85 ff                test   %r15,%r15
    1e00:       0f 84 5c 01 00 00       je     1f62 <futex_wait+0x292>
spin_lock():
linux/include/linux/spinlock.h:302
    1e06:       4c 89 ff                mov    %r15,%rdi
    1e09:       e8 00 00 00 00          callq  1e0e <futex_wait+0x13e>

Code size is also the same.

Many thanks to Darren Hart <dvhart@...radead.org> for reviewing and
suggestion. 

Acked-by: Christian Borntraeger <borntraeger@...ibm.com>
Signed-off-by: Jianyu Zhan <nasa4836@...il.com>
---
 kernel/futex.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 5d6ce64..25dbfed 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1927,8 +1927,12 @@ static int unqueue_me(struct futex_q *q)
 
 	/* In the common case we don't take the spinlock, which is nice. */
 retry:
-	lock_ptr = q->lock_ptr;
-	barrier();
+	/*
+	 * q->lock_ptr can change between this read and the following spin_lock.
+	 * Use READ_ONCE to forbid the compiler from reloading q->lock_ptr and
+	 * optimizing lock_ptr out of the logic below.
+	 */
+	lock_ptr = READ_ONCE(q->lock_ptr);
 	if (lock_ptr != NULL) {
 		spin_lock(lock_ptr);
 		/*
-- 
2.4.3