Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1413057 > unrolled thread

[tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [tip:locking/core] locking/barriers: Validate  lockless_dereference() is used on a pointer type tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2016-06-03 13:00 +0200
    Re: [tip:locking/core] locking/barriers: Validate  lockless_dereference() is used on a pointer type Mateusz Guzik <mguzik@redhat.com> - 2016-06-06 23:40 +0200
      Re: [tip:locking/core] locking/barriers: Validate  lockless_dereference() is used on a pointer type Peter Zijlstra <peterz@infradead.org> - 2016-06-07 09:20 +0200

#1413057 — [tip:locking/core] locking/barriers: Validate lockless_dereference() is used on a pointer type

Commit-ID:  25841ee0e9d2a1d952828138416701f20ea831eb
Gitweb:     http://git.kernel.org/tip/25841ee0e9d2a1d952828138416701f20ea831eb
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Sun, 22 May 2016 12:48:27 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
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.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20160522104827.GP3193@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/compiler.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 793c082..e9c6417 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -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); \

[toc] | [next] | [standalone]

#1415498

On Fri, Jun 03, 2016 at 03:58:09AM -0700, tip-bot for Peter Zijlstra wrote:
> Commit-ID:  25841ee0e9d2a1d952828138416701f20ea831eb
> Gitweb:     http://git.kernel.org/tip/25841ee0e9d2a1d952828138416701f20ea831eb
> Author:     Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Sun, 22 May 2016 12:48:27 +0200
> Committer:  Ingo Molnar <mingo@kernel.org>
> 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.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/20160522104827.GP3193@twins.programming.kicks-ass.net
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/compiler.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 793c082..e9c6417 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -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); \


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:
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index e9c6417..06f27fd 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -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); \
 })

-- 
Mateusz Guzik

[toc] | [prev] | [next] | [standalone]

#1415780

On Mon, Jun 06, 2016 at 11:31:39PM +0200, Mateusz Guzik wrote:
> 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. 

Indeed, bad that; yes I think the below will work, will you send a
proper patch so I can apply?

> The simplest fix I see does the assignment later, that is:
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index e9c6417..06f27fd 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -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); \
>  })
> 
> -- 
> Mateusz Guzik

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web