seqlock: fix raw_read_seqcount_latch()
Subsystems (3)
PATCH 1/1 seqlock: fix raw_read_seqcount_latch()
lockless_dereference() is supposed to take pointer not integer.
---
include/linux/seqlock.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/seqlock.h +2 -2
@@ -277,7 +277,7 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)static inline int raw_read_seqcount_latch (seqcount_t *s) { -return lockless_dereference (s->sequence); +return lockless_dereference (s)->sequence; }/**
@@ -331,7 +331,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s)* unsigned seq, idx; * * do { -* seq = lockless_dereference(latch->seq); +* seq = lockless_dereference(latch)->seq; * * idx = seq & 0x01; * entry = data_query(latch->data[idx], ...);
Peter Zijlstra
maintainerQuoted 1 line · Alexey Dobriyan
lockless_dereference()is supposed to take pointer not integer.
Urgh :/
Is there any way we can make lockless_dereference() issue a warning if we don't feed it a pointer?
Would something like so work? All pointer types should silently cast to void * while integer (and others) should refuse to.
include/linux/compiler.h +1 -0
@@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s*/ #define lockless_dereference (p) \ ({ \ + __maybe_unused void * _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \
Quoted 10 lines · Peter Zijlstra
Quoted 1 line · Alexey Dobriyan
lockless_dereference()is supposed to take pointer not integer.Urgh :/
Is there any way we can make
lockless_dereference()issue a warning if we don't feed it a pointer?Would something like so work? All pointer types should silently cast to void * while integer (and others) should refuse to.
This works (and spammy enough in case of seqlock, which is good) but not for "unsigned long":
include/linux/percpu-refcount.h:146:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion] percpu_ptr = lockless_dereference(ref->percpu_count_ptr);
Quoted 10 lines
include/linux/compiler.h +1
-0@@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s*/ #define lockless_dereference (p) \ ({ \ + __maybe_unused void * _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \
Peter Zijlstra
maintainerQuoted 9 lines · Alexey Dobriyan
Quoted 8 lines · Peter Zijlstra
Quoted 1 line · Alexey Dobriyan
lockless_dereference()is supposed to take pointer not integer.Urgh :/
Is there any way we can make
lockless_dereference()issue a warning if we don't feed it a pointer?
Quoted 8 lines
Quoted 2 lines
Would something like so work? All pointer types should silently cast to void * while integer (and others) should refuse to.
This works (and spammy enough in case of seqlock, which is good) but not for "unsigned long":
include/linux/percpu-refcount.h:146:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion] percpu_ptr = lockless_dereference(ref->percpu_count_ptr);
TJ; would you prefer casting or not using lockless_dereference() here?
Quoted 11 lines
Quoted 11 lines
include/linux/compiler.h +1
-0@@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s*/ #define lockless_dereference (p) \ ({ \ + __maybe_unused void * _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \
Tejun Heo
maintainerHello,
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines
include/linux/percpu-refcount.h:146:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion] percpu_ptr = lockless_dereference(ref->percpu_count_ptr);TJ; would you prefer casting or not using
lockless_dereference()here?
Casting is nasty - *(unsigned long __percpu **)& - because the macro expects an lvalue. I think it'd be better to revert to opencoding READ_ONCE() and barrier there. It's a pretty special case anyway.
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerPATCH 1/1 percpu: Revert ("percpu: Replace smp_read_barrier_depends() with lockless_dereference()")
lockless_dereference() is planned to grow a sanity check to ensure that the input parameter is a pointer. __ref_is_percpu() passes in an unsinged long value which is a combination of a pointer and a flag. While it can be casted to a pointer lvalue, the casting looks messy and it's a special case anyway. Let's revert back to open-coding READ_ONCE() and explicit barrier.
This doesn't cause any functional changes.
---
So, something like this. Please feel free to include in the series.
Thanks.
include/linux/percpu-refcount.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
include/linux/percpu-refcount.h +5 -7
@@ -136,14 +136,12 @@ static inline bool __ref_is_percpu(struct percpu_ref *ref,* used as a pointer. If the compiler generates a separate fetch * when using it as a pointer, __PERCPU_REF_ATOMIC may be set in * between contaminating the pointer value, meaning that -* ACCESS_ONCE() is required when fetching it. -* -* Also, we need a data dependency barrier to be paired with -* smp_store_release() in __percpu_ref_switch_to_percpu(). -* -* Use lockless deref which contains both. +* READ_ONCE() is required when fetching it. */ - percpu_ptr =lockless_dereference (ref->percpu_count_ptr); + percpu_ptr =READ_ONCE (ref->percpu_count_ptr); + +/* paired with smp_store_release() in __percpu_ref_switch_to_percpu() */ +smp_read_barrier_depends ();/* * Theoretically, the following could test just ATOMIC; however,
tip-bot for Peter Zijlstra
merge bot[tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type
In reply to Peter Zijlstralocking/barriers: Validate lockless_dereference() is used on a pointer type
Commit-ID: 25841ee0e9d2a1d952828138416701f20ea831eb Gitweb: http://git.kernel.org/tip/25841ee0e9d2a1d952828138416701f20ea831eb Author: Peter Zijlstra <[email protected]> AuthorDate: Sun, 22 May 2016 12:48:27 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 3 Jun 2016 12:06:11 +0200
locking/barriers: Validate lockless_dereference() is used on a pointer type
Add a cast to void * to validate the argument @p is indeed a pointer.
--- include/linux/compiler.h | 4 ++++ 1 file changed, 4 insertions(+)
include/linux/compiler.h +4 -0
@@ -545,9 +545,13 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s* 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 void * variable is to validate @p is indeed a pointer +* type. All pointer types silently cast to void *. */ #define lockless_dereference (p) \ ({ \ + __maybe_unused const void * const _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \
Re: [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type
Quoted 33 lines · tip-bot for Peter Zijlstra
Commit-ID: 25841ee0e9d2a1d952828138416701f20ea831eb Gitweb: http://git.kernel.org/tip/25841ee0e9d2a1d952828138416701f20ea831eb Author: Peter Zijlstra <[email protected]> AuthorDate: Sun, 22 May 2016 12:48:27 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 3 Jun 2016 12:06:11 +0200locking/barriers: Validate
lockless_dereference()is used on a pointer typeAdd a cast to void * to validate the argument @p is indeed a pointer.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>Cc Alexey Dobriyan <[email protected]>Cc Andrew Morton <[email protected]>Cc Linus Torvalds <[email protected]>Cc Paul E. McKenney <[email protected]>Cc Paul McKenney <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Ingo Molnar <[email protected]>--- include/linux/compiler.h | 4 ++++ 1 file changed, 4 insertions(+)include/linux/compiler.h +0
-0@@ -545,9 +545,13 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s* 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.
include/linux/compiler.h static __always_inline void __write_once_size(volatile void *p, void *res, int s · line 557
Quoted 10 lines
include/linux/compiler.h +4
-0+* +* The seemingly unused void * variable is to validate @p is indeed a pointer +* type. All pointer types silently cast to void *. */ #define lockless_dereference (p) \ ({ \ + __maybe_unused const void * const _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \
This causes issues, e.g.:
BUG: KASAN: user-memory-access on address 000060ff95001931^M Read of size 1 by task NetworkManager/897^M CPU: 1 PID: 897 Comm: NetworkManager Not tainted 4.7.0-rc1dupa+ #355^M Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011^M ffff88003a1f8040 0000000017d08b23 ffff880033aff4b0 ffffffff8187b2f8^M ffff88005b44f2c8 ffff880033aff548 ffff880033aff538 ffffffff813506a6^M ffffffff81167e5esystemd[1]: sshd.service: Forked /usr/sbin/sshd as 904^M ^M ffffed000675fe9d 0000000000000286 ffff880033aff588^M Call Trace:^M [<ffffffff8187b2f8>] dump_stack+0x85/0xcd^M [<ffffffff813506a6>] kasan_report_error+0x456/0x560^M [<ffffffff81167e5e>] ? vprintk_default+0x3e/0x60^M [<ffffffff812a19e3>] ? printk+0xa8/0xd8^M [<ffffffff812a193b>] ? power_down+0xa9/0xa9^M [<ffffffff81350d48>] kasan_report+0x58/0x60^M [<ffffffff81e84fe5>] ? leaf_walk_rcu+0x235/0x2d0^M [<ffffffff8134f447>] __asan_load1+0x47/0x50^M [<ffffffff81e84fe5>] leaf_walk_rcu+0x235/0x2d0^M
[snip]
The reason is that leaf_walk_rcu does get_child_rcu(pn, cindex++), which ends up in lockless_dereference as pn[cindex++], which is now evaluated twice.
The simplest fix I see does the assignment later, that is:
include/linux/compiler.h +1 -1
@@ -551,8 +551,8 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s*/ #define lockless_dereference (p) \ ({ \ - __maybe_unused const void * const _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ + __maybe_unused const void * const _________p2 = _________p1; \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \ })
Signature · 1 line
Mateusz Guzik
Peter Zijlstra
maintainerRe: [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type
Quoted 3 lines · Mateusz Guzik
The reason is that
leaf_walk_rcudoesget_child_rcu(pn, cindex++), which ends up inlockless_dereferenceas pn[cindex++], which is now evaluated twice.
Indeed, bad that; yes I think the below will work, will you send a proper patch so I can apply?
Quoted 18 lines
The simplest fix I see does the assignment later, that is:
include/linux/compiler.h +1
-1@@ -551,8 +551,8 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s*/ #define lockless_dereference (p) \ ({ \ - __maybe_unused const void * const _________p2 = p; \ typeof(p) _________p1 = READ_ONCE(p); \ + __maybe_unused const void * const _________p2 = _________p1; \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \ })Signature · 2 lines
-- Mateusz Guzik
tip-bot for Peter Zijlstra
merge bot[tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type
In reply to Peter Zijlstralocking/barriers: Validate lockless_dereference() is used on a pointer type
Commit-ID: 331b6d8c7afc2e5b900b9dcd850c265e1ba8d8e7 Gitweb: http://git.kernel.org/tip/331b6d8c7afc2e5b900b9dcd850c265e1ba8d8e7 Author: Peter Zijlstra <[email protected]> AuthorDate: Sun, 22 May 2016 12:48:27 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 8 Jun 2016 14:22:47 +0200
locking/barriers: Validate lockless_dereference() is used on a pointer type
Use the type to validate the argument @p is indeed a pointer type.
--- include/linux/compiler.h | 4 ++++ 1 file changed, 4 insertions(+)
include/linux/compiler.h +4 -0
@@ -545,10 +545,14 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s* 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 void * variable is to validate @p is indeed a pointer +* type. All pointer types silently cast to void *. */ #define lockless_dereference (p) \ ({ \ typeof(p) _________p1 = READ_ONCE(p); \ + __maybe_unused const void * const _________p2 = _________p1; \ smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \ })
Peter Zijlstra
maintainerQuoted 20 lines · Alexey Dobriyan
lockless_dereference()is supposed to take pointer not integer.Signed-off-by Alexey Dobriyan <[email protected]>---
include/linux/seqlock.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)include/linux/seqlock.h +1
-1@@ -277,7 +277,7 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)static inline int raw_read_seqcount_latch (seqcount_t *s) { -return lockless_dereference (s->sequence); +return lockless_dereference (s)->sequence; }/**
include/linux/seqlock.h static inline int raw_read_seqcount_latch(seqcount_t *s) · line 337
Quoted 8 lines
include/linux/seqlock.h +1
-1@@ -331,7 +331,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s)* unsigned seq, idx; * * do { -* seq = lockless_dereference(latch->seq); +* seq = lockless_dereference(latch)->seq; * * idx = seq & 0x01; * entry = data_query(latch->data[idx], ...);
So while the code was dubious; I it is now wrong, but my head hurts.
I'll queue the below, TJs per-cpu change and the lockless_dereference() void * cast trick.
---
Subject: seqcount: Re-fix raw_read_seqcount_latch()
Commit 50755bc1c305 ("seqlock: fix raw_read_seqcount_latch()") broke raw_read_seqcount_latch().
If you look at the comment that was modified; the thing that changes is the seq count, not the latch pointer.
* void latch_modify(struct latch_struct *latch, ...)
* {
* smp_wmb(); <- Ensure that the last data[1] update is visible
* latch->seq++;
* smp_wmb(); <- Ensure that the seqcount update is visible
*
* modify(latch->data[0], ...);
*
* smp_wmb(); <- Ensure that the data[0] update is visible
* latch->seq++;
* smp_wmb(); <- Ensure that the seqcount update is visible
*
* modify(latch->data[1], ...);
* }
*
* The query will have a form like:
*
* struct entry *latch_query(struct latch_struct *latch, ...)
* {
* struct entry *entry;
* unsigned seq, idx;
*
* do {
* seq = lockless_dereference(latch->seq);
So here we have:
seq = READ_ONCE(latch->seq); smp_read_barrier_depends();
Which is exactly what we want; the new code:
seq = ({ p = READ_ONCE(latch);
smp_read_barrier_depends(); p })->seq;is just wrong; because it looses the volatile read on seq, which can now be torn or worse 'optimized'. And the read_depend barrier is also placed wrong, we want it after the load of seq, to match the above data[] up-to-date wmb()s.
Such that when we dereference latch->data[] below, we're guaranteed to observe the right data.
*
* idx = seq & 0x01;
* entry = data_query(latch->data[idx], ...);
*
* smp_rmb();
* } while (seq != latch->seq);
*
* return entry;
* }
So yes, not passing a pointer is not pretty, but the code was correct, and isn't anymore now.
Change to explicit READ_ONCE()+smp_read_barrier_depends() to avoid confusion and allow strict lockless_dereference() checking.
--- include/linux/seqlock.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
include/linux/seqlock.h +4 -2
@@ -277,7 +277,9 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)static inline int raw_read_seqcount_latch (seqcount_t *s) { -return lockless_dereference (s)->sequence; +int seq =READ_ONCE (s->sequence); +smp_read_barrier_depends (); +return seq; }/**
@@ -331,7 +333,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s)* unsigned seq, idx; * * do { -* seq = lockless_dereference(latch)->seq; +* seq = raw_read_seqcount_latch(&latch->seq); * * idx = seq & 0x01; * entry = data_query(latch->data[idx], ...);
tip-bot for Peter Zijlstra
merge bot[tip:locking/core] locking/seqcount: Re-fix raw_read_seqcount_latch()
locking/seqcount: Re-fix raw_read_seqcount_latch()
Commit-ID: 55eed755c6e30a89be3a791a6b0ad208aadd9bdc Gitweb: http://git.kernel.org/tip/55eed755c6e30a89be3a791a6b0ad208aadd9bdc Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 27 May 2016 13:11:17 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 3 Jun 2016 08:37:25 +0200
locking/seqcount: Re-fix raw_read_seqcount_latch()
Commit 50755bc1c305 ("seqlock: fix raw_read_seqcount_latch()") broke raw_read_seqcount_latch().
If you look at the comment that was modified; the thing that changes is the seq count, not the latch pointer.
* void latch_modify(struct latch_struct *latch, ...)
* {
* smp_wmb(); <- Ensure that the last data[1] update is visible
* latch->seq++;
* smp_wmb(); <- Ensure that the seqcount update is visible
*
* modify(latch->data[0], ...);
*
* smp_wmb(); <- Ensure that the data[0] update is visible
* latch->seq++;
* smp_wmb(); <- Ensure that the seqcount update is visible
*
* modify(latch->data[1], ...);
* }
*
* The query will have a form like:
*
* struct entry *latch_query(struct latch_struct *latch, ...)
* {
* struct entry *entry;
* unsigned seq, idx;
*
* do {
* seq = lockless_dereference(latch->seq);
So here we have:
seq = READ_ONCE(latch->seq); smp_read_barrier_depends();
Which is exactly what we want; the new code:
seq = ({ p = READ_ONCE(latch);
smp_read_barrier_depends(); p })->seq;is just wrong; because it looses the volatile read on seq, which can now be torn or worse 'optimized'. And the read_depend barrier is also placed wrong, we want it after the load of seq, to match the above data[] up-to-date wmb()s.
Such that when we dereference latch->data[] below, we're guaranteed to observe the right data.
*
* idx = seq & 0x01;
* entry = data_query(latch->data[idx], ...);
*
* smp_rmb();
* } while (seq != latch->seq);
*
* return entry;
* }
So yes, not passing a pointer is not pretty, but the code was correct, and isn't anymore now.
Change to explicit READ_ONCE()+smp_read_barrier_depends() to avoid confusion and allow strict lockless_dereference() checking.
--- include/linux/seqlock.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
include/linux/seqlock.h +5 -2
@@ -277,7 +277,10 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)static inline int raw_read_seqcount_latch (seqcount_t *s) { -return lockless_dereference (s)->sequence; +int seq =READ_ONCE (s->sequence); +/* Pairs with the first smp_wmb() in raw_write_seqcount_latch() */ +smp_read_barrier_depends (); +return seq; }/**
@@ -331,7 +334,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s)* unsigned seq, idx; * * do { -* seq = lockless_dereference(latch)->seq; +* seq = raw_read_seqcount_latch(&latch->seq); * * idx = seq & 0x01; * entry = data_query(latch->data[idx], ...);