Missing READ_ONCE in core and arch-specific pgtable code leading to crashes

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Also, a very long standing item on my TODO list is to see how much of it we can unify across the various architectures, because there's a giant amount of boiler plate involved with all this.

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

As always, if anyone is, we must continue to support Alpha, but sounds like time to check again.

Quoted 23 lines · Paul E. McKenney

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Quoted 4 lines

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

As always, if anyone is, we must continue to support Alpha, but sounds like time to check again.

I'll be honest and say that I haven't updated mine for a while, but I do have a soft spot for those machines :(

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

Quoted 23 lines · Paul E. McKenney

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Quoted 1 line

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

Yes. I run two Alpha build daemons that build the unofficial debian-alpha port. Debian popcon reports nine machines running Alpha, which are likely to be running the 4.12.y kernel which is currently in debian-alpha, (and presumably soon to be 4.13.y which is now built on Alpha in experimental).

Quoted 26 lines · Michael Cree

Quoted 24 lines · Paul E. McKenney

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

Quoted 5 lines

Yes. I run two Alpha build daemons that build the unofficial debian-alpha port. Debian popcon reports nine machines running Alpha, which are likely to be running the 4.12.y kernel which is currently in debian-alpha, (and presumably soon to be 4.13.y which is now built on Alpha in experimental).

I salute your dedication to Alpha! ;-)

Quoted 27 lines · Paul E. McKenney

Quoted 26 lines · Michael Cree

Quoted 24 lines · Paul E. McKenney

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

Quoted 7 lines

Quoted 5 lines

Yes. I run two Alpha build daemons that build the unofficial debian-alpha port. Debian popcon reports nine machines running Alpha, which are likely to be running the 4.12.y kernel which is currently in debian-alpha, (and presumably soon to be 4.13.y which is now built on Alpha in experimental).

I salute your dedication to Alpha! ;-)

Ok, but where does that leave us wrt my initial proposal of moving smp_read_barrier_depends() into READ_ONCE and getting rid of lockless_dereference?

Michael (or anybody else running mainline on SMP Alpha) -- would you be able to give the diff below a spin and see whether there's a measurable performance impact?

Cheers,

Will

--->8

include/linux/compiler.h +1 -0

index e95a2631e545..0ce21e25492a 100644
@@ -340,6 +340,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
+	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
 	__u.__val;							\
 })
 #define READ_ONCE(x) __READ_ONCE(x, 1)

Quoted 40 lines · Will Deacon

Quoted 27 lines · Paul E. McKenney

Quoted 26 lines · Michael Cree

Quoted 24 lines · Paul E. McKenney

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

Quoted 7 lines

Quoted 5 lines

Yes. I run two Alpha build daemons that build the unofficial debian-alpha port. Debian popcon reports nine machines running Alpha, which are likely to be running the 4.12.y kernel which is currently in debian-alpha, (and presumably soon to be 4.13.y which is now built on Alpha in experimental).

I salute your dedication to Alpha! ;-)

Ok, but where does that leave us wrt my initial proposal of moving smp_read_barrier_depends() into READ_ONCE and getting rid of lockless_dereference?

Quoted 3 lines

Michael (or anybody else running mainline on SMP Alpha) -- would you be able to give the diff below a spin and see whether there's a measurable performance impact?

This will be a sensitive test. The smp_read_barrier_depends() can be removed from lockless_dereference(). Without this removal Alpha will get two memory barriers from rcu_dereference() and friends.

Quoted 45 lines · Paul E. McKenney

Quoted 43 lines · Will Deacon

Quoted 27 lines · Paul E. McKenney

Quoted 26 lines · Michael Cree

Quoted 24 lines · Paul E. McKenney

Quoted 13 lines · Will Deacon

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Will Deacon

In many cases, page tables can be accessed concurrently by either another CPU (due to things like fast gup) or by the hardware page table walker itself, which may set access/dirty bits. In such cases, it is important to use READ_ONCE/WRITE_ONCE when accessing page table entries so that entries cannot be torn, merged or subject to apparent loss of coherence.

In fact, we should use lockless_dereference() for many of them. Yes Alpha is the only one that cares about the difference between that and READ_ONCE() and they do have the extra barrier, but if we're going to do this, we might as well do it 'right' :-)

Quoted 6 lines

I know this sounds daft, but I think one of the big reasons why lockless_dereference() doesn't get an awful lot of use is because it's such a mouthful! Why don't we just move the smp_read_barrier_depends() into READ_ONCE? Would anybody actually care about the potential impact on Alpha (which, frankly, is treading on thin ice given the low adoption of lockless_dereference())?

This is my cue to ask my usual question... ;-)

Are people still running mainline kernels on Alpha? (Added Alpha folks.)

Quoted 7 lines

Quoted 5 lines

Yes. I run two Alpha build daemons that build the unofficial debian-alpha port. Debian popcon reports nine machines running Alpha, which are likely to be running the 4.12.y kernel which is currently in debian-alpha, (and presumably soon to be 4.13.y which is now built on Alpha in experimental).

I salute your dedication to Alpha! ;-)

Ok, but where does that leave us wrt my initial proposal of moving smp_read_barrier_depends() into READ_ONCE and getting rid of lockless_dereference?

Michael (or anybody else running mainline on SMP Alpha) -- would you be able to give the diff below a spin and see whether there's a measurable performance impact?

Quoted 3 lines

This will be a sensitive test. The smp_read_barrier_depends() can be removed from lockless_dereference(). Without this removal Alpha will get two memory barriers from rcu_dereference() and friends.

Oh yes, good point. I was trying to keep the diff simple, but you're right that this is packing too many barriers. Fixed diff below.

Thanks,

Will

--->8

include/linux/compiler.h +1 -1

index e95a2631e545..c4ee9d6d8f2d 100644
@@ -340,6 +340,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
+	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
 	__u.__val;							\
 })
 #define READ_ONCE(x) __READ_ONCE(x, 1)

@@ -620,7 +621,6 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 ({ \
 	typeof(p) _________p1 = READ_ONCE(p); \
 	typeof(*(p)) *___typecheck_p __maybe_unused; \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
 	(_________p1); \
 })
 

Quoted 14 lines · Will Deacon

Quoted 12 lines · Paul E. McKenney

Quoted 7 lines · Will Deacon

Ok, but where does that leave us wrt my initial proposal of moving smp_read_barrier_depends() into READ_ONCE and getting rid of lockless_dereference?

Michael (or anybody else running mainline on SMP Alpha) -- would you be able to give the diff below a spin and see whether there's a measurable performance impact?

This will be a sensitive test. The smp_read_barrier_depends() can be removed from lockless_dereference(). Without this removal Alpha will get two memory barriers from rcu_dereference() and friends.

Quoted 2 lines

Oh yes, good point. I was trying to keep the diff simple, but you're right that this is packing too many barriers. Fixed diff below.

Not seeing any objections thus far. If there are none by (say) the end of this week, I would be happy to queue a patch for the 4.16 merge window. That should give ample opportunity for further review and testing.

Hi Paul,

Quoted 18 lines · Paul E. McKenney

Quoted 16 lines · Will Deacon

Quoted 5 lines · Paul E. McKenney

Quoted 4 lines · Will Deacon

Ok, but where does that leave us wrt my initial proposal of moving smp_read_barrier_depends() into READ_ONCE and getting rid of lockless_dereference?

Quoted 7 lines

Quoted 3 lines

Michael (or anybody else running mainline on SMP Alpha) -- would you be able to give the diff below a spin and see whether there's a measurable performance impact?

This will be a sensitive test. The smp_read_barrier_depends() can be removed from lockless_dereference(). Without this removal Alpha will get two memory barriers from rcu_dereference() and friends.

Oh yes, good point. I was trying to keep the diff simple, but you're right that this is packing too many barriers. Fixed diff below.

Quoted 4 lines

Not seeing any objections thus far. If there are none by (say) the end of this week, I would be happy to queue a patch for the 4.16 merge window. That should give ample opportunity for further review and testing.

Ok, full patch below.

Hi Paul,

Quoted 15 lines · Paul E. McKenney

Quoted 14 lines · Will Deacon

Quoted 12 lines · Paul E. McKenney

Quoted 7 lines · Will Deacon

Ok, but where does that leave us wrt my initial proposal of moving smp_read_barrier_depends() into READ_ONCE and getting rid of lockless_dereference?

Michael (or anybody else running mainline on SMP Alpha) -- would you be able to give the diff below a spin and see whether there's a measurable performance impact?

This will be a sensitive test. The smp_read_barrier_depends() can be removed from lockless_dereference(). Without this removal Alpha will get two memory barriers from rcu_dereference() and friends.

Quoted 7 lines

Quoted 2 lines

Oh yes, good point. I was trying to keep the diff simple, but you're right that this is packing too many barriers. Fixed diff below.

Not seeing any objections thus far. If there are none by (say) the end of this week, I would be happy to queue a patch for the 4.16 merge window. That should give ample opportunity for further review and testing.

Ok, full patch below.

Will

--->8

From 15956d0cc6b37208d8542b1858a8d8b64227acf4 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Thu, 5 Oct 2017 16:57:36 +0100
Subject: [PATCH] locking/barriers: Kill lockless_dereference

lockless_dereference is a nice idea, but it's gained little traction in kernel code since it's introduction three years ago. This is partly because it's a pain to type, but also because using READ_ONCE instead will work correctly on all architectures apart from Alpha, which is a fully supported but somewhat niche architecture these days.

This patch moves smp_read_barrier_depends() (a NOP on all architectures other than Alpha) from lockless_dereference into READ_ONCE, converts the few actual users over to READ_ONCE and then finally removes lockless_dereference altogether.

Signed-off-by Will Deacon <[email protected]>

---
 Documentation/memory-barriers.txt                   | 12 ------------
 .../translations/ko_KR/memory-barriers.txt          | 12 ------------
 arch/x86/events/core.c                              |  2 +-
 arch/x86/include/asm/mmu_context.h                  |  4 ++--
 arch/x86/kernel/ldt.c                               |  2 +-
 drivers/md/dm-mpath.c                               | 20 ++++++++++----------
 fs/dcache.c                                         |  4 ++--
 fs/overlayfs/ovl_entry.h                            |  2 +-
 fs/overlayfs/readdir.c                              |  2 +-
 include/linux/compiler.h                            | 21 +--------------------
 include/linux/rculist.h                             |  4 ++--
 include/linux/rcupdate.h                            |  4 ++--
 kernel/events/core.c                                |  4 ++--
 kernel/seccomp.c                                    |  2 +-
 kernel/task_work.c                                  |  2 +-
 mm/slab.h                                           |  2 +-
 16 files changed, 28 insertions(+), 71 deletions(-)

Documentation/memory-barriers.txt +0 -12

index b759a60624fd..470a682f3fa4 100644
@@ -1886,18 +1886,6 @@ There are some more advanced barrier functions:
      See Documentation/atomic_{t,bitops}.txt for more information.
 
 
- (*) lockless_dereference();
-
-     This can be thought of as a pointer-fetch wrapper around the
-     smp_read_barrier_depends() data-dependency barrier.
-
-     This is also similar to rcu_dereference(), but in cases where
-     object lifetime is handled by some mechanism other than RCU, for
-     example, when the objects removed only when the system goes down.
-     In addition, lockless_dereference() is used in some data structures
-     that can be used both with and without RCU.
-
-
  (*) dma_wmb();
  (*) dma_rmb();
 

Documentation/translations/ko_KR/memory-barriers.txt +0 -12

index a7a813258013..ec3b46e27b7a 100644
@@ -1858,18 +1858,6 @@ Mandatory 배리어들은 SMP 시스템에서도 UP 시스템에서도 SMP 효
      참고하세요.
 
 
- (*) lockless_dereference();
-
-     이 함수는 smp_read_barrier_depends() 데이터 의존성 배리어를 사용하는
-     포인터 읽어오기 래퍼(wrapper) 함수로 생각될 수 있습니다.
-
-     객체의 라이프타임이 RCU 외의 메커니즘으로 관리된다는 점을 제외하면
-     rcu_dereference() 와도 유사한데, 예를 들면 객체가 시스템이 꺼질 때에만
-     제거되는 경우 등입니다.  또한, lockless_dereference() 은 RCU 와 함께
-     사용될수도, RCU 없이 사용될 수도 있는 일부 데이터 구조에 사용되고
-     있습니다.
-
-
  (*) dma_wmb();
  (*) dma_rmb();
 

arch/x86/events/core.c +1 -1

index 80534d3c2480..589af1eec7c1 100644
@@ -2371,7 +2371,7 @@ static unsigned long get_segment_base(unsigned int segment)
 		struct ldt_struct *ldt;
 
 		/* IRQs are off, so this synchronizes with smp_store_release */
-		ldt = lockless_dereference(current->active_mm->context.ldt);
+		ldt = READ_ONCE(current->active_mm->context.ldt);
 		if (!ldt || idx >= ldt->nr_entries)
 			return 0;
 

arch/x86/include/asm/mmu_context.h +2 -2

index c120b5db178a..9037a4e546e8 100644
@@ -72,8 +72,8 @@ static inline void load_mm_ldt(struct mm_struct *mm)
 #ifdef CONFIG_MODIFY_LDT_SYSCALL
 	struct ldt_struct *ldt;
 
-	/* lockless_dereference synchronizes with smp_store_release */
-	ldt = lockless_dereference(mm->context.ldt);
+	/* READ_ONCE synchronizes with smp_store_release */
+	ldt = READ_ONCE(mm->context.ldt);
 
 	/*
 	 * Any change to mm->context.ldt is followed by an IPI to all

arch/x86/kernel/ldt.c +1 -1

index f0e64db18ac8..0a21390642c4 100644
@@ -101,7 +101,7 @@ static void finalize_ldt_struct(struct ldt_struct *ldt)
 static void install_ldt(struct mm_struct *current_mm,
 			struct ldt_struct *ldt)
 {
-	/* Synchronizes with lockless_dereference in load_mm_ldt. */
+	/* Synchronizes with READ_ONCE in load_mm_ldt. */
 	smp_store_release(&current_mm->context.ldt, ldt);
 
 	/* Activate the LDT for all CPUs using current_mm. */

drivers/md/dm-mpath.c +10 -10

index 11f273d2f018..3f88c9d32f7e 100644
@@ -366,7 +366,7 @@ static struct pgpath *choose_path_in_pg(struct multipath *m,
 
 	pgpath = path_to_pgpath(path);
 
-	if (unlikely(lockless_dereference(m->current_pg) != pg)) {
+	if (unlikely(READ_ONCE(m->current_pg) != pg)) {
 		/* Only update current_pgpath if pg changed */
 		spin_lock_irqsave(&m->lock, flags);
 		m->current_pgpath = pgpath;

@@ -390,7 +390,7 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes)
 	}
 
 	/* Were we instructed to switch PG? */
-	if (lockless_dereference(m->next_pg)) {
+	if (READ_ONCE(m->next_pg)) {
 		spin_lock_irqsave(&m->lock, flags);
 		pg = m->next_pg;
 		if (!pg) {

@@ -406,7 +406,7 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes)
 
 	/* Don't change PG until it has no remaining paths */
 check_current_pg:
-	pg = lockless_dereference(m->current_pg);
+	pg = READ_ONCE(m->current_pg);
 	if (pg) {
 		pgpath = choose_path_in_pg(m, pg, nr_bytes);
 		if (!IS_ERR_OR_NULL(pgpath))

@@ -473,7 +473,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	struct request *clone;
 
 	/* Do we need to select a new pgpath? */
-	pgpath = lockless_dereference(m->current_pgpath);
+	pgpath = READ_ONCE(m->current_pgpath);
 	if (!pgpath || !test_bit(MPATHF_QUEUE_IO, &m->flags))
 		pgpath = choose_pgpath(m, nr_bytes);
 

@@ -535,7 +535,7 @@ static int __multipath_map_bio(struct multipath *m, struct bio *bio, struct dm_m
 	bool queue_io;
 
 	/* Do we need to select a new pgpath? */
-	pgpath = lockless_dereference(m->current_pgpath);
+	pgpath = READ_ONCE(m->current_pgpath);
 	queue_io = test_bit(MPATHF_QUEUE_IO, &m->flags);
 	if (!pgpath || !queue_io)
 		pgpath = choose_pgpath(m, nr_bytes);

@@ -1804,7 +1804,7 @@ static int multipath_prepare_ioctl(struct dm_target *ti,
 	struct pgpath *current_pgpath;
 	int r;
 
-	current_pgpath = lockless_dereference(m->current_pgpath);
+	current_pgpath = READ_ONCE(m->current_pgpath);
 	if (!current_pgpath)
 		current_pgpath = choose_pgpath(m, 0);
 

@@ -1826,7 +1826,7 @@ static int multipath_prepare_ioctl(struct dm_target *ti,
 	}
 
 	if (r == -ENOTCONN) {
-		if (!lockless_dereference(m->current_pg)) {
+		if (!READ_ONCE(m->current_pg)) {
 			/* Path status changed, redo selection */
 			(void) choose_pgpath(m, 0);
 		}

@@ -1895,9 +1895,9 @@ static int multipath_busy(struct dm_target *ti)
 		return (m->queue_mode != DM_TYPE_MQ_REQUEST_BASED);
 
 	/* Guess which priority_group will be used at next mapping time */
-	pg = lockless_dereference(m->current_pg);
-	next_pg = lockless_dereference(m->next_pg);
-	if (unlikely(!lockless_dereference(m->current_pgpath) && next_pg))
+	pg = READ_ONCE(m->current_pg);
+	next_pg = READ_ONCE(m->next_pg);
+	if (unlikely(!READ_ONCE(m->current_pgpath) && next_pg))
 		pg = next_pg;
 
 	if (!pg) {

fs/dcache.c +2 -2

index f90141387f01..34c852af215c 100644
@@ -231,7 +231,7 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
 {
 	/*
 	 * Be careful about RCU walk racing with rename:
-	 * use 'lockless_dereference' to fetch the name pointer.
+	 * use 'READ_ONCE' to fetch the name pointer.
 	 *
 	 * NOTE! Even if a rename will mean that the length
 	 * was not loaded atomically, we don't care. The

@@ -245,7 +245,7 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
 	 * early because the data cannot match (there can
 	 * be no NUL in the ct/tcount data)
 	 */
-	const unsigned char *cs = lockless_dereference(dentry->d_name.name);
+	const unsigned char *cs = READ_ONCE(dentry->d_name.name);
 
 	return dentry_string_cmp(cs, ct, tcount);
 }

fs/overlayfs/ovl_entry.h +1 -1

index 878a750986dd..0f6809fa6628 100644
@@ -74,5 +74,5 @@ static inline struct ovl_inode *OVL_I(struct inode *inode)
 
 static inline struct dentry *ovl_upperdentry_dereference(struct ovl_inode *oi)
 {
-	return lockless_dereference(oi->__upperdentry);
+	return READ_ONCE(oi->__upperdentry);
 }

fs/overlayfs/readdir.c +1 -1

index 62e9b22a2077..0b389d330613 100644
@@ -754,7 +754,7 @@ static int ovl_dir_fsync(struct file *file, loff_t start, loff_t end,
 	if (!od->is_upper && OVL_TYPE_UPPER(ovl_path_type(dentry))) {
 		struct inode *inode = file_inode(file);
 
-		realfile = lockless_dereference(od->upperfile);
+		realfile = READ_ONCE(od->upperfile);
 		if (!realfile) {
 			struct path upperpath;
 

include/linux/compiler.h +1 -20

index e95a2631e545..f260ff39f90f 100644
@@ -340,6 +340,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
+	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
 	__u.__val;							\
 })
 #define READ_ONCE(x) __READ_ONCE(x, 1)

@@ -604,24 +605,4 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 	(volatile typeof(x) *)&(x); })
 #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
 
-/**
- * lockless_dereference() - safely load a pointer for later dereference
- * @p: The pointer to load
- *
- * Similar to rcu_dereference(), but for situations where the pointed-to
- * object's lifetime is managed by something other than RCU.  That
- * "something other" might be reference counting or simple immortality.
- *
- * The seemingly unused variable ___typecheck_p validates that @p is
- * indeed a pointer type by using a pointer to typeof(*p) as the type.
- * Taking a pointer to typeof(*p) again is needed in case p is void *.
- */
-#define lockless_dereference(p) \
-({ \
-	typeof(p) _________p1 = READ_ONCE(p); \
-	typeof(*(p)) *___typecheck_p __maybe_unused; \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
-	(_________p1); \
-})
-
 #endif /* __LINUX_COMPILER_H */

include/linux/rculist.h +2 -2

index b1fd8bf85fdc..3a2bb7d8ed4d 100644
@@ -274,7 +274,7 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
  */
 #define list_entry_rcu(ptr, type, member) \
-	container_of(lockless_dereference(ptr), type, member)
+	container_of(READ_ONCE(ptr), type, member)
 
 /**
  * Where are list_empty_rcu() and list_first_entry_rcu()?

@@ -367,7 +367,7 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * example is when items are added to the list, but never deleted.
  */
 #define list_entry_lockless(ptr, type, member) \
-	container_of((typeof(ptr))lockless_dereference(ptr), type, member)
+	container_of((typeof(ptr))READ_ONCE(ptr), type, member)
 
 /**
  * list_for_each_entry_lockless - iterate over rcu list of given type

include/linux/rcupdate.h +2 -2

index de50d8a4cf41..380a3aeb09d7 100644
@@ -346,7 +346,7 @@ static inline void rcu_preempt_sleep_check(void) { }
 #define __rcu_dereference_check(p, c, space) \
 ({ \
 	/* Dependency order vs. p above. */ \
-	typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
+	typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); \
 	RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
 	rcu_dereference_sparse(p, space); \
 	((typeof(*p) __force __kernel *)(________p1)); \

@@ -360,7 +360,7 @@ static inline void rcu_preempt_sleep_check(void) { }
 #define rcu_dereference_raw(p) \
 ({ \
 	/* Dependency order vs. p above. */ \
-	typeof(p) ________p1 = lockless_dereference(p); \
+	typeof(p) ________p1 = READ_ONCE(p); \
 	((typeof(*p) __force __kernel *)(________p1)); \
 })
 

kernel/events/core.c +2 -2

index 6bc21e202ae4..417812ce0099 100644
@@ -4231,7 +4231,7 @@ static void perf_remove_from_owner(struct perf_event *event)
 	 * indeed free this event, otherwise we need to serialize on
 	 * owner->perf_event_mutex.
 	 */
-	owner = lockless_dereference(event->owner);
+	owner = READ_ONCE(event->owner);
 	if (owner) {
 		/*
 		 * Since delayed_put_task_struct() also drops the last

@@ -4328,7 +4328,7 @@ int perf_event_release_kernel(struct perf_event *event)
 		 * Cannot change, child events are not migrated, see the
 		 * comment with perf_event_ctx_lock_nested().
 		 */
-		ctx = lockless_dereference(child->ctx);
+		ctx = READ_ONCE(child->ctx);
 		/*
 		 * Since child_mutex nests inside ctx::mutex, we must jump
 		 * through hoops. We start by grabbing a reference on the ctx.

kernel/seccomp.c +1 -1

index bb3a38005b9c..1daa8b61a268 100644
@@ -189,7 +189,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
 	u32 ret = SECCOMP_RET_ALLOW;
 	/* Make sure cross-thread synced filter points somewhere sane. */
 	struct seccomp_filter *f =
-			lockless_dereference(current->seccomp.filter);
+			READ_ONCE(current->seccomp.filter);
 
 	/* Ensure unexpected behavior doesn't result in failing open. */
 	if (unlikely(WARN_ON(f == NULL)))

kernel/task_work.c +1 -1

index 836a72a66fba..9a9f262fc53d 100644
@@ -67,7 +67,7 @@ task_work_cancel(struct task_struct *task, task_work_func_t func)
 	 * we raced with task_work_run(), *pprev == NULL/exited.
 	 */
 	raw_spin_lock_irqsave(&task->pi_lock, flags);
-	while ((work = lockless_dereference(*pprev))) {
+	while ((work = READ_ONCE(*pprev))) {
 		if (work->func != func)
 			pprev = &work->next;
 		else if (cmpxchg(pprev, work, work->next) == work)

mm/slab.h +1 -1

index 073362816acc..8894f811a89d 100644
@@ -258,7 +258,7 @@ cache_from_memcg_idx(struct kmem_cache *s, int idx)
 	 * memcg_caches issues a write barrier to match this (see
 	 * memcg_create_kmem_cache()).
 	 */
-	cachep = lockless_dereference(arr->entries[idx]);
+	cachep = READ_ONCE(arr->entries[idx]);
 	rcu_read_unlock();
 
 	return cachep;

Signature · 1 line

2.1.4

Hi Will,

none of my comments below represent objections to this patch, but let me remark:

Quoted 37 lines · Will Deacon

Hi Paul,

Quoted 15 lines · Paul E. McKenney

Quoted 14 lines · Will Deacon

Quoted 12 lines · Paul E. McKenney

Quoted 7 lines · Will Deacon

Ok, but where does that leave us wrt my initial proposal of moving smp_read_barrier_depends() into READ_ONCE and getting rid of lockless_dereference?

Michael (or anybody else running mainline on SMP Alpha) -- would you be able to give the diff below a spin and see whether there's a measurable performance impact?

This will be a sensitive test. The smp_read_barrier_depends() can be removed from lockless_dereference(). Without this removal Alpha will get two memory barriers from rcu_dereference() and friends.

Quoted 7 lines

Quoted 2 lines

Oh yes, good point. I was trying to keep the diff simple, but you're right that this is packing too many barriers. Fixed diff below.

Not seeing any objections thus far. If there are none by (say) the end of this week, I would be happy to queue a patch for the 4.16 merge window. That should give ample opportunity for further review and testing.

Ok, full patch below.

Will

--->8

From 15956d0cc6b37208d8542b1858a8d8b64227acf4 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Thu, 5 Oct 2017 16:57:36 +0100
Subject: [PATCH] locking/barriers: Kill lockless_dereference

Quoted 5 lines

lockless_dereference is a nice idea, but it's gained little traction in kernel code since it's introduction three years ago. This is partly because it's a pain to type, but also because using READ_ONCE instead will work correctly on all architectures apart from Alpha, which is a fully supported but somewhat niche architecture these days.

lockless_dereference might be a mouthful, but it does (explicitly) say/remark: "Yep, we are relying on the following address dep. to be "in strong-ppo" ".

Such information will be lost or, at least, not immediately clear by just reading a READ_ONCE(). (And Yes, this information is only relevant when we "include" Alpha in the picture/analysis.)

Quoted 4 lines

This patch moves smp_read_barrier_depends() (a NOP on all architectures other than Alpha) from lockless_dereference into READ_ONCE, converts the few actual users over to READ_ONCE and then finally removes lockless_dereference altogether.

Notice that several "potential users" of lockless_dereference are currently hidden in other call sites for smp_read_barrier_depends (i.e., cases where this barrier is not called from within a lock- less or an RCU dereference).

Some of these usages (e.g.,

include/linux/percpu-refcount.h:__ref_is_percpu, mm/ksm.c:get_ksm_page, security/keys/keyring.c:search_nested_keyrings )

precedes this barrier with a READ_ONCE; others (e.g.,

arch/alpha/include/asm/pgtable.h:pmd_offset, net/ipv4/netfilter/arp_tables.c:arpt_do_table kernel/kernel/events/uprobes.c:get_trampiline_vaddr )

with a plain read.

There also appear to be cases where the barrier is preceded by an ACCESS_ONCE (c.f, fs/dcache.c:prepend_name) or by an xchg_release (c.f., kernel/locking/qspinlock.c:queued_spin_lock_slowpath), and it would not be difficult to imagine/create different usages.

Quoted 1 line

Signed-off-by Will Deacon <[email protected]>

I understand that we all agree we're missing a Tested-by here ;-).

Andrea

Quoted 377 lines

---
 Documentation/memory-barriers.txt                   | 12 ------------
 .../translations/ko_KR/memory-barriers.txt          | 12 ------------
 arch/x86/events/core.c                              |  2 +-
 arch/x86/include/asm/mmu_context.h                  |  4 ++--
 arch/x86/kernel/ldt.c                               |  2 +-
 drivers/md/dm-mpath.c                               | 20 ++++++++++----------
 fs/dcache.c                                         |  4 ++--
 fs/overlayfs/ovl_entry.h                            |  2 +-
 fs/overlayfs/readdir.c                              |  2 +-
 include/linux/compiler.h                            | 21 +--------------------
 include/linux/rculist.h                             |  4 ++--
 include/linux/rcupdate.h                            |  4 ++--
 kernel/events/core.c                                |  4 ++--
 kernel/seccomp.c                                    |  2 +-
 kernel/task_work.c                                  |  2 +-
 mm/slab.h                                           |  2 +-
 16 files changed, 28 insertions(+), 71 deletions(-)

Documentation/memory-barriers.txt +0 -12

index b759a60624fd..470a682f3fa4 100644
@@ -1886,18 +1886,6 @@ There are some more advanced barrier functions:
      See Documentation/atomic_{t,bitops}.txt for more information.
 
 
- (*) lockless_dereference();
-
-     This can be thought of as a pointer-fetch wrapper around the
-     smp_read_barrier_depends() data-dependency barrier.
-
-     This is also similar to rcu_dereference(), but in cases where
-     object lifetime is handled by some mechanism other than RCU, for
-     example, when the objects removed only when the system goes down.
-     In addition, lockless_dereference() is used in some data structures
-     that can be used both with and without RCU.
-
-
  (*) dma_wmb();
  (*) dma_rmb();
 

Documentation/translations/ko_KR/memory-barriers.txt +0 -12

index a7a813258013..ec3b46e27b7a 100644
@@ -1858,18 +1858,6 @@ Mandatory 배리어들은 SMP 시스템에서도 UP 시스템에서도 SMP 효
      참고하세요.
 
 
- (*) lockless_dereference();
-
-     이 함수는 smp_read_barrier_depends() 데이터 의존성 배리어를 사용하는
-     포인터 읽어오기 래퍼(wrapper) 함수로 생각될 수 있습니다.
-
-     객체의 라이프타임이 RCU 외의 메커니즘으로 관리된다는 점을 제외하면
-     rcu_dereference() 와도 유사한데, 예를 들면 객체가 시스템이 꺼질 때에만
-     제거되는 경우 등입니다.  또한, lockless_dereference() 은 RCU 와 함께
-     사용될수도, RCU 없이 사용될 수도 있는 일부 데이터 구조에 사용되고
-     있습니다.
-
-
  (*) dma_wmb();
  (*) dma_rmb();
 

arch/x86/events/core.c +1 -1

index 80534d3c2480..589af1eec7c1 100644
@@ -2371,7 +2371,7 @@ static unsigned long get_segment_base(unsigned int segment)
 		struct ldt_struct *ldt;
 
 		/* IRQs are off, so this synchronizes with smp_store_release */
-		ldt = lockless_dereference(current->active_mm->context.ldt);
+		ldt = READ_ONCE(current->active_mm->context.ldt);
 		if (!ldt || idx >= ldt->nr_entries)
 			return 0;
 

arch/x86/include/asm/mmu_context.h +2 -2

index c120b5db178a..9037a4e546e8 100644
@@ -72,8 +72,8 @@ static inline void load_mm_ldt(struct mm_struct *mm)
 #ifdef CONFIG_MODIFY_LDT_SYSCALL
 	struct ldt_struct *ldt;
 
-	/* lockless_dereference synchronizes with smp_store_release */
-	ldt = lockless_dereference(mm->context.ldt);
+	/* READ_ONCE synchronizes with smp_store_release */
+	ldt = READ_ONCE(mm->context.ldt);
 
 	/*
 	 * Any change to mm->context.ldt is followed by an IPI to all

arch/x86/kernel/ldt.c +1 -1

index f0e64db18ac8..0a21390642c4 100644
@@ -101,7 +101,7 @@ static void finalize_ldt_struct(struct ldt_struct *ldt)
 static void install_ldt(struct mm_struct *current_mm,
 			struct ldt_struct *ldt)
 {
-	/* Synchronizes with lockless_dereference in load_mm_ldt. */
+	/* Synchronizes with READ_ONCE in load_mm_ldt. */
 	smp_store_release(&current_mm->context.ldt, ldt);
 
 	/* Activate the LDT for all CPUs using current_mm. */

drivers/md/dm-mpath.c +10 -10

index 11f273d2f018..3f88c9d32f7e 100644
@@ -366,7 +366,7 @@ static struct pgpath *choose_path_in_pg(struct multipath *m,
 
 	pgpath = path_to_pgpath(path);
 
-	if (unlikely(lockless_dereference(m->current_pg) != pg)) {
+	if (unlikely(READ_ONCE(m->current_pg) != pg)) {
 		/* Only update current_pgpath if pg changed */
 		spin_lock_irqsave(&m->lock, flags);
 		m->current_pgpath = pgpath;

@@ -390,7 +390,7 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes)
 	}
 
 	/* Were we instructed to switch PG? */
-	if (lockless_dereference(m->next_pg)) {
+	if (READ_ONCE(m->next_pg)) {
 		spin_lock_irqsave(&m->lock, flags);
 		pg = m->next_pg;
 		if (!pg) {

@@ -406,7 +406,7 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes)
 
 	/* Don't change PG until it has no remaining paths */
 check_current_pg:
-	pg = lockless_dereference(m->current_pg);
+	pg = READ_ONCE(m->current_pg);
 	if (pg) {
 		pgpath = choose_path_in_pg(m, pg, nr_bytes);
 		if (!IS_ERR_OR_NULL(pgpath))

@@ -473,7 +473,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	struct request *clone;
 
 	/* Do we need to select a new pgpath? */
-	pgpath = lockless_dereference(m->current_pgpath);
+	pgpath = READ_ONCE(m->current_pgpath);
 	if (!pgpath || !test_bit(MPATHF_QUEUE_IO, &m->flags))
 		pgpath = choose_pgpath(m, nr_bytes);
 

@@ -535,7 +535,7 @@ static int __multipath_map_bio(struct multipath *m, struct bio *bio, struct dm_m
 	bool queue_io;
 
 	/* Do we need to select a new pgpath? */
-	pgpath = lockless_dereference(m->current_pgpath);
+	pgpath = READ_ONCE(m->current_pgpath);
 	queue_io = test_bit(MPATHF_QUEUE_IO, &m->flags);
 	if (!pgpath || !queue_io)
 		pgpath = choose_pgpath(m, nr_bytes);

@@ -1804,7 +1804,7 @@ static int multipath_prepare_ioctl(struct dm_target *ti,
 	struct pgpath *current_pgpath;
 	int r;
 
-	current_pgpath = lockless_dereference(m->current_pgpath);
+	current_pgpath = READ_ONCE(m->current_pgpath);
 	if (!current_pgpath)
 		current_pgpath = choose_pgpath(m, 0);
 

@@ -1826,7 +1826,7 @@ static int multipath_prepare_ioctl(struct dm_target *ti,
 	}
 
 	if (r == -ENOTCONN) {
-		if (!lockless_dereference(m->current_pg)) {
+		if (!READ_ONCE(m->current_pg)) {
 			/* Path status changed, redo selection */
 			(void) choose_pgpath(m, 0);
 		}

@@ -1895,9 +1895,9 @@ static int multipath_busy(struct dm_target *ti)
 		return (m->queue_mode != DM_TYPE_MQ_REQUEST_BASED);
 
 	/* Guess which priority_group will be used at next mapping time */
-	pg = lockless_dereference(m->current_pg);
-	next_pg = lockless_dereference(m->next_pg);
-	if (unlikely(!lockless_dereference(m->current_pgpath) && next_pg))
+	pg = READ_ONCE(m->current_pg);
+	next_pg = READ_ONCE(m->next_pg);
+	if (unlikely(!READ_ONCE(m->current_pgpath) && next_pg))
 		pg = next_pg;
 
 	if (!pg) {

fs/dcache.c +2 -2

index f90141387f01..34c852af215c 100644
@@ -231,7 +231,7 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
 {
 	/*
 	 * Be careful about RCU walk racing with rename:
-	 * use 'lockless_dereference' to fetch the name pointer.
+	 * use 'READ_ONCE' to fetch the name pointer.
 	 *
 	 * NOTE! Even if a rename will mean that the length
 	 * was not loaded atomically, we don't care. The

@@ -245,7 +245,7 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
 	 * early because the data cannot match (there can
 	 * be no NUL in the ct/tcount data)
 	 */
-	const unsigned char *cs = lockless_dereference(dentry->d_name.name);
+	const unsigned char *cs = READ_ONCE(dentry->d_name.name);
 
 	return dentry_string_cmp(cs, ct, tcount);
 }

fs/overlayfs/ovl_entry.h +1 -1

index 878a750986dd..0f6809fa6628 100644
@@ -74,5 +74,5 @@ static inline struct ovl_inode *OVL_I(struct inode *inode)
 
 static inline struct dentry *ovl_upperdentry_dereference(struct ovl_inode *oi)
 {
-	return lockless_dereference(oi->__upperdentry);
+	return READ_ONCE(oi->__upperdentry);
 }

fs/overlayfs/readdir.c +1 -1

index 62e9b22a2077..0b389d330613 100644
@@ -754,7 +754,7 @@ static int ovl_dir_fsync(struct file *file, loff_t start, loff_t end,
 	if (!od->is_upper && OVL_TYPE_UPPER(ovl_path_type(dentry))) {
 		struct inode *inode = file_inode(file);
 
-		realfile = lockless_dereference(od->upperfile);
+		realfile = READ_ONCE(od->upperfile);
 		if (!realfile) {
 			struct path upperpath;
 

include/linux/compiler.h +1 -20

index e95a2631e545..f260ff39f90f 100644
@@ -340,6 +340,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
+	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
 	__u.__val;							\
 })
 #define READ_ONCE(x) __READ_ONCE(x, 1)

@@ -604,24 +605,4 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 	(volatile typeof(x) *)&(x); })
 #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
 
-/**
- * lockless_dereference() - safely load a pointer for later dereference
- * @p: The pointer to load
- *
- * Similar to rcu_dereference(), but for situations where the pointed-to
- * object's lifetime is managed by something other than RCU.  That
- * "something other" might be reference counting or simple immortality.
- *
- * The seemingly unused variable ___typecheck_p validates that @p is
- * indeed a pointer type by using a pointer to typeof(*p) as the type.
- * Taking a pointer to typeof(*p) again is needed in case p is void *.
- */
-#define lockless_dereference(p) \
-({ \
-	typeof(p) _________p1 = READ_ONCE(p); \
-	typeof(*(p)) *___typecheck_p __maybe_unused; \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
-	(_________p1); \
-})
-
 #endif /* __LINUX_COMPILER_H */

include/linux/rculist.h +2 -2

index b1fd8bf85fdc..3a2bb7d8ed4d 100644
@@ -274,7 +274,7 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
  */
 #define list_entry_rcu(ptr, type, member) \
-	container_of(lockless_dereference(ptr), type, member)
+	container_of(READ_ONCE(ptr), type, member)
 
 /**
  * Where are list_empty_rcu() and list_first_entry_rcu()?

@@ -367,7 +367,7 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * example is when items are added to the list, but never deleted.
  */
 #define list_entry_lockless(ptr, type, member) \
-	container_of((typeof(ptr))lockless_dereference(ptr), type, member)
+	container_of((typeof(ptr))READ_ONCE(ptr), type, member)
 
 /**
  * list_for_each_entry_lockless - iterate over rcu list of given type

include/linux/rcupdate.h +2 -2

index de50d8a4cf41..380a3aeb09d7 100644
@@ -346,7 +346,7 @@ static inline void rcu_preempt_sleep_check(void) { }
 #define __rcu_dereference_check(p, c, space) \
 ({ \
 	/* Dependency order vs. p above. */ \
-	typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
+	typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); \
 	RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
 	rcu_dereference_sparse(p, space); \
 	((typeof(*p) __force __kernel *)(________p1)); \

@@ -360,7 +360,7 @@ static inline void rcu_preempt_sleep_check(void) { }
 #define rcu_dereference_raw(p) \
 ({ \
 	/* Dependency order vs. p above. */ \
-	typeof(p) ________p1 = lockless_dereference(p); \
+	typeof(p) ________p1 = READ_ONCE(p); \
 	((typeof(*p) __force __kernel *)(________p1)); \
 })
 

kernel/events/core.c +2 -2

index 6bc21e202ae4..417812ce0099 100644
@@ -4231,7 +4231,7 @@ static void perf_remove_from_owner(struct perf_event *event)
 	 * indeed free this event, otherwise we need to serialize on
 	 * owner->perf_event_mutex.
 	 */
-	owner = lockless_dereference(event->owner);
+	owner = READ_ONCE(event->owner);
 	if (owner) {
 		/*
 		 * Since delayed_put_task_struct() also drops the last

@@ -4328,7 +4328,7 @@ int perf_event_release_kernel(struct perf_event *event)
 		 * Cannot change, child events are not migrated, see the
 		 * comment with perf_event_ctx_lock_nested().
 		 */
-		ctx = lockless_dereference(child->ctx);
+		ctx = READ_ONCE(child->ctx);
 		/*
 		 * Since child_mutex nests inside ctx::mutex, we must jump
 		 * through hoops. We start by grabbing a reference on the ctx.

kernel/seccomp.c +1 -1

index bb3a38005b9c..1daa8b61a268 100644
@@ -189,7 +189,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
 	u32 ret = SECCOMP_RET_ALLOW;
 	/* Make sure cross-thread synced filter points somewhere sane. */
 	struct seccomp_filter *f =
-			lockless_dereference(current->seccomp.filter);
+			READ_ONCE(current->seccomp.filter);
 
 	/* Ensure unexpected behavior doesn't result in failing open. */
 	if (unlikely(WARN_ON(f == NULL)))

kernel/task_work.c +1 -1

index 836a72a66fba..9a9f262fc53d 100644
@@ -67,7 +67,7 @@ task_work_cancel(struct task_struct *task, task_work_func_t func)
 	 * we raced with task_work_run(), *pprev == NULL/exited.
 	 */
 	raw_spin_lock_irqsave(&task->pi_lock, flags);
-	while ((work = lockless_dereference(*pprev))) {
+	while ((work = READ_ONCE(*pprev))) {
 		if (work->func != func)
 			pprev = &work->next;
 		else if (cmpxchg(pprev, work, work->next) == work)

mm/slab.h +1 -1

index 073362816acc..8894f811a89d 100644
@@ -258,7 +258,7 @@ cache_from_memcg_idx(struct kmem_cache *s, int idx)
 	 * memcg_caches issues a write barrier to match this (see
 	 * memcg_create_kmem_cache()).
 	 */
-	cachep = lockless_dereference(arr->entries[idx]);
+	cachep = READ_ONCE(arr->entries[idx]);
 	rcu_read_unlock();
 
 	return cachep;

Signature · 1 line

2.1.4

This patch consistently uses these macros in the arch
code, as well as explicitly namespacing pointers to page table entries from the entries themselves by using adopting a 'p' suffix for the former (as is sometimes used elsewhere in the kernel source).

Hi,

We recently had a crash report[1] on arm64 that involved a bad dereference in the page_vma_mapped code during ext4 writeback with THP active. I can reproduce this on -rc2:

[  254.032812] PC is at check_pte+0x20/0x170
[  254.032948] LR is at page_vma_mapped_walk+0x2e0/0x540
[...]
[  254.036114] Process doio (pid: 2463, stack limit = 0xffff00000f2e8000)
[  254.036361] Call trace:
[  254.038977] [<ffff000008233328>] check_pte+0x20/0x170
[  254.039137] [<ffff000008233758>] page_vma_mapped_walk+0x2e0/0x540
[  254.039332] [<ffff000008234adc>] page_mkclean_one+0xac/0x278
[  254.039489] [<ffff000008234d98>] rmap_walk_file+0xf0/0x238
[  254.039642] [<ffff000008236e74>] rmap_walk+0x64/0xa0
[  254.039784] [<ffff0000082370c8>] page_mkclean+0x90/0xa8
[  254.040029] [<ffff0000081f3c64>] clear_page_dirty_for_io+0x84/0x2a8
[  254.040311] [<ffff00000832f984>] mpage_submit_page+0x34/0x98
[  254.040518] [<ffff00000832fb4c>] mpage_process_page_bufs+0x164/0x170
[  254.040743] [<ffff00000832fc8c>] mpage_prepare_extent_to_map+0x134/0x2b8
[  254.040969] [<ffff00000833530c>] ext4_writepages+0x484/0xe30
[  254.041175] [<ffff0000081f6ab4>] do_writepages+0x44/0xe8
[  254.041372] [<ffff0000081e5bd4>] __filemap_fdatawrite_range+0xbc/0x110
[  254.041568] [<ffff0000081e5e68>] file_write_and_wait_range+0x48/0xd8
[  254.041739] [<ffff000008324310>] ext4_sync_file+0x80/0x4b8
[  254.041907] [<ffff0000082bd434>] vfs_fsync_range+0x64/0xc0
[  254.042106] [<ffff0000082332b4>] SyS_msync+0x194/0x1e8

After digging into the issue, I found that we appear to be racing with a concurrent pmd update in page_vma_mapped_walk, assumedly due a THP splitting operation. Looking at the code there:

	pvmw->pmd = pmd_offset(pud, pvmw->address);
	if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {
		[...]
	} else {
		if (!check_pmd(pvmw))
			return false;
	}
	if (!map_pte(pvmw))
		goto next_pte;

what happens in the crashing scenario is that we see all zeroes for the PMD in pmd_trans_huge(*pvmw->pmd), and so go to the 'else' case (migration isn't enabled, so the test is removed at compile-time). check_pmd then does:

	pmde = READ_ONCE(*pvmw->pmd);
	return pmd_present(pmde) && !pmd_trans_huge(pmde);

and reads a valid table entry for the PMD because the splitting has completed (i.e. the first dereference reads from the pmdp_invalidate in the splitting code, whereas the second dereferenced reads from the following pmd_populate). It returns true because we should descend to the PTE level in map_pte. map_pte does:

	pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);

which on arm64 (and this appears to be the same on x86) ends up doing:

	(pmd_page_paddr((*(pvmw->pmd))) + pte_index(pvmw->address) * sizeof(pte_t))

as part of its calculation. However, this is horribly broken because GCC inlines everything and reuses the register it loaded for the initial pmd_trans_huge check (when we loaded the value of zero) here, so we end up calculating a junk pointer and crashing when we dereference it. Disassembly at the end of the mail[2] for those who are curious.

The moral of the story is that read-after-read (same address) ordering *only* applies if READ_ONCE is used consistently. This means we need to fix page table dereferences in the core code as well as the arch code to avoid this problem. The two RFC patches in this series fix arm64 (which is a bigger fix that necessary since I clean things up too) and page_vma_mapped_walk.

Comments welcome.

Will

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-September/532786.html
[2]

// page_vma_mapped_walk
// pvmw->pmd = pmd_offset(pud, pvmw->address);
ldr x0, [x19, #24] // pvmw->pmd

// if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {

ldr     x1, [x0]		// *pvmw->pmd
cbz     x1, ffff0000082336a0 <page_vma_mapped_walk+0x228>
tbz     w1, #1, ffff000008233788 <page_vma_mapped_walk+0x310>	// pmd_trans_huge?

// else if (!check_pmd(pvmw))

ldr     x0, [x0]		// READ_ONCE in check_pmd
tst     x0, x24			// pmd_present?
b.eq    ffff000008233538 <page_vma_mapped_walk+0xc0>  // b.none
tbz     w0, #1, ffff000008233538 <page_vma_mapped_walk+0xc0>	// pmd_trans_huge?

// if (!map_pte(pvmw))
ldr x0, [x19, #16] // pvmw->address

// pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);
and x1, x1, #0xfffffffff000 // Reusing the old value of *pvmw->pmd!!!
[...]

--->8

Will Deacon (2):
arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables
mm: page_vma_mapped: Ensure pmd is loaded with READ_ONCE outside of
lock

 arch/arm64/include/asm/hugetlb.h     |   2 +-
 arch/arm64/include/asm/kvm_mmu.h     |  18 +--
 arch/arm64/include/asm/mmu_context.h |   4 +-
 arch/arm64/include/asm/pgalloc.h     |  42 +++---
 arch/arm64/include/asm/pgtable.h     |  29 ++--
 arch/arm64/kernel/hibernate.c        | 148 +++++++++---------
 arch/arm64/mm/dump.c                 |  54 ++++---
 arch/arm64/mm/fault.c                |  44 +++---
 arch/arm64/mm/hugetlbpage.c          |  94 ++++++------
 arch/arm64/mm/kasan_init.c           |  62 ++++----
 arch/arm64/mm/mmu.c                  | 281 ++++++++++++++++++-----------------
 arch/arm64/mm/pageattr.c             |  30 ++--
 mm/page_vma_mapped.c                 |  25 ++--
 13 files changed, 427 insertions(+), 406 deletions(-)

Signature · 1 line

2.1.4

Hi,

We recently had a crash report[1] on arm64 that involved a bad dereference in the page_vma_mapped code during ext4 writeback with THP active. I can reproduce this on -rc2:

[  254.032812] PC is at check_pte+0x20/0x170
[  254.032948] LR is at page_vma_mapped_walk+0x2e0/0x540
[...]
[  254.036114] Process doio (pid: 2463, stack limit = 0xffff00000f2e8000)
[  254.036361] Call trace:
[  254.038977] [<ffff000008233328>] check_pte+0x20/0x170
[  254.039137] [<ffff000008233758>] page_vma_mapped_walk+0x2e0/0x540
[  254.039332] [<ffff000008234adc>] page_mkclean_one+0xac/0x278
[  254.039489] [<ffff000008234d98>] rmap_walk_file+0xf0/0x238
[  254.039642] [<ffff000008236e74>] rmap_walk+0x64/0xa0
[  254.039784] [<ffff0000082370c8>] page_mkclean+0x90/0xa8
[  254.040029] [<ffff0000081f3c64>] clear_page_dirty_for_io+0x84/0x2a8
[  254.040311] [<ffff00000832f984>] mpage_submit_page+0x34/0x98
[  254.040518] [<ffff00000832fb4c>] mpage_process_page_bufs+0x164/0x170
[  254.040743] [<ffff00000832fc8c>] mpage_prepare_extent_to_map+0x134/0x2b8
[  254.040969] [<ffff00000833530c>] ext4_writepages+0x484/0xe30
[  254.041175] [<ffff0000081f6ab4>] do_writepages+0x44/0xe8
[  254.041372] [<ffff0000081e5bd4>] __filemap_fdatawrite_range+0xbc/0x110
[  254.041568] [<ffff0000081e5e68>] file_write_and_wait_range+0x48/0xd8
[  254.041739] [<ffff000008324310>] ext4_sync_file+0x80/0x4b8
[  254.041907] [<ffff0000082bd434>] vfs_fsync_range+0x64/0xc0
[  254.042106] [<ffff0000082332b4>] SyS_msync+0x194/0x1e8

After digging into the issue, I found that we appear to be racing with a concurrent pmd update in page_vma_mapped_walk, assumedly due a THP splitting operation. Looking at the code there:

	pvmw->pmd = pmd_offset(pud, pvmw->address);
	if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {
		[...]
	} else {
		if (!check_pmd(pvmw))
			return false;
	}
	if (!map_pte(pvmw))
		goto next_pte;

what happens in the crashing scenario is that we see all zeroes for the PMD in pmd_trans_huge(*pvmw->pmd), and so go to the 'else' case (migration isn't enabled, so the test is removed at compile-time). check_pmd then does:

	pmde = READ_ONCE(*pvmw->pmd);
	return pmd_present(pmde) && !pmd_trans_huge(pmde);

and reads a valid table entry for the PMD because the splitting has completed (i.e. the first dereference reads from the pmdp_invalidate in the splitting code, whereas the second dereferenced reads from the following pmd_populate). It returns true because we should descend to the PTE level in map_pte. map_pte does:

	pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);

which on arm64 (and this appears to be the same on x86) ends up doing:

	(pmd_page_paddr((*(pvmw->pmd))) + pte_index(pvmw->address) * sizeof(pte_t))

as part of its calculation. However, this is horribly broken because GCC inlines everything and reuses the register it loaded for the initial pmd_trans_huge check (when we loaded the value of zero) here, so we end up calculating a junk pointer and crashing when we dereference it. Disassembly at the end of the mail[2] for those who are curious.

The moral of the story is that read-after-read (same address) ordering *only* applies if READ_ONCE is used consistently. This means we need to fix page table dereferences in the core code as well as the arch code to avoid this problem. The two RFC patches in this series fix arm64 (which is a bigger fix that necessary since I clean things up too) and page_vma_mapped_walk.

Comments welcome.

Will

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-September/532786.html
[2]

// page_vma_mapped_walk
// pvmw->pmd = pmd_offset(pud, pvmw->address);
ldr x0, [x19, #24] // pvmw->pmd

// if (pmd_trans_huge(*pvmw->pmd) || is_pmd_migration_entry(*pvmw->pmd)) {

ldr     x1, [x0]		// *pvmw->pmd
cbz     x1, ffff0000082336a0 <page_vma_mapped_walk+0x228>
tbz     w1, #1, ffff000008233788 <page_vma_mapped_walk+0x310>	// pmd_trans_huge?

// else if (!check_pmd(pvmw))

ldr     x0, [x0]		// READ_ONCE in check_pmd
tst     x0, x24			// pmd_present?
b.eq    ffff000008233538 <page_vma_mapped_walk+0xc0>  // b.none
tbz     w0, #1, ffff000008233538 <page_vma_mapped_walk+0xc0>	// pmd_trans_huge?

// if (!map_pte(pvmw))
ldr x0, [x19, #16] // pvmw->address

// pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);
and x1, x1, #0xfffffffff000 // Reusing the old value of *pvmw->pmd!!!
[...]

--->8

Will Deacon (2):
arm64: mm: Use READ_ONCE/WRITE_ONCE when accessing page tables
mm: page_vma_mapped: Ensure pmd is loaded with READ_ONCE outside of
lock

 arch/arm64/include/asm/hugetlb.h     |   2 +-
 arch/arm64/include/asm/kvm_mmu.h     |  18 +--
 arch/arm64/include/asm/mmu_context.h |   4 +-
 arch/arm64/include/asm/pgalloc.h     |  42 +++---
 arch/arm64/include/asm/pgtable.h     |  29 ++--
 arch/arm64/kernel/hibernate.c        | 148 +++++++++---------
 arch/arm64/mm/dump.c                 |  54 ++++---
 arch/arm64/mm/fault.c                |  44 +++---
 arch/arm64/mm/hugetlbpage.c          |  94 ++++++------
 arch/arm64/mm/kasan_init.c           |  62 ++++----
 arch/arm64/mm/mmu.c                  | 281 ++++++++++++++++++-----------------
 arch/arm64/mm/pageattr.c             |  30 ++--
 mm/page_vma_mapped.c                 |  25 ++--
 13 files changed, 427 insertions(+), 406 deletions(-)

The moral of the story is that read-after-read (same address) ordering *only* applies if READ_ONCE is used consistently. This means we need to fix page table dereferences in the core code as well as the arch code to avoid this problem. The two RFC patches in this series fix arm64 (which is a bigger fix that necessary since I clean things up too) and page_vma_mapped_walk.

Comments welcome.

Quoted 6 lines · Will Deacon

The moral of the story is that read-after-read (same address) ordering *only* applies if READ_ONCE is used consistently. This means we need to fix page table dereferences in the core code as well as the arch code to avoid this problem. The two RFC patches in this series fix arm64 (which is a bigger fix that necessary since I clean things up too) and page_vma_mapped_walk.

Quoted 1 line

Comments welcome.

Thanks for this Will. I'll echo Timur's comment that it would be ideal to split this up into the critical piece needed for ordering access/update to the PMD in the face of a THP split and separately have the cosmetic cleanups. Needless to say, we've got a bunch of people who are tracking this one and tracking it ready for backport. We just got THP re-enabled so I'm pretty keen that we not have to disable again.

The full fix isn't just cosmetic; it's also addressing the wider problem of unannotated racing page table accesses outside of the specific failure case we've run into.

Quoted 3 lines · Will Deacon

The full fix isn't just cosmetic; it's also addressing the wider problem of unannotated racing page table accesses outside of the specific failure case we've run into.

Let us know if there are additional tests we should be running on the Red Hat end. We've got high hundreds of ARM server systems at this point, including pretty much everything out there.